Skip to content

Comments

fix(src/compiler/c): take common_args into account during preprocessor hashing#2039

Merged
Xuanwo merged 1 commit intomozilla:mainfrom
GZGavinZhao:fix-preprocessor-cache
Jan 18, 2024
Merged

fix(src/compiler/c): take common_args into account during preprocessor hashing#2039
Xuanwo merged 1 commit intomozilla:mainfrom
GZGavinZhao:fix-preprocessor-cache

Conversation

@GZGavinZhao
Copy link
Contributor

@GZGavinZhao GZGavinZhao commented Jan 16, 2024

Fixes #2038.

Since common_args represents the flags that are used both for compilation and for preprocessing, during preprocessor caching common_args should be taken into account as well.

@sylvestre
Copy link
Collaborator

Could you please add a test to make sure we don't regress? Thanks

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2024

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (13b018f) 30.71% compared to head (e9e0d1e) 31.00%.

Files Patch % Lines
src/compiler/compiler.rs 37.77% 3 Missing and 25 partials ⚠️
src/compiler/c.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2039      +/-   ##
==========================================
+ Coverage   30.71%   31.00%   +0.28%     
==========================================
  Files          51       51              
  Lines       19604    19653      +49     
  Branches     9422     9452      +30     
==========================================
+ Hits         6022     6094      +72     
+ Misses       7861     7853       -8     
+ Partials     5721     5706      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GZGavinZhao GZGavinZhao force-pushed the fix-preprocessor-cache branch from f9ac3d0 to 189d0cf Compare January 16, 2024 20:36
@GZGavinZhao
Copy link
Contributor Author

@sylvestre Added a test case. Would this be enough?

@GZGavinZhao GZGavinZhao force-pushed the fix-preprocessor-cache branch from 189d0cf to 4d5267b Compare January 16, 2024 21:12
@GZGavinZhao
Copy link
Contributor Author

Addressed lints.

@sylvestre sylvestre force-pushed the fix-preprocessor-cache branch from 4d5267b to e9e0d1e Compare January 17, 2024 07:22
@GZGavinZhao
Copy link
Contributor Author

Would someone mind merging this? IMO this is a pretty serious issue, because sccache may return incorrect build artifact for C/C++ compilations whenever the -D and every other common compiler flags changes.

Copy link
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

let mut preprocessor_and_arch_args = parsed_args.preprocessor_args.clone();
preprocessor_and_arch_args.extend(parsed_args.arch_args.to_vec());
// common_args is used in preprocessing too
preprocessor_and_arch_args.extend(parsed_args.common_args.to_vec());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this line doesn't make the test fail.

tottoto added a commit to tottoto/sccache that referenced this pull request Feb 6, 2026
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.

common_args is not included in preprocessor caching

5 participants