Skip to content

Comments

Use different hashing functions for different file types#197

Closed
metajack wants to merge 1 commit intomozilla:mainfrom
metajack:custom-hashing
Closed

Use different hashing functions for different file types#197
metajack wants to merge 1 commit intomozilla:mainfrom
metajack:custom-hashing

Conversation

@metajack
Copy link
Contributor

This adds a new special hasher for static libraries, which contain timestamps
and other info that prevent them from being cachable in some cases.

@metajack
Copy link
Contributor Author

r? @luser

Copy link

@payload payload left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zeroing looks wrong. Please add tests.

src/util.rs Outdated
},
Ok(()) => {
// timestamp
zero_slice(&mut buffer[16..28], 12);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed the first 16 bytes of ar_name here. You zero ar_name and parts of ar_date. uid and gid are left intact. See man ar(5)

This adds a new special hasher for static libraries, which contain timestamps
and other info that prevent them from being cachable in some cases.
@luser
Copy link
Contributor

luser commented Feb 15, 2019

I did a bit of rework on this patch. It's passing tests now. I'd like to add a real-world cargo test for it alongside the existing cargo tests we have and I also need to verify that it works for windows-msvc, but otherwise I think this is pretty close to landable.

@luser
Copy link
Contributor

luser commented Feb 20, 2019

I tested this on windows-msvc and it works (apparently MSVC .lib files are the same archive format, TIL), but there's a slight complication in that MSVC generates timestamps in object files unless you pass the undocumented /Brepro flag. I opened an issue on the cc crate (which you can see above) to suggest doing so by default. Adding that to my test crate's build script made things work such that I was able to get 100% cache hits on Windows.

@metajack
Copy link
Contributor Author

metajack commented Aug 9, 2019

What's left to land this then?

@luser
Copy link
Contributor

luser commented Aug 12, 2019

I forget what I was hung up on with this. Maybe writing tests? We had disabled the cargo integration tests on Mac because they were flaky:

#[cfg(not(target_os="macos"))] // test currently fails on macos

...and I think I wanted to fix that before landing this, since otherwise we wouldn't have test coverage of the changes on the platform where we actually care about them?

In any event I'm no longer actively working on sccache. If you want to get this landed feel free to take my changes and work with @chmanchester to get it done.

@drahnr
Copy link
Collaborator

drahnr commented Mar 19, 2020

@chmanchester what's required to get this landed :)

@luser
Copy link
Contributor

luser commented May 28, 2020

FYI the cc crate added support for invoking Apple's ar tool with the undocumented flag that makes it zero out timestamps, so with up-to-date dependencies in your crate graph this change is likely no longer necessary:
alexcrichton/cc-rs@555e773

drahnr added a commit to paritytech/cachepot that referenced this pull request Nov 19, 2020
In order to avoid timestamp presence in static lib files,
making them non-deterministic.

alexcrichton/cc-rs@555e773
mozilla/sccache#197
drahnr added a commit to paritytech/cachepot that referenced this pull request Nov 20, 2020
In order to avoid timestamp presence in static lib files,
making them non-deterministic.

alexcrichton/cc-rs@555e773
mozilla/sccache#197
@glandium glandium added this to the 0.2.16 milestone Jan 8, 2021
@mitchhentges
Copy link
Contributor

Hey Jack, this patch has withstanded the weather out here for quite some time, woah 😮
Would you mind rebasing this off master, then we'll get this reviewed and landed?

@mitchhentges
Copy link
Contributor

Going to mark this as help wanted: if a contributor wants to dupe and update this PR, while keeping partial attribution to the original author, that'd be fantastic :)

@FooBarWidget
Copy link
Contributor

Here is a fork that rebases the PR on top of the latest main. It compiles, but the test_generate_hash_key test does not pass, not yet sure why. https://github.com/FooBarWidget/sccache/tree/feature/ar-hash

@FooBarWidget
Copy link
Contributor

FooBarWidget commented Feb 25, 2022

cargo test now passes. Turns out I was using iterators in a wrong way.

@FooBarWidget
Copy link
Contributor

I opened a separate PR in #1135 for my rebased version.

@mitchhentges
Copy link
Contributor

Thanks metajack, and thanks FooBarWidget for rebasing this 👍 :)

@metajack
Copy link
Contributor Author

metajack commented Mar 2, 2022

Thanks for picking this up! I can't believe this is 4.5 years old now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants