Skip to content

Conversation

@AJIOB
Copy link
Contributor

@AJIOB AJIOB commented Jan 4, 2026

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2026

Codecov Report

❌ Patch coverage is 84.25926% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.14%. Comparing base (24624cd) to head (ff781bd).

Files with missing lines Patch % Lines
src/compiler/c.rs 83.70% 22 Missing ⚠️
src/compiler/clang.rs 0.00% 4 Missing ⚠️
src/compiler/compiler.rs 94.52% 4 Missing ⚠️
src/compiler/gcc.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2538      +/-   ##
==========================================
+ Coverage   71.10%   71.14%   +0.04%     
==========================================
  Files          64       64              
  Lines       35487    35584      +97     
==========================================
+ Hits        25232    25316      +84     
- Misses      10255    10268      +13     

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

@AJIOB AJIOB force-pushed the support-preprocessed branch from 9f561eb to 8ed5ba5 Compare January 4, 2026 12:27
@AJIOB AJIOB requested a review from sylvestre January 5, 2026 08:48
Copy link
Collaborator

@ahartmetz ahartmetz left a comment

Choose a reason for hiding this comment

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

The commit message should either say "preprocessor output" or "preprocessed input". Preprocessor output is sccache input.

@ahartmetz
Copy link
Collaborator

By the way, what's the use case for this? Would be nice to mention it somewhere. When I see this, I think "Huh, I guess this could make sense, but I've compiled a ton of software and never had a need for it".
AFAICS, not having this feature would at worst cause a minor waste of time and disk space in the rare case that you are compiling already preprocessed code.

@AJIOB
Copy link
Contributor Author

AJIOB commented Jan 5, 2026

By the way, what's the use case for this? Would be nice to mention it somewhere. When I see this, I think "Huh, I guess this could make sense, but I've compiled a ton of software and never had a need for it". AFAICS, not having this feature would at worst cause a minor waste of time and disk space in the rare case that you are compiling already preprocessed code.

I have 2 reasons to do it:

  1. Support asm files (that clang/gcc do natively, see Add support for asm sources support #2537) - some configure scripts uses the same binaries for C and asm. And some asm files (.s) MUSTN'T be preprocessed
  2. Fix TODOs

The C preprocessed files can be easier verified, that the arch-dependent asm files.

Are these reasons enough?

@ahartmetz
Copy link
Collaborator

ahartmetz commented Jan 5, 2026

Are these reasons enough?

Yes, thanks. I see the TODOs now. Would it be possible to add .s (and maybe .S, I know that one does need preprocessing) support in the same branch then, or is that a larger change that you don't want to make yet?

@AJIOB
Copy link
Contributor Author

AJIOB commented Jan 5, 2026

I want to do it as a dedicated PR: need to test it additionally

@AJIOB AJIOB force-pushed the support-preprocessed branch from c37dde7 to 9438cca Compare January 5, 2026 18:36
@AJIOB AJIOB requested a review from ahartmetz January 5, 2026 18:36
@AJIOB AJIOB changed the title Add support for preprocessed output Add support for preprocessor output Jan 5, 2026
@AJIOB AJIOB force-pushed the support-preprocessed branch from 9438cca to 0292994 Compare January 5, 2026 18:37
@AJIOB AJIOB changed the title Add support for preprocessor output Add support for C preprocessor output Jan 5, 2026
@AJIOB AJIOB force-pushed the support-preprocessed branch from 0292994 to ff781bd Compare January 6, 2026 06:04
@AJIOB AJIOB requested a review from ahartmetz January 6, 2026 06:10
preprocessor_result.stdout.len()
);
(preprocessor_result.stdout, include_files)
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could move this to the beginning (followed by else if) and prevent a lot of "chaff" whitespace changes and a level of indentation. Yes it's not the common case, but OTOH, having a trivial implementation first is helpful to see what the full version is essentially doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you suggest adding next parts duplication? Or just swap if & else parts between each other?

P.S. You can see the difference without spaces on the github UI

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I could only understand what's going on well enough to suggest this after I found the option. (And git diff and git blame have it, too, TIL)
So anyway...

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I meant was:

let (preprocessor_output, include_files) = if !needs_preprocessing {
    //...
} else if let Some(preprocessor_key) = &preprocessor_key {
// ... everything else as before

Not sure if that syntax even works

@ahartmetz
Copy link
Collaborator

This looks OK to me, but I feel too noob at Rust (or sccache for that matter) to approve it - at least for now. I'll change my mind if nobody else steps up for a several weeks.

@sylvestre sylvestre merged commit 5f09103 into mozilla:main Jan 7, 2026
54 checks passed
@sylvestre
Copy link
Collaborator

lgtm
thanks!

@AJIOB AJIOB deleted the support-preprocessed branch January 7, 2026 12:49
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