-
Notifications
You must be signed in to change notification settings - Fork 557
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
Adding SCCACHE_BASEDIR with path_helper module #104
Conversation
Each command line option which could take a path attempts to have the path rewritten to a relative path if it points underneath the SCCACHE_BASEDIR option. I am a Rust newb so if you think you're going overboard on criticism, you probably aren't.
Hey, thanks for the patch! I will try to find time to give it a once-over in the next couple of days. |
This PR didn't work for me due to what seems like a bug in This actually rewrites the arguments going to the compiler to be a relative path. It might be more robust to separate the argument-set that is used as a key for the cache and the argument-set that is actually used to compile - if we did this, we wouldn't need to relativize the arguments going to the compiler at all. The path relativization code then wouldn't have to know the CWD of the sccache daemon, removing a lot of the complexity of this PR. |
I'm unsurprised that it didn't work, especially given the patch no longer applied cleanly. I've updated it such that it does, fixed a bug generating the common prefix of paths, and used the cwd that's been passed to the compiler options to avoid the location of the daemon meaning anything.
EDIT: I was able to clean the history up a bit, but it's still messier than anyone would like. Hopefully there's an easy way to reconcile this if it ever makes it into master. |
Merge, bugs, whitespace, tests.
Neat |
This only addresses one part of the problem, which is the path in the command line, but leaves the elephant in the room: the preprocessed output, which is also part of the hash for caching, contains paths too. Furthermore, due to how sccache is used for Firefox builds, it would be better to have several base directories (an arbitrary number of them). |
I've been using this very happily with a toy C++ project on personal time and with a large C++ project for $WORK (along with another patch to also rewrite paths which don't actually exist, please don't ask ...), and I get cache hits on builds starting in different locations: there is nothing about preprocessed output which prevents cache hits. I also grabbed the log of what command the preprocessor ran for the same file in two different build directories, and verified the preprocessor output was identical. How would the preprocessor generate output with full paths if it's called with relative paths? I can only think that you mean there are files which #include something with a full path and you want to rewrite the actual files being compiled? Rewriting paths based on multiple roots didn't seem to part of issue #35, since it was just calling for the ccache feature. Is there not some common root to all build directories on all of the Firefox build machines which could be used as the basedir? Just like with the CCACHE_BASEDIR option, you'd want to make the parent of the build directory be your basedir, not a directory with the branch in its name (as the issue states Firefox has). |
I'm going to close this given the lack of interest in merging it to master. Other people should feel free to contribute a PR which solves the problem. |
This solves #35.
Each command line option which could take a path attempts to have the
path rewritten to a relative path if it points underneath the
SCCACHE_BASEDIR option.
I am a Rust newb so if you think you're going overboard on criticism,
you probably aren't. :-)