-
Notifications
You must be signed in to change notification settings - Fork 630
Avoid double-caching when ccache is installed in PATH #2524
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 #2524 +/- ##
==========================================
+ Coverage 72.04% 72.12% +0.07%
==========================================
Files 66 66
Lines 36953 37085 +132
==========================================
+ Hits 26623 26746 +123
- Misses 10330 10339 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ec91704 to
608d5c6
Compare
|
update: fix rustfmt error |
|
@sylvestre please review |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
1fe7bb9 to
4bb5579
Compare
|
@sylvestre can I help in any way to push this along? |
src/util.rs
Outdated
| use std::env; | ||
| use std::path::Path; | ||
|
|
||
| const CCACHE_DIRS: &[&str] = &["/usr/lib/ccache", "/usr/lib64/ccache"]; |
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.
The CCACHE_DIRS list only contains Linux paths. Consider adding macOS Homebrew paths (/usr/local/opt/ccache/libexec, /opt/homebrew/opt/ccache/libexec) or adding a comment explaining why they're omitted
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.
also, it would be nice to be able to configure things somewhere
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.
We can generalize it to any path that contains a component ccache.
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.
About configuration, is it still needed if we just filter out directories with ccache components?
src/util.rs
Outdated
| /// This prevents double-caching when ccache is installed and has | ||
| /// /usr/lib/ccache or /usr/lib64/ccache in PATH, since those directories | ||
| /// contain wrapper scripts that would call ccache. | ||
| pub fn filter_ccache_from_path(env_vars: Vec<(OsString, OsString)>) -> Vec<(OsString, OsString)> { |
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.
Function allocates a new Vec even when no ccache dirs are present in PATH. Consider short-circuiting to return the input Vec unchanged when filtering is not needed
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.
Sure
src/compiler/compiler.rs
Outdated
| } = self; | ||
| // Filter out ccache directories from PATH to avoid double-caching | ||
| // when ccache is also installed on the system. | ||
| let env_vars = filter_ccache_from_path(env_vars.to_vec()); |
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.
Filter is only applied to SingleCompileCommand but NvccCompileCommand (in nvcc.rs) also implements CompileCommandImpl and may need the same treatment it is intentional ?
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.
Not intentional, I'll take a look.
|
sorry for the latency |
4bb5579 to
2a7f25c
Compare
|
v2:
|
2a7f25c to
ecce16b
Compare
|
v3:
|
ecce16b to
ce9f95b
Compare
|
v4:
|
|
I believe the performance regression report is spurious, the code doesn't touch the macro finder code. |
|
So I'm not the maintainer here and this is merely a suggestion. I'm a bit uncomfortable with the brittleness of this approach to recognizing ccache. An additional, somewhat - perhaps less - brittle heuristic that could be added on POSIX platforms is to recognize when gcc, g++, clang, clang++ [...] is a symlink to a binary called "ccache". Assuming that ccache works like icecream and sccache (which btw supports symlinks compiler -> sccache now). |
I believe it's unnecessary - it's unlikely people will have directories in path named ccache that host the real compilers instead of ccache masquerades. I'm happy to make that change if the maintainers think it is necessary though. |
I don't mean ANDing the two conditions, I mean ORing. The false positive rate of a compiler binary linking to a binary called ccache, which is actually not ccache, should be negligible. |
To clarify: IF the directory contains a path component ccache, OR if the looked-up compiler is a symlink to something named ccache, THEN skip this compiler executable? Makes sense. |
That's what I meant, yes |
|
(Background: I MAY have had a distributed compiler, which I compiled myself, installed in [obscure]/local/bin at some point, but I have never renamed the distributed compiler binary to something else than its usual name - which would probably break something because it avoids calling itself by checking its binary name) On third thought, what the hell was I thinking, if it's not called gcc or g++ etc, it won't be found as a compiler at all, so you'd need to resolve the symlink to ccache, which is... doable but where this maybe gets a bit "too much". |
d8913d5 to
6681201
Compare
|
update:
|
On Linux, ccache is typically installed in $PATH as /usr/lib64/ccache/g++ or similar. If we keep it there, all compilations will be cached by both ccache and sccache. While the user could easily disable ccache with CCACHE_DISABLE, it's reasonable to assume many will forget and will have their disk space doubly consumed by both caches. Better to recognize this and disable ccache under sccache. This patch does this resolving the input compiler name to an absolute path, with these rules: - if a directory in PATH contains a component `ccache`, it is skipped - if the compiler exists in a directory in PATH, but is a symlink to a target `ccache`, it is skipped These two tests will catch most ccache masquerade directories. Fixes mozilla#2519
6681201 to
83fc260
Compare
|
update:
|
On Linux, ccache is typically installed in $PATH as /usr/lib64/ccache/g++
or similar. If we keep it there, all compilations will be cached by both
ccache and sccache.
While the user could easily disable ccache with CCACHE_DISABLE, it's
reasonable to assume many will forget and will have their disk space
doubly consumed by both caches. Better to recognize this and disable
ccache under sccache.
This patch does this resolving the input compiler name to an absolute
path, with these rules:
ccache, it is skippedto a target
ccache, it is skippedThese two tests will catch most ccache masquerade directories.
Fixes #2519