-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework rustc output file tracking. #8210
Conversation
r? @Eh2406 (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing on this @ehuss! I only sort of lightly skimmed the internal refactorings. It all sounds fine at a high-level and everything internally looked "correct enough" that it should either be covered by tests or we'll figure it out on nightly pretty soon anyway.
I'm left with really only one minor comment, but otherwise this I think is a great improvement to cargo clean -p
, so r=me when ready
src/cargo/ops/cargo_clean.rs
Outdated
if matches.is_empty() { | ||
anyhow::bail!("package ID specification `{}` matched no packages", spec); | ||
} | ||
packages.extend(pkg_set.get_many(matches)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the get_many
here starts a new download session each time, so maybe these could all be collected from the opts.spec.iter()
loop and one get_many
could be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
When a unit does not have Metadata, the computation of fingerprints depends on reusing the same fingerprint file to detect if the output needs to be rebuilt. The previous change that put each unit's fingerprint into a separate directory was wrong, and broke change detection in this case (for example, executables on MSVC). Instead, use a different approach to deal with compiler output caching by using the same naming convention as the fingerprint file itself.
I pushed an update that reverted the "always compute Metadata" change. It doesn't work for targets like MSVC in the following scenario:
In the case where there is no metadata hash in the filename, the units must reuse the same fingerprint file. I changed the fix for the compiler output cache by changing the |
@bors: r+ |
📌 Commit 7438770 has been approved by |
☀️ Test successful - checks-azure |
Fix fingerprinting for lld on Windows with dylib. This fixes an issue where if `lld` is used on Windows, dynamic libraries will never be treated as "fresh". This is a regression from #8210 where Cargo is expecting export files to be created, but lld does not create these. The solution is to ignore "Auxiliary" files in fingerprinting, which AFAIK aren't really needed (only the primary output files really matter). Fixes #8284
Fix fingerprinting for lld on Windows with dylib. This fixes an issue where if `lld` is used on Windows, dynamic libraries will never be treated as "fresh". This is a regression from rust-lang#8210 where Cargo is expecting export files to be created, but lld does not create these. The solution is to ignore "Auxiliary" files in fingerprinting, which AFAIK aren't really needed (only the primary output files really matter). Fixes rust-lang#8284
Fix overzealous `clean -p` for reserved names. #8210 changed the way `clean -p` worked, but in some ways it is a little too sloppy. If a package has a test named `build`, then it would delete the `build` directory thinking an executable named "build" exists. This changes it so that it does not attempt to delete tests/benches from the uplift directory.
This is some cleanup around how Cargo computes the outputs from rustc, and changes how
cargo clean -p
works. This also includes some minor observable differences detailed below.clean -p changes
Previously
cargo clean -p
would build the unit graph and attempt to guess what all the filename hashes are. This has several drawbacks. It incorrectly guesses the hashes in many cases (such as different features). It also tends to cause bugs because it passes every permutation of every setting into Cargo's internals, which may not be prepared to accept unsupported combinations like "test a build-script".This new implementation uses a strategy of querying rustc for the filename prefix/suffix, and then deletes the files using globs.
cargo clean -p
should now be more complete in deleting a package's artifacts, for example:Internal changes
There are a bunch of internal changes to make Cargo do a better job of tracking the outputs from rustc, and to make the code easier to understand:
TargetInfo
. The files which are uplifted are solely computed inCompilationFiles::uplift_to
. Previously both responsibilities were awkwardly spread in both locations.FileType::should_replace_hyphens
).CrateType
to replaceLibKind
, to avoid usage of strings, and to use the same structure for all of the target kinds.FileFlavor::Rmeta
to be more explicit as to which output files are ".rmeta". (Previously theLinkable{rmeta}
flag was specific for pipelining, and rmeta wasfalse
for things likecargo check
, which was a bit hard to deal with.)rustc
. This was mostly unused, because it did not consider hashes in the filename, so it only applied to binaries without hashes, which is essentially just wasm32 and msvc. This hyphen/underscore translation still happens during "uplift".Metadata
is always computed for every unit. The logic of whether or not something should use it is moved toshould_use_metadata
. I didn't realize that multiple units were sharing the same fingerprint directory (when they don't have a hash), which caused some bugs (like bad output caching).Behavioral changes
Cargo now does more complete tracking of the files generated by rustc (and the linker). This means that some files will now be uplifted that previously weren't. It also means they will show up in the artifact JSON notifications. The newly tracked files are:
.exp
export files for Windows MSVC dynamic libraries. I don't know if these are actually useful (nobody has asked for them AFAIK). Presumably the linker creates them for some reason. Note that lld doesn't generate these files, this is only link.exe..map
files for wasm32-unknown-emscripten.Some other subtle changes:
examples/foo_bar.exe
andexamples/foo-bar.exe
. Previously Cargo would just rename it to contain the hyphen. This is a consequence of simplifying the code, and I was reluctant to add a special case for this very narrow situation.Notes
clean -p
may change again in the future. If and when Cargo implements some kind of on-disk database that tracks artifacts (for the purpose of garbage collection), thencargo clean -p
can be rewritten to use that mechanism if appropriate.build_script_build
incremental directory isn't deleted because Cargo doesn't know which one belongs to which package. I'm uncertain if that's reasonably fixable. The only option I've thought of is to place each package's incremental output in a separate directory.globset
to handle non-utf-8 filenames? I suspect that Cargo's support for non-utf-8 names is pretty poor, so I'm uncertain how important that is.Closes #8149
Closes #6937
Closes #5788
Closes #5375
Closes #3530