Skip to content
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

fix: Don't try to un-archive ar files for hashing #1773

Closed
wants to merge 5 commits into from

Conversation

Xuanwo
Copy link
Collaborator

@Xuanwo Xuanwo commented May 24, 2023

Fix #1250

Thanks for @alex's repro, this PR fixed this issue by removing the un-archive logic of AR files.

I believe this issue could be reproduced by put any ar files like the following for sccache (which ar can't handle):

ossl/lib/libssl.a: Mach-O universal binary with 2 architectures: [x86_64:\012- current ar archive random library] [\012- arm64:\012- current ar archive random library]

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo changed the title fix: Don't try to read ar files fix: Don't try to un-archive ar files for hashing May 24, 2023
@alex
Copy link
Contributor

alex commented May 24, 2023

Is it a problem to remove the ar handling for timestamps on macOS? If yes, do we need to make this code universal-archive aware? https://github.com/rust-lang/rust/blob/master/compiler/rustc_codegen_ssa/src/back/archive.rs#L176-L183 is some code I wrote for rustc to process them.

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented May 24, 2023

Is it a problem to remove the ar handling for timestamps on macOS?

Based on my current understanding, the worst-case scenario is a cache miss when the timestamps are different, which I consider acceptable.

@alex

This comment was marked as resolved.

@Xuanwo

This comment was marked as resolved.

@alex

This comment was marked as resolved.

Signed-off-by: Xuanwo <github@xuanwo.io>
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.06 🎉

Comparison is base (1c6ec75) 29.37% compared to head (1cdcab1) 29.43%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1773      +/-   ##
==========================================
+ Coverage   29.37%   29.43%   +0.06%     
==========================================
  Files          49       49              
  Lines       17626    17616      -10     
  Branches     8507     8501       -6     
==========================================
+ Hits         5177     5185       +8     
+ Misses       7357     7341      -16     
+ Partials     5092     5090       -2     
Impacted Files Coverage Δ
src/compiler/rust.rs 32.76% <0.00%> (+0.15%) ⬆️
src/lib.rs 5.47% <0.00%> (+0.05%) ⬆️
src/server.rs 32.23% <0.00%> (-0.42%) ⬇️
src/util.rs 22.38% <0.00%> (-1.22%) ⬇️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

let reader = File::open(&path)
.with_context(|| format!("Failed to open file for hashing: {:?}", path))?;
let mut archive = Archive::new(reader);
while let Some(entry) = archive.next_entry() {
let entry = entry?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to keep the logic as is, but fallback on the hash in case ? takes effect, such that the hash of the entire file is used. That way we can preserve behavior as is for anything non-mac

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this could lead to increasingly delicate and intricate behavior. I suggest removing this logic entirely unless someone wishes to provide a correct implementation.

Copy link
Collaborator

@drahnr drahnr May 25, 2023

Choose a reason for hiding this comment

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

What are the consequences of removing it entirely? The timestamps are still present so on mac os we get (according to the comment) no cache hits for ar files at all. How many ar-format files are present in an average compilation run C/C++/rust? With that information we could make a more informed choice what is worth it. We can merge the change in the meantime, to avoid breakage at the potential cost of efficiency, but the topic should be followed up + tracked in an issue. Just deleting it is not solving it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi, I add a TODO notes here for hash_all_archives which explains what happened here. PTAL.

Signed-off-by: Xuanwo <github@xuanwo.io>
sagudev added a commit to sagudev/mozjs that referenced this pull request May 25, 2023
@alex
Copy link
Contributor

alex commented May 26, 2023

I've created #1776, which uses object to handle fat archives.

@sylvestre
Copy link
Collaborator

it needs rebasing, could you please have a look?

@drahnr
Copy link
Collaborator

drahnr commented May 30, 2023

I think it is obsolete with the merge of #1776 iiuc

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented May 30, 2023

Replaced by #1776, closing now. Thanks @alex for the better solution!

@Xuanwo Xuanwo closed this May 30, 2023
@Xuanwo Xuanwo deleted the don't-try-to-read-ar-files branch May 30, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug when Compiling rust-mozjs
5 participants