Skip to content

Manually cherry-pick fix for normalize_path()#2751

Draft
SomeTiramisu wants to merge 5 commits into
mozilla:mainfrom
SomeTiramisu:main
Draft

Manually cherry-pick fix for normalize_path()#2751
SomeTiramisu wants to merge 5 commits into
mozilla:mainfrom
SomeTiramisu:main

Conversation

@SomeTiramisu

@SomeTiramisu SomeTiramisu commented Jun 24, 2026

Copy link
Copy Markdown

As discussed in #2510 (comment)
normalize_path() was copied from Cargo before commit
rust-lang/cargo@7a6eaf9
which had an issue with ..s in processor output path (at least). This prevent
meson, or any exotic build environment to cache preprocessed objects.

This PR manually cherry pick the fix, but lack any associated test (yet)

@SomeTiramisu SomeTiramisu changed the title As discussed in https://github.com/mozilla/sccache/issues/2510 Manually cherry-pick fix for normalize_path() Jun 24, 2026
@SomeTiramisu SomeTiramisu marked this pull request as draft June 24, 2026 13:11
@AJIOB

AJIOB commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Hi @SomeTiramisu ,

Please, add the test to exclude the possible regression for that.

@codecov-commenter

codecov-commenter commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.63%. Comparing base (b799af2) to head (038cee4).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
src/compiler/c.rs 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2751      +/-   ##
==========================================
- Coverage   74.63%   74.63%   -0.01%     
==========================================
  Files          70       70              
  Lines       39898    39902       +4     
==========================================
+ Hits        29778    29779       +1     
- Misses      10120    10123       +3     

☔ View full report in Codecov by Harness.
📢 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.

@SomeTiramisu

SomeTiramisu commented Jun 27, 2026

Copy link
Copy Markdown
Author

Please, add the test to exclude the possible regression for that.

I'm new with the rust toolchain, so far I haven't found what obscure test triggered the Component::ParentDir => path. Since this does not fail with the patch, i assume there is no regression there. The worst I can do is return an even more invalid path.

What I can do is testing the improved accuracy of normalize_path() in case of ..; adding test cases in test_process_preprocessor_line_fs_access() that would fail before the patch.

Curiously, msvc.rs have it's own way of normalizing, clang.rs omit it completely

@SomeTiramisu

Copy link
Copy Markdown
Author

It's whether I'm on the right track or completely lost. I fear going too deep for what looks like an edge case optimisation (allow preprocessor cache when include path contain a ..).

No more warning is a thing, but does this really improve performance? I still have to benchmark this (I will use mpc like in #2510 (comment))

`normalize_path()` is a no-op when path start with `..`, canonicalize.
This should help to resolve `../whatever.h` include to an absolute path.

is this really what we want here?
@SomeTiramisu SomeTiramisu force-pushed the main branch 3 times, most recently from 21ea439 to 854292f Compare June 28, 2026 00:11
@SomeTiramisu

Copy link
Copy Markdown
Author

This now looks like a two in one PR. On one hand normalize_path() is now more correct with new code path tested. On the other hand process_preprocessor_line() now normalize on absolute path. The latter fix the original issue.

Benchmark on over optimistic scenario (full clean rebuild, same code) with sccache, on a meson project (GIMP in my case) shows improvement on build time (because preprocessor cache is now working) at the cost of a bit slower caching.

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.

3 participants