Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

ahartmetz · 2025-05-06T12:54:01Z

No description provided.

codecov-commenter · 2025-05-06T12:58:20Z

Codecov Report

Attention: Patch coverage is 86.95652% with 33 lines in your changes missing coverage. Please review.

Project coverage is 70.94%. Comparing base (5b1e93e) to head (f9f181c).

Files with missing lines	Patch %	Lines
src/compiler/gcc.rs	37.50%	20 Missing ⚠️
src/compiler/nvcc.rs	0.00%	4 Missing ⚠️
src/compiler/nvhpc.rs	0.00%	4 Missing ⚠️
src/compiler/rust.rs	94.82%	3 Missing ⚠️
src/compiler/c.rs	98.24%	1 Missing ⚠️
tests/dist.rs	98.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2392      +/-   ##
==========================================
- Coverage   71.58%   70.94%   -0.64%     
==========================================
  Files          65       65              
  Lines       36214    35523     -691     
==========================================
- Hits        25923    25203     -720     
- Misses      10291    10320      +29

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sylvestre · 2025-05-13T07:27:23Z

nice
a few jobs are failing btw

ahartmetz · 2025-05-13T14:13:30Z

This seems really annoying to auto-test, and to be honest, I haven't even tested it manually yet ;)
I will obviously need to do at least that.

ahartmetz · 2025-05-18T20:08:00Z

So I did do some practical testing and ran into another blocker for preprocessor cache mode, this time that it didn't work with -MD (which is used by at least Make and Ninja backends of CMake, so very widely I'd say).
@glandium, do the last three commits look reasonable to you?

It is curious that
#2194
is older than
#2322
so it seems like there is another, yet unfixed, bug that breaks caching and restoring .d files in preprocessor cache mode?

ahartmetz · 2025-06-16T12:50:38Z

Well, that cherry-pick didn't fix the tests.

Copilot

Pull Request Overview

This PR refactors compiler hasher APIs to support mutable state, enhances preprocessor cache behavior to preserve dependency files (.d), and updates tests and language-specific compilers to emit and handle dependency artifacts.

Change generate_hash_key and get_cached_or_compile to take &mut self and propagate mutability to callers
Add logic to filter out or restore the .d dependency file based on is_locally_preprocessed
Extend NVHPC, NVCC, and GCC compiler backends and tests to include .d artifacts

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/server.rs	Marked `hasher` argument as `mut` to match new signature
src/compiler/rust.rs	Refactored `generate_hash_key` to use `&mut self` and replaced destructuring with `self` references
src/compiler/nvhpc.rs	Added `.d` dependency descriptor in test outputs
src/compiler/nvcc.rs	Added `.d` dependency descriptor in test outputs
src/compiler/gcc.rs	Tracked `dep_path`, inserted `"d"` artifact, updated tests
src/compiler/compiler.rs	Switched to `&mut self`, filtered `.d` in cached outputs, added `is_locally_preprocessed`
src/compiler/c.rs	Added `is_locally_preprocessed` field, updated `generate_hash_key`, introduced poison constant

Comments suppressed due to low confidence (3)

src/compiler/compiler.rs:515

Fix spelling: change "informaiton" to "information."

                    outputs

src/compiler/gcc.rs:293

[nitpick] The variable dep_path is somewhat ambiguous; consider renaming it to dep_file_path or dependency_file_path for clarity.

    let mut dep_path = None;

src/compiler/compiler.rs:3386

Reusing the same hasher instance across multiple get_cached_or_compile calls may carry over mutable state between tests. Consider cloning the hasher for each invocation to ensure isolation.

                Some((

src/compiler/rust.rs

sylvestre · 2025-06-22T08:19:58Z

nice, green builds :)

Could you please add tests ?
Especially tests that would fail without your changes and would pass now
thanks

ahartmetz · 2025-06-24T15:23:54Z

nice, green builds :)

Could you please add tests ? Especially tests that would fail without your changes and would pass now thanks

For dependency files (-MD etc) with preprocessor cache mode, I have it easy: when preprocessor cache mode was disabled in case of -MD etc detected: c0a0b6e, there were tests added which I kept, and they failed in builds without the "dist" feature, which was required to make the new logic work before 60b826b.
(Edit: nope, that's wrong - different failure mode: these tests were for non-preprocessor cache mode.)

For distributed compilation with preprocessor cache mode, I will actually need to write the tests.

ahartmetz · 2025-07-07T16:41:55Z

For dependency files (-MD etc) with preprocessor cache mode, I have it easy: when preprocessor cache mode was disabled in case of -MD etc detected: c0a0b6e, there were tests added which I kept, and they failed in builds without the "dist" feature, which was required to make the new logic work before 60b826b. (Edit: nope, that's wrong - different failure mode: these tests were for non-preprocessor cache mode.)

Ugh. I always have to reload a lot of context when getting back to this. In fact, there are existing tests covering that part, quoting from my updated commit message:
This is covered by the existing test test_sccache_command(preprocessor_cache_mode = true) -> run_sccache_command_tests() -> test_gcc_clang_depfile().

For distributed compilation with preprocessor cache mode, I will actually need to write the tests.

And I did!

It will need to call itself (in the next commit), and the "moves values out of the instance and consumes them" model doesn't work for that.

Under the following conditions: - Build with preprocessor cache mode enabled - Distributed build - Preprocessor cache hit - Main cache miss the compile that was run as "fallback" after the cache miss did not have preprocessed sources passed to it since skipping the preprocessing step is the whole point of preprocessor cache mode. But local preprocessing is what enables distributed compilation. To fix this, when the problem occurs, recursively re-invoke get_cached_or_compile() with ForceRecache so that the preprocessor cache hit is (mostly) ignored, which makes get_cached_or_compile() invoke the local preprocessing codepath, which yields code suitable for distributed compilation.

This reverts the "main" code changes of commit c0a0b6e but keeps the added test. That commit fixed getting wrong dep files from cache in non preprocessor cache mode, but made it impossible to support dep files in preprocessor cache mode. We can do better.

It works as follows: - Always store the dep file when committing entries to cache. Pprobably not actually necessary in non-pp cache mode because cache entries between the modes are "incompatible", or are they - but dep file are relatively tiny anyway and this is simpler. - Cache hit in non-preprocessor cache mode: This is based on preprocessed file contents, so it can confuse cache entries with same preprocessed contents but different dependency file names. Do not restore the dep file. It could be for another source file, and restoring it is not necessary because local preprocessing (to get the preprocessed source file contents!) will produce the dep file. - Cache hit in preprocessor cache mode: This cache key is made up of input file names and hashes and preprocessing is skipped. In this case, restoring the dep file from cache is both safe and necessary, so do it. CMake's dependency scanning in C[++] at least with GCC and Clang is based on dep files, so this enables preprocessor cache mode in that pretty common build setup. This is covered by the existing test test_sccache_command( preprocessor_cache_mode = true) -> run_sccache_command_tests() -> test_gcc_clang_depfile().

ahartmetz · 2025-07-11T10:36:05Z

The ordering of commits follows a red-green pattern now: first break the tests by removing workarounds, then fix them again by fixing the bugs "properly".

ahartmetz marked this pull request as draft May 6, 2025 12:54

ahartmetz force-pushed the fix_direct_mode_distributed branch from 100c577 to 422cf17 Compare May 13, 2025 14:12

ahartmetz force-pushed the fix_direct_mode_distributed branch from 422cf17 to d97d906 Compare May 18, 2025 19:53

ahartmetz force-pushed the fix_direct_mode_distributed branch 3 times, most recently from 6004164 to c30cfb7 Compare May 24, 2025 21:55

ahartmetz marked this pull request as ready for review May 31, 2025 13:21

ahartmetz changed the title ~~Fix the combination preprocessor cache mode + distributed compilation~~ Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) May 31, 2025

ahartmetz force-pushed the fix_direct_mode_distributed branch from c30cfb7 to 3576a2b Compare June 1, 2025 19:10

ahartmetz force-pushed the fix_direct_mode_distributed branch 2 times, most recently from ded3235 to 5296730 Compare June 12, 2025 00:40

ahartmetz force-pushed the fix_direct_mode_distributed branch from b44b158 to 5296730 Compare June 16, 2025 12:51

sylvestre requested a review from Copilot June 20, 2025 16:05

Copilot AI reviewed Jun 20, 2025

View reviewed changes

src/compiler/rust.rs Show resolved Hide resolved

ahartmetz force-pushed the fix_direct_mode_distributed branch from d1a8e71 to 73f9766 Compare June 21, 2025 13:46

sylvestre force-pushed the fix_direct_mode_distributed branch from 73f9766 to bf29f51 Compare June 23, 2025 07:07

ahartmetz force-pushed the fix_direct_mode_distributed branch 5 times, most recently from 58b98bd to 16e05e8 Compare July 7, 2025 16:12

ahartmetz force-pushed the fix_direct_mode_distributed branch from 16e05e8 to e064474 Compare July 7, 2025 20:32

ahartmetz force-pushed the fix_direct_mode_distributed branch 2 times, most recently from 6784fca to d7daa50 Compare July 8, 2025 15:35

ahartmetz requested a review from sylvestre July 8, 2025 15:44

ahartmetz force-pushed the fix_direct_mode_distributed branch 4 times, most recently from c2a88a3 to 4897573 Compare July 9, 2025 10:08

ahartmetz added 9 commits July 11, 2025 12:34

Add test for bug mozilla#2173

8b651e3

Remove workaround for mozilla#2173

208823b

Remove a few IMO pointless indirections / aliases

3148b4d

Change self of generate_hash_key() from Box<Self> to &mut Self

76811a3

It will need to call itself (in the next commit), and the "moves values out of the instance and consumes them" model doesn't work for that.

Do not disable preprocessor cache mode if there is a dep file

fc3f474

Remove documentation for removed limitations of preprocessor cache mode

f9f181c

ahartmetz force-pushed the fix_direct_mode_distributed branch from 4897573 to f9f181c Compare July 11, 2025 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Uh oh!

ahartmetz commented May 6, 2025

Uh oh!

codecov-commenter commented May 6, 2025 •

edited

Loading

Uh oh!

sylvestre commented May 13, 2025

Uh oh!

ahartmetz commented May 13, 2025

Uh oh!

ahartmetz commented May 18, 2025 •

edited

Loading

Uh oh!

ahartmetz commented Jun 16, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

sylvestre commented Jun 22, 2025

Uh oh!

ahartmetz commented Jun 24, 2025 •

edited

Loading

Uh oh!

ahartmetz commented Jul 7, 2025 •

edited

Loading

Uh oh!

ahartmetz commented Jul 11, 2025

Uh oh!

Uh oh!

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Are you sure you want to change the base?

Fix preprocessor cache mode + (distributed compilation || -MD dependency scanning) #2392

Uh oh!

Conversation

ahartmetz commented May 6, 2025

Uh oh!

codecov-commenter commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sylvestre commented May 13, 2025

Uh oh!

ahartmetz commented May 13, 2025

Uh oh!

ahartmetz commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahartmetz commented Jun 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

sylvestre commented Jun 22, 2025

Uh oh!

ahartmetz commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahartmetz commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahartmetz commented Jul 11, 2025

Uh oh!

Uh oh!

codecov-commenter commented May 6, 2025 •

edited

Loading

ahartmetz commented May 18, 2025 •

edited

Loading

ahartmetz commented Jun 24, 2025 •

edited

Loading

ahartmetz commented Jul 7, 2025 •

edited

Loading