Skip to content
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

Closed
wants to merge 5 commits into from
Closed

Adding SCCACHE_BASEDIR with path_helper module #104

wants to merge 5 commits into from

Conversation

gumpt
Copy link

@gumpt gumpt commented Apr 11, 2017

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. :-)

gumpt added 2 commits April 10, 2017 22:09
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.
@luser
Copy link
Contributor

luser commented Apr 11, 2017

Hey, thanks for the patch! I will try to find time to give it a once-over in the next couple of days.

@russellmcc
Copy link
Contributor

This PR didn't work for me due to what seems like a bug in get_common_prefix_path (still looking into exactly what the issue is).

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.

@gumpt
Copy link
Author

gumpt commented Jun 6, 2017

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.

Sorry for the really messy commit history -- I intended to squash the last four (all of the catching up from the past couple of months, a bug fix, plumbing cwd around), but I am out of my depth managing the intermittent history and the new changes.

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.

gumpt and others added 2 commits June 5, 2017 21:28
@posix4e
Copy link
Contributor

posix4e commented Jul 19, 2017

Neat

@glandium
Copy link
Collaborator

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

@gumpt
Copy link
Author

gumpt commented Jul 28, 2017

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

@gumpt
Copy link
Author

gumpt commented Sep 21, 2017

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.

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.

5 participants