-
Notifications
You must be signed in to change notification settings - Fork 623
Add SCCACHE_BASEDIRS support
#2521
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2521 +/- ##
==========================================
+ Coverage 71.17% 72.31% +1.13%
==========================================
Files 64 64
Lines 35592 36995 +1403
==========================================
+ Hits 25334 26752 +1418
+ Misses 10258 10243 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bfba6ec to
9eb3241
Compare
e818064 to
d2e6edd
Compare
|
I got an idea, that basedirs should be added to the stats command as well |
ca0a0db to
6f47f36
Compare
b5a7d22 to
cf0b871
Compare
I think we can also provide the stat about base dir usage translation:
In this case we can see, do we need to provide more/better base dirs or not |
It's a tricky one. After taking a look, the number of successful substitutions is relatively easy to implement, although the counter should be threaded to the ServerStats. But how to count the number of skipped directories? What is it? If a |
No, just to check what number of cache requests has been converted using any of base dirs and what number was kept as an absolute one |
|
About paths: we can try to normalize paths via https://docs.rs/normpath/latest/normpath/ or something similar instead of doing one more implementation. |
It looks like https://doc.rust-lang.org/stable/std/path/fn.absolute.html is what I should use for both cases. It normalizes slashes, but keeps the cases as is. Although see my comment regarding the |
a97218c to
bccc8f4
Compare
I am afraid it causes changes in too many places, including the signatures of |
0887cea to
e42a1f6
Compare
src/util.rs
Outdated
| for basedir_path in basedirs.iter() { | ||
| let basedir_str = basedir_path.to_string_lossy(); | ||
| let basedir = basedir_str | ||
| .trim_end_matches(|c| c == '/' || c == '\\') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\ should not be trimmed on non-Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, stripping \|/ on windows, / on other OSes, and then adding the trailing /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll use https://docs.rs/typed-path/latest/typed_path/ here.
Then, the trailing / will be added explicitly and completely stripped from the preprocessor_output instead of being replaced by ..
The last question is whether ./ should be treated as a special case. Because ./test.c and test.c won't match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 87151ba, I implemented this logic. The basedir is stripped completely to match /dir/test.c and test.c in the cache.
The ./test.c is still not covered by the code.
6529696 to
fae67f8
Compare
Co-authored-by: whisperity <[email protected]>
Co-authored-by: Alex Overchenko <[email protected]>
Co-authored-by: Ben Boeckel <[email protected]>
1d0156e to
31f9f6b
Compare
|
Rebased it on top of the current master |
mathstuf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just one nit and a possible followup.
Thanks!
| /// If `basedirs` are provided, paths in the preprocessor output will be normalized by | ||
| /// stripping the longest matching basedir prefix. This enables cache hits across different | ||
| /// absolute paths (similar to ccache's CCACHE_BASEDIR). | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something for a new PR, but a Builder pattern to deal with these arguments looks like a nice enhancement. The (test) call sites being a zoo of &[] and unlabeled bool literals isn't that nice IMO.
| * Multiple developers working with different username paths | ||
| * Working with multiple project checkouts simultaneously | ||
|
|
||
| **Note:** Only absolute paths are supported. Relative paths will prevent server from start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| **Note:** Only absolute paths are supported. Relative paths will prevent server from start. | |
| **Note:** Only absolute paths are supported. Relative paths will prevent server from starting. |
This is an attempt to fix #35.
The ENV variable
SCCACHE_BASEDIRSand configuration parameterbasedirsare added.As well as new tests to validate the behavior.