Check if rerun-if-changed points to a directory. #8973
Conversation
|
r? @Eh2406 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Basically looks good to me. I worry about the performance of scanning large folder trees on widows, but the correctness is probably worth it. |
src/cargo/util/paths.rs
Outdated
| let max_meta = walkdir::WalkDir::new(path) | ||
| .follow_links(true) | ||
| .into_iter() | ||
| .filter_map(|e| e.ok()) |
There was a problem hiding this comment.
I'm a little nervous about ignoring errors here and below in unwrap_or_else. I suppose though that the thinking is that if Cargo can't read the file then the build script probably also couldn't read the file so it couldn't be considered input? I think it'd be good in any case to add some comments here about why errors are ignored.
There was a problem hiding this comment.
Correct, my thinking was that if Cargo can't access it, the build script can't either.
I pushed a commit with more comments, and added debug logging just in case. It makes the code a little longer than I'd like, but such is life with good error handling.
| match e.metadata() { | ||
| Ok(target_meta) => { | ||
| let target_mtime = FileTime::from_last_modification_time(&target_meta); | ||
| Some(sym_mtime.max(target_mtime)) |
There was a problem hiding this comment.
This seems a bit odd to me, if walkdir follows symlinks then how come we need to manually check the mtime of the symlink target? If the symlink points to a directory doesn't that mean we're doing the same thing as before where we're looking at the mtime of a directory which isn't too useful?
There was a problem hiding this comment.
If you have:
somedir/
foo.txt
bar.txt
thing -> foo.txt
If you run a build, it will use the max mtime of those 3 files. If you then modify thing to point to bar.txt, that will update the mtime of thing, and will trigger a rebuild. If this didn't look at both the symlink and the target, it would only read the mtime of foo.txt and bar.txt, and Cargo would fail to trigger a rebuild.
If the thing symlink pointed to a directory, yes it will read the mtime of the directory. But then, walkdir will traverse into that directory and scan all the contents. Also, reading the mtime of the directory itself is important for deleting files. Otherwise, Cargo wouldn't know that a file was deleted.
There was a problem hiding this comment.
Ah I see. I didn't actually realized until just before this that the mtime of directories was important. I was mostly wondering here why it dealt with both the target and the source since for file-based symlinks I think the symlink mtime itself should be appropriate, but even then I can see how it would matter if the file is located outside of the directory.
There was a problem hiding this comment.
Also, reading the mtime of the directory itself is important for deleting files. Otherwise, Cargo wouldn't know that a file was deleted.
This seems like an error-prone way to find out a file was deleted. I think instead Cargo should cache the list of files in the directory (recursively) and consider the directory changed if any file that is in that cached list doesn't exist (or a new file exists that isn't on the list), in addition to the file timestamping.
| if !meta.is_dir() { | ||
| return Ok(FileTime::from_last_modification_time(&meta)); | ||
| } | ||
| let max_meta = walkdir::WalkDir::new(path) |
There was a problem hiding this comment.
I'm actually a little surprised we already had this dependency...
|
@bors: r+ |
|
📌 Commit 4c68a4a has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
I really appreciate this, and I've been wanting to use something like this in several crates. This kind of thing also makes it more urgent that we have some way to declare a minimum Cargo version for a crate. If a crate uses this with an older version of Cargo, it'll just silently not rebuild. What's the most minimal mechanism we could use to enforce a minimum Cargo version in a crate? Would it be unreasonable to just add a |
|
I don't think we'd want to do that as a PR given all the past conversations about minimum versions, personally, so if we'd want to pursue that I think we'd want to go the RFC route. |
Update cargo 4 commits in d274fcf862b89264fa2c6b917b15230705257317..a3c2627fbc2f5391c65ba45ab53b81bf71fa323c 2020-12-07 23:08:44 +0000 to 2020-12-14 17:21:26 +0000 - Check if rerun-if-changed points to a directory. (rust-lang/cargo#8973) - Implement external credential process. (RFC 2730) (rust-lang/cargo#8934) - Revert recent build-std vendoring changes (rust-lang/cargo#8968) - Fix the unit dependency graph with dev-dependency `links` (rust-lang/cargo#8969)
|
@alexcrichton I wasn't suggesting it could be done without an RFC; I was asking how minimal that RFC might be able to be. |
Pkgsrc changes:
* Adjust patches, re-compute line offsets, fix capitalization.
* Remove i686/FreeBSD support, no longer provided upstream.
* Bump bootstraps to 1.49.0.
* Change USE_TOOLS from bsdtar to gtar.
* Reduce diffs to pkgsrc-wip package patches.
* Allow rust.BUILD_TARGET to override automatic choice of target.
* Add an i586/NetBSD (pentium) bootstrap variant (needs testing),
not yet added as bootstrap since 1.49 doesn't have that variant.
Upstream changes:
Version 1.50.0 (2021-02-11)
============================
Language
-----------------------
- [You can now use `const` values for `x` in `[x; N]` array
expressions.][79270] This has been technically possible since
1.38.0, as it was unintentionally stabilized.
- [Assignments to `ManuallyDrop<T>` union fields are now considered
safe.][78068]
Compiler
-----------------------
- [Added tier 3\* support for the `armv5te-unknown-linux-uclibceabi`
target.][78142]
- [Added tier 3 support for the `aarch64-apple-ios-macabi` target.][77484]
- [The `x86_64-unknown-freebsd` is now built with the full toolset.][79484]
\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.
Libraries
-----------------------
- [`proc_macro::Punct` now implements `PartialEq<char>`.][78636]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
arrays of any length.][74989]
- [On Unix platforms, the `std::fs::File` type now has a "niche"
of `-1`.][74699] This value cannot be a valid file descriptor,
and now means `Option<File>` takes up the same amount of space
as `File`.
Stabilized APIs
---------------
- [`bool::then`]
- [`btree_map::Entry::or_insert_with_key`]
- [`f32::clamp`]
- [`f64::clamp`]
- [`hash_map::Entry::or_insert_with_key`]
- [`Ord::clamp`]
- [`RefCell::take`]
- [`slice::fill`]
- [`UnsafeCell::get_mut`]
The following previously stable methods are now `const`.
- [`IpAddr::is_ipv4`]
- [`IpAddr::is_ipv6`]
- [`Layout::size`]
- [`Layout::align`]
- [`Layout::from_size_align`]
- `pow` for all integer types.
- `checked_pow` for all integer types.
- `saturating_pow` for all integer types.
- `wrapping_pow` for all integer types.
- `next_power_of_two` for all unsigned integer types.
- `checked_power_of_two` for all unsigned integer types.
Cargo
-----------------------
- [Added the `[build.rustc-workspace-wrapper]` option.][cargo/8976]
This option sets a wrapper to execute instead of `rustc`, for
workspace members only.
- [`cargo:rerun-if-changed` will now, if provided a directory, scan the entire
contents of that directory for changes.][cargo/8973]
- [Added the `--workspace` flag to the `cargo update` command.][cargo/8725]
Misc
----
- [The search results tab and the help button are focusable with
keyboard in rustdoc.][79896]
- [Running tests will now print the total time taken to execute.][75752]
Compatibility Notes
-------------------
- [The `compare_and_swap` method on atomics has been deprecated.][79261]
It's recommended to use the `compare_exchange` and
`compare_exchange_weak` methods instead.
- [Changes in how `TokenStream`s are checked have fixed some cases
where you could write unhygenic `macro_rules!` macros.][79472]
- [`#![test]` as an inner attribute is now considered unstable like
other inner macro attributes, and reports an error by default
through the `soft_unstable` lint.][79003]
- [Overriding a `forbid` lint at the same level that it was set is
now a hard error.][78864]
- [Dropped support for all cloudabi targets.][78439]
- [You can no longer intercept `panic!` calls by supplying your
own macro.][78343] It's recommended to use the `#[panic_handler]`
attribute to provide your own implementation.
- [Semi-colons after item statements (e.g. `struct Foo {};`) now
produce a warning.][78296]
[74989]: rust-lang/rust#74989
[79261]: rust-lang/rust#79261
[79896]: rust-lang/rust#79896
[79484]: rust-lang/rust#79484
[79472]: rust-lang/rust#79472
[79270]: rust-lang/rust#79270
[79003]: rust-lang/rust#79003
[78864]: rust-lang/rust#78864
[78636]: rust-lang/rust#78636
[78439]: rust-lang/rust#78439
[78343]: rust-lang/rust#78343
[78296]: rust-lang/rust#78296
[78068]: rust-lang/rust#78068
[75752]: rust-lang/rust#75752
[74699]: rust-lang/rust#74699
[78142]: rust-lang/rust#78142
[77484]: rust-lang/rust#77484
[cargo/8976]: rust-lang/cargo#8976
[cargo/8973]: rust-lang/cargo#8973
[cargo/8725]: rust-lang/cargo#8725
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv6
[`Layout::align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.align
[`Layout::from_size_align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.from_size_align
[`Layout::size`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.size
[`Ord::clamp`]: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html#method.clamp
[`RefCell::take`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.take
[`UnsafeCell::get_mut`]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#method.get_mut
[`bool::then`]: https://doc.rust-lang.org/stable/std/primitive.bool.html#method.then
[`btree_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.or_insert_with_key
[`f32::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.clamp
[`f64::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.clamp
[`hash_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/hash_map/enum.Entry.html#method.or_insert_with_key
[`slice::fill`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.fill
cargo:rerun-if-changed applies recursively: rust-lang/cargo#8973
This changes it so that if a build script emits
cargo:rerun-if-changedpointing to a directory, then Cargo will scan the entire directory for changes (instead of just looking at the mtime of the directory itself). I think this is more useful, as otherwise build scripts have to recreate this logic.I've tried to make it semi-intelligent in the face of symbolic links. It checks the mtime of the link and its target, and follows the link if it points to a directory.
There are a few other edge cases. For example, if it doesn't have permission for a directory, it will skip it. I think this is relatively reasonable, though it's hard to say for sure.