Skip to content

Reword and correct the preprocessor cache mode documentation #2362

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

Merged
merged 4 commits into from
Jun 20, 2025

Conversation

ahartmetz
Copy link
Contributor

@ahartmetz ahartmetz commented Mar 14, 2025

The correction is that there is a long list of compiler options that disable preprocessor cache mode.

@ahartmetz ahartmetz force-pushed the improve_direct_mode_docs branch from 1fb6e20 to fae1c11 Compare March 14, 2025 15:00
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.06%. Comparing base (7025295) to head (ffc6335).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2362      +/-   ##
==========================================
- Coverage   67.06%   67.06%   -0.01%     
==========================================
  Files          64       64              
  Lines       34718    34718              
==========================================
- Hits        23285    23284       -1     
- Misses      11433    11434       +1     

☔ 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.

@ahartmetz ahartmetz force-pushed the improve_direct_mode_docs branch 2 times, most recently from 0c44acb to ba85488 Compare March 14, 2025 19:23
@ahartmetz ahartmetz marked this pull request as draft March 14, 2025 20:05
@ahartmetz
Copy link
Contributor Author

ahartmetz commented Mar 14, 2025

Draft because: The second commit needs review from someone who understands preprocessor cache mode! I think this explanation is much clearer, but I don't know if it is actually true. The previous text mentioned "caching the preprocessor result", but that doesn't seem to make any sense. If the preprocessor result is the same, compilation can be trivially skipped as well (except if only the compiler ID changed, seems to be a bit of a long shot). So I think what is actually happening is skipping from input files to output files directly if there is a cached result, not skipping anything if there is a cache miss.

docs/Local.md Outdated

In order to cache the preprocessor step, [sccache] needs to remember, among other things, all files included by a given input file. To quote ccache's documentation:
To ensure that the cached outputs for a source file correspond to the un-preprocessed inputs, [sccache] needs to remember, among other things, all files included by that source file (i.e. the complete set of inputs). To quote ccache's documentation:
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 be explicit about the issue with __TIME__ and __DATE__ macros and the config flag that allows one to handle the trade-off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, have a look now, I now mention the full list in two places but not everywhere.

Copy link
Collaborator

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

I think it needs one more pass to achieve its goal, the direction is good!

Thank you!

@drahnr
Copy link
Collaborator

drahnr commented Mar 27, 2025

If you take a look into the code (older ref)

storage.get_preprocessor_cache_entry(preprocessor_key)?
is where we do a lookup in the storage instance based on the preprocessor hashkey which is then used to do a lookup of the preprocessor output. And only then proceeeds to actual compilation Box<dyn Compilation> type utilizing the preprocessor output as input for the C compiler.

@ahartmetz ahartmetz force-pushed the improve_direct_mode_docs branch from f8f6125 to 56dbedb Compare May 6, 2025 12:50
@sylvestre sylvestre force-pushed the improve_direct_mode_docs branch from 56dbedb to 5d0838b Compare May 20, 2025 07:44
@sylvestre sylvestre requested a review from drahnr May 20, 2025 07:44
@drahnr
Copy link
Collaborator

drahnr commented May 22, 2025

@ahartmetz is this ready for a review pass?

ahartmetz added 4 commits May 25, 2025 21:34
The correction is that there is a *long* list of compiler options
that disable preprocessor cache mode.
Most importantly, group similar things together. Also improve some
of the wording and document previously undocumented restrictions.
The list of restrictions is a little ridiculous at this point, but
I'm trying to do something about it.
@ahartmetz ahartmetz force-pushed the improve_direct_mode_docs branch from 5d0838b to ffc6335 Compare May 25, 2025 20:03
@ahartmetz
Copy link
Contributor Author

@ahartmetz is this ready for a review pass?

It is now.

@ahartmetz ahartmetz marked this pull request as ready for review May 26, 2025 08:12
@sylvestre sylvestre merged commit 0a81357 into mozilla:main Jun 20, 2025
53 of 57 checks passed
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.

4 participants