Repository navigation
rust: hash proc-macro externs by their crate metadata - #2882
xdustinface wants to merge 4 commits into
Conversation
| /// metadata section rather than as a whole file. On macOS the linker writes the | ||
| /// library's own output path into it, so the same proc-macro built into two target | ||
| /// directories would hash differently and every crate using it would miss the cache. | ||
| /// The metadata section does not depend on the output path. Everything else, and a |
There was a problem hiding this comment.
could you please make this comment shorter? 2 lines should be enough :)
| return Ok(digest); | ||
| } | ||
| } | ||
| Digest::file(path, pool).await |
There was a problem hiding this comment.
when there is no section, we read the file twice.
could we hash data directly in the closure instead?
There was a problem hiding this comment.
Good catch. It now hashes the bytes it already read when there's no .rustc section, so each dylib is read once.
|
|
||
| fn rustc_metadata_section(data: &[u8]) -> Option<&[u8]> { | ||
| let file = object::File::parse(data).ok()?; | ||
| let section = file.section_by_name(".rustc")?; |
There was a problem hiding this comment.
what about a proc-macro linking native code (a build.rs with cc)?
if the C code changes, .rustc stays the same and we would get a stale hit, no?
There was a problem hiding this comment.
You're right, the metadata doesn't cover native code linked into the macro. In practice that means a local proc-macro crate with a cc-built helper edited in place, since a published version never changes. I've added a line to docs/Rust.md next to the existing caveat about proc-macros that read files.
There is an exact alternative. When I build serde_derive as a dependency in two target directories, the dylibs differ only by the install name the linker takes from the output path. With -C link-arg=-Wl,-install_name,@rpath/<file> they come out byte-identical, so sccache could add that when it runs a proc-macro compile on macOS and keep hashing externs whole. The catch is that sccache would then change a compile it doesn't cache, and -Wl, assumes cc drives the linker. If you'd prefer that route anyway, I'm happy to rework the PR around it.
| use std::path::Path; | ||
|
|
||
| #[test] | ||
| fn test_hash_all_externs_hashes_whole_files_without_rustc_metadata() { |
There was a problem hiding this comment.
this only tests the fallback path.
please add a test where the .rustc section is used, e.g. two objects written with object::write that differ outside that section and hash the same
There was a problem hiding this comment.
Added test_hash_all_externs_hashes_a_dylib_by_its_metadata: two dylibs with the same .rustc section and different code hash the same, and a changed section hashes differently. It uses object's write feature as a dev-dependency.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2882 +/- ##
==========================================
- Coverage 77.58% 74.63% -2.96%
==========================================
Files 72 72
Lines 41409 41519 +110
==========================================
- Hits 32126 30986 -1140
- Misses 9283 10533 +1250 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
03d842f to
9c891e5
Compare
The proc-macro dylib a crate links against was hashed as a whole file. On macOS the linker writes the dylib's own output path into it, so the same proc-macro built into two target directories hashed differently and every crate using it missed the cache. Hash a dylib extern by its `.rustc` metadata section instead, which does not depend on the output path.
- Shorten the doc comment on `hash_all_externs` to two lines. - Read a dylib once: hash its `.rustc` section, or the bytes already read when there is none. - Test the metadata path with two `object::write` objects that differ outside `.rustc` (adds `object`'s `write` feature as a dev-dependency). - Document in docs/Rust.md that native code linked into a proc-macro is not part of its key.
3560c2e to
3f63368
Compare
On macOS the linker writes a dylib's own output path into it. sccache hashes each
--externfile whole, so the same proc-macro built into two target directories (two git worktrees, two CI checkouts) hashes differently, and every crate that uses the macro misses the cache.This hashes a dylib extern by its
.rustcmetadata section instead. That section doesn't depend on the output path:serde_derivebuilt in four checkouts had four different file hashes and one metadata hash. It still changes with the macro's source and dependencies. Every other extern, and a dylib without the section, is hashed whole as before.test_rust_cargo_proc_macro_in_another_target_dirbuilds a proc-macro and a crate using it into two target directories and expects the second build to hit. It fails without this change on macOS. I've only run the tests on macOS.Related to #2652, but independent of the base-dir work there.