-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Attempt to cache git modules #40780
Attempt to cache git modules #40780
Conversation
6f54e0d
to
dba2a07
Compare
876e6e2
to
30272e2
Compare
While muddling through the bits and pieces that broke, I was able to observe the new cache in action. The below logs show step 1 (of 3) as described above (note that step 2 and 3 are effectively constant time so aren't relevant). Cache creation (https://travis-ci.org/rust-lang/rust/jobs/214478030#L308):
Cache reuse (https://travis-ci.org/rust-lang/rust/jobs/214478030#L308):
I've finished messing with this PR now, I think the next build will be successful on travis. |
Thanks again for the PR! Exciting to see improvements already! Some thoughts on this from me would be:
|
Absolutely (I actually considered asking if this somewhat unwieldy inline shell script should become a separate file!), but I wanted to give it a chance to work out any minor kinks on Linux first as that's where I develop and can investigate issues. If you're keen to get it on appveyor asap then I can do this now.
Sadly not :( http://stackoverflow.com/questions/12641469/list-submodules-in-a-git-repository:
Things are done in two steps:
Hopefully this answers your question? |
Good lord.
Does indeed, thanks! Sounds good to me :) |
Started to look at appveyor and encountered a potential problem related to cache size - https://www.appveyor.com/docs/build-cache/#cache-size-beta. All the git repos combined come to ~1.1GB when compressed in the way they describe, so depending on the plan rust is on (and the size of the objects, which I'm not sure how to inspect), this could blow the cache. @alexcrichton is this possibly an issue, or should I just assume it's fine and continue? |
Yeah that should be ok, I believe we have a larger cache than normal. Right now we're caching entire builds of LLVM and that's probably larger than the git repo! |
47326eb
to
9afae97
Compare
Ok, I think this is ready to try. It now implements corruption paranoia, only caching if the pristine repo is known to be in a good state. All the logic is in a standalone script and should also work on appveyor, though I'm not really able to test it. The travis cache seems to work fine (population and usage). @alexcrichton assuming the changes look ok, is it possible to do two tries with bors (one for populating cache, one for cache usage if the first is successful)? Just one try will only test appveyor cache population. Ideally there'd be a way to just test one appveyor build combo (like travis ALLOW_PR) but I don't know if that's possible? |
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 great! Unfortunately I don't know of a way to bounce, save caches, then try again. I think caching for this PR's Travis builds should be working?
.travis.yml
Outdated
@@ -134,11 +134,14 @@ script: | |||
- > | |||
if [ "$ALLOW_PR" = "" ] && [ "$TRAVIS_BRANCH" != "auto" ]; then | |||
echo skipping, not a full build; | |||
elif [ "$TRAVIS_OS_NAME" = "osx" ]; then | |||
travis_retry stamp sh -c 'git submodule deinit -f . && git submodule update --init' && | |||
exit 0; |
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 think this wreaks havoc with Travis's script, no?
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've not seen any issues with it, but I'll rewrite to use &&
.
.travis.yml
Outdated
travis_retry stamp sh -c 'git submodule deinit -f . && git submodule update --init' && | ||
exit 0; | ||
fi; | ||
set -o errexit; |
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.
Maybe we could just use && below?
9afae97
to
0347ff5
Compare
Ah of course, I now recall that appveyor does mention not saving caches on PRs. I've squinted hard at the changes exclusive to appveyor and I can't think of any issues outside of possible 'permission denied' errors (which will be caught by bors e.g. when creating Yes, the travis cache works fine. https://travis-ci.org/rust-lang/rust/jobs/216280916#L446-L450 - this is the time taken for a checkout of the LLVM submodule, using the pristine repo as a reference (and not retrieving anything from the network). In fact, you can see the total time for the I've changed |
@bors: r+ Looks great! Let's see how this fares on appveyor |
📌 Commit 96e174f has been approved by |
@bors: r+ |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit 96e174f has been approved by |
…lexcrichton Attempt to cache git modules Partial resolution of rust-lang#40772, appveyor remains to be done once travis looks like it's working ok. The approach in this PR is based on the `--reference` flag to `git-clone`/`git-submodule --update` and is a compromise based on the current limitations of the tools we're using. The ideal would be: 1. have a cached pristine copy of rust-lang/rust master in `$HOME/rustsrc` with all submodules initialised 2. clone the PR branch with `git clone --recurse-submodules --reference $HOME/rustsrc git@github.com:rust-lang/rust.git` This would (in the nonexistent ideal world) use the pristine copy as an object cache for the top level repo and all submodules, transferring over the network only the changes on the branch. Unfortunately, a) there is no way to manually control the initial clone with travis and b) even if there was, cloned submodules don't use the submodules of the reference as an object cache. So the steps we end up with are: 1. have a cached pristine copy of rust-lang/rust master in `$HOME/rustsrc` with all submodules initialised 2. have a cloned PR branch 3. extract the path of each submodule, and explicitly `git submodule update --init --reference $HOME/rustsrc/$module $module` (i.e. point directly to the location of the pristine submodule repo) for each one I've also taken some care to make this forward compatible, both for adding and removing submodules. r? @alexcrichton
It merged. Next build: https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2621
Uh oh. I'm going to prepare a PR to disable this on appveyor. |
Partial resolution of #40772, appveyor remains to be done once travis looks like it's working ok.
The approach in this PR is based on the
--reference
flag togit-clone
/git-submodule --update
and is a compromise based on the current limitations of the tools we're using.The ideal would be:
$HOME/rustsrc
with all submodules initialisedgit clone --recurse-submodules --reference $HOME/rustsrc git@github.com:rust-lang/rust.git
This would (in the nonexistent ideal world) use the pristine copy as an object cache for the top level repo and all submodules, transferring over the network only the changes on the branch. Unfortunately, a) there is no way to manually control the initial clone with travis and b) even if there was, cloned submodules don't use the submodules of the reference as an object cache. So the steps we end up with are:
$HOME/rustsrc
with all submodules initialisedgit submodule update --init --reference $HOME/rustsrc/$module $module
(i.e. point directly to the location of the pristine submodule repo) for each oneI've also taken some care to make this forward compatible, both for adding and removing submodules.
r? @alexcrichton