Git repo cache #7424

Globegitter · 2019-02-13T22:57:07Z

This picks up the work mentioned here #5928 (comment) and should fix #5116

I refactored things a bit and made sure that the git cache actually works offline as well as without a cache and added a simple test. I have tested it in our monorepo and it seems to be working well.

Not sure what the TODO needs a proper implementation. in the _hash method refers to maybe you now @aehlig or @siedentop ?

@aehlig

Inspired by the following pull request and done in collaboration with @aehlig (Klaus Aehlig) at the Bazel Hackathon 2018. #5928 Lots of open issues remaining: * Unit tests needs to be adapted to the fact that there is now a cache. * Klaus would like it to be optional only (I agree). * 'git worktree add ...' does not work on second run. --> tests failing because of that.

…HE is set.

googlebot · 2019-02-13T22:57:12Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Globegitter · 2019-02-13T22:57:44Z

@siedentop could you please confirm the CLA - thanks :)

siedentop · 2019-02-14T03:14:18Z

Hi @Globegitter and @googlebot , I have confirmed the CLA a few years back. This was under my old handle "unapiedra" but still the same GitHub account.

Can I do anything to confirm it again?

siedentop · 2019-02-14T03:22:27Z

@Globegitter I've just seen your comment regarding the TODO in the _hash function. This has been a while back, but I am pretty sure that @aehlig and I implemented the hashing here through the shell as a shortcut, to quickly get some hash implementation. In my recollection I was of the opinion back then, that the maintainers would not approve the use of the shell for this.

An alternative to hashing would be simply to sanitize the remote URL to work as a file name. This has the benefit that it stays readable.

jin · 2019-02-14T16:31:46Z

@siedentop I cannot find any Google CLAs signed by this current username. I believe you'll have to sign it again at https://cla.developers.google.com/ while logged in to the email used by this GitHub account, and for the commits.

siedentop · 2019-02-14T18:05:53Z

@jin I had to update the username. Could you please check again?

jin · 2019-02-14T18:06:30Z

Looks good!

googlebot · 2019-02-14T18:06:46Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

Globegitter · 2019-02-14T20:20:45Z

I am seeing the tests are just failing on Ubuntu 14.04 - is there any way I can look at the logs @aehlig

siedentop · 2019-02-16T00:41:06Z

@Globegitter you can find the logs under the "Artifacts" tab on Build Kite:
https://buildkite.com/bazel/bazel-bazel-github-presubmit/builds/1733#35f70797-c976-405f-9098-c84e461744e7

And the specific log is here: https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/35f70797-c976-405f-9098-c84e461744e7/src/test/shell/bazel/skylark_git_repository_test/attempt_3.log

Globegitter · 2019-02-18T10:03:29Z

Great thanks for that pointer @siedentop

googlebot · 2019-02-18T10:05:52Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Googlers can find more info about SignCLA and this PR by following this link.

Globegitter · 2019-02-18T11:28:13Z

The error on 14.04 is git: 'worktree' is not a git command. See 'git --help'. is there any way we can upgrade git on the the image @aehlig? I am not sure what the procedure is for a case like this.

tools/build_defs/repo/git.bzl

Globegitter · 2019-03-24T19:29:58Z

The test is now skipped if git worktree --help does not exist. So it is ready to review now. @aehlig any chance to get a review?

aehlig · 2019-04-16T11:16:32Z

The test is now skipped if git worktree --help does not exist.

Is skipping the test entirely the right thing to do? Yes, we only expect caching, if the version of git supports it, but we still should ensure that we can fall back to a normal checkout if an older version of git is installed.

aehlig

Thanks for your pull request and I'm sorry for the long delay. I generally like the idea of having a cache for git repositories. However, I think your patch does not yet provide what one usually expects of a cache; can you please address the comments.

Thanks,
Klaus

aehlig · 2019-04-16T11:22:17Z

src/test/shell/bazel/skylark_git_repository_test.sh

+
+  # Do not run tests if the git worktree command does not exist.
+  # Testing without if check - is windows timing out due to that?
+  # if git worktree --help; then


Why is this condition commented out? Should it be there? Or should we actually run the test even in this case, and just skip the verification that the correct hash ended up in the cache.

aehlig · 2019-04-16T11:27:14Z

src/test/shell/bazel/skylark_git_repository_test.sh

+  do_git_repository_test $commit_hash
+
+  cache_commit_hash="$(cat $cache_dir/worktrees/pluto/HEAD)"
+  assert_equals $commit_hash $cache_commit_hash


A verification of caching usually verifies that we can do a clone from cache only in an "offline" situation, if we successfully filled the cache first, and attempt to check out a fixed hash (not a branch). I don't see your test verifying that main property of a cache.

aehlig · 2019-04-16T11:34:44Z

tools/build_defs/repo/git.bzl

+            _populate_cache(ctx, git_cache, remote_name, ref, shallow)
+            st = _get_repository_from_cache(ctx, directory, ref, git_cache)
+
+            if st.return_code:


Should we fail or fall back to using no cache?

aehlig · 2019-04-16T11:51:33Z

tools/build_defs/repo/git.bzl

+        st = _get_repository_from_cache(ctx, directory, ref, git_cache)
+
+        if st.return_code:
+            _populate_cache(ctx, git_cache, remote_name, ref, shallow)


This approach has the problem, that git fetch is called unconditionally, and git fetch tries to contact the remote site, even if we only need a commit already in cache. That means, we cannot use the cache when being offline, which would be a main use case for a cache.

This can be avoided by first trying to check out the correct commit and only if that fails, trying to update the cache.

jin · 2019-10-02T10:52:20Z

Closing as activity on this seem to have stalled. PR authors: please address the review comments and rebase off master.

Globegitter · 2020-02-11T10:14:34Z

I unfortunately will not have time in the foreseeable future to address the PR comments and bring this in a mergable state. So feel free to close for now. If I do hopefully get time again I am happy to reopen this as a new PR. But would be great if anyone else interested in this could pick this up.

aiuto · 2020-02-14T15:04:55Z

Closing again for staleness.
PR authors: please address the review comments and rebase off master.

siedentop and others added 9 commits October 11, 2018 17:20

Fixup: Replace tabs & left-over work

72f53fa

Fix error message

3295078

Merge branch 'git_repo_rule_cache' into git-repo-cache

015bb2d

Some progress on caching git repos only when BAZEL_GIT_REPOSITORY_CAC…

cf9effb

…HE is set.

Merge latest master.

ca447ed

Further progress and fixes for git cache.

3f0cabb

Fix tests.

135cffc

Add simple test.

e2666dd

googlebot added the cla: no label Feb 13, 2019

jin added cla: yes and removed cla: no labels Feb 14, 2019

jin assigned aehlig Feb 14, 2019

jin added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Feb 14, 2019

Missing variable fix.

edc5a0a

googlebot added cla: no and removed cla: yes labels Feb 18, 2019

siedentop reviewed Feb 20, 2019

View reviewed changes

tools/build_defs/repo/git.bzl Outdated Show resolved Hide resolved

tools/build_defs/repo/git.bzl Outdated Show resolved Hide resolved

tools/build_defs/repo/git.bzl Outdated Show resolved Hide resolved

globegitter added 12 commits February 28, 2019 10:00

Merge branch 'master' into git-repo-cache

aa84693

Progress on error.

f4e813d

Manually merge master.

a04e22e

Do not check git version anymore.

95ad420

Remove stray shallow and quote properly.

3b6bc56

Add additional fetch fallback without shallow and fix typo.

157ac5f

Add better error message.

61cd5ed

str of hash.

aa08e59

Try other multiline string.

2f2492f

Skip test on ubuntu 14.04.

79ffdb6

Fix tests.

acb3696

Fix if check to check for worktree command directly.

da14007

globegitter added 2 commits April 5, 2019 00:07

Retrigger tests.

28db515

Rerun tests normally everywhere.

cd5267d

aehlig reviewed Apr 16, 2019

View reviewed changes

Globegitter mentioned this pull request May 12, 2019

build_bazel_rules_nodejs should rarely require updates bazel-contrib/rules_nodejs#740

Closed

Globegitter mentioned this pull request Jun 21, 2019

go_repository: git repositories should be stored in the cache bazel-contrib/bazel-gazelle#549

Open

jin closed this Oct 2, 2019

jin reopened this Oct 10, 2019

aiuto closed this Feb 14, 2020

philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020

philwo removed the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Nov 29, 2021

sluongng mentioned this pull request Feb 15, 2022

Should git_repository use repository cache? #5116

Open

thii mentioned this pull request May 20, 2022

Add support for repository cache to git_repository #15536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Git repo cache #7424

Git repo cache #7424

Globegitter commented Feb 13, 2019 •

edited

Loading

googlebot commented Feb 13, 2019

Globegitter commented Feb 13, 2019

siedentop commented Feb 14, 2019

siedentop commented Feb 14, 2019

jin commented Feb 14, 2019

siedentop commented Feb 14, 2019

jin commented Feb 14, 2019

googlebot commented Feb 14, 2019

Globegitter commented Feb 14, 2019 •

edited

Loading

siedentop commented Feb 16, 2019

Globegitter commented Feb 18, 2019

googlebot commented Feb 18, 2019

Globegitter commented Feb 18, 2019

Globegitter commented Mar 24, 2019

aehlig commented Apr 16, 2019

aehlig left a comment

aehlig Apr 16, 2019

aehlig Apr 16, 2019

aehlig Apr 16, 2019

aehlig Apr 16, 2019

jin commented Oct 2, 2019

Globegitter commented Feb 11, 2020 •

edited

Loading

aiuto commented Feb 14, 2020

Git repo cache #7424

Git repo cache #7424

Conversation

Globegitter commented Feb 13, 2019 • edited Loading

googlebot commented Feb 13, 2019

Globegitter commented Feb 13, 2019

siedentop commented Feb 14, 2019

siedentop commented Feb 14, 2019

jin commented Feb 14, 2019

siedentop commented Feb 14, 2019

jin commented Feb 14, 2019

googlebot commented Feb 14, 2019

Globegitter commented Feb 14, 2019 • edited Loading

siedentop commented Feb 16, 2019

Globegitter commented Feb 18, 2019

googlebot commented Feb 18, 2019

Globegitter commented Feb 18, 2019

Globegitter commented Mar 24, 2019

aehlig commented Apr 16, 2019

aehlig left a comment

Choose a reason for hiding this comment

aehlig Apr 16, 2019

Choose a reason for hiding this comment

aehlig Apr 16, 2019

Choose a reason for hiding this comment

aehlig Apr 16, 2019

Choose a reason for hiding this comment

aehlig Apr 16, 2019

Choose a reason for hiding this comment

jin commented Oct 2, 2019

Globegitter commented Feb 11, 2020 • edited Loading

aiuto commented Feb 14, 2020

Globegitter commented Feb 13, 2019 •

edited

Loading

Globegitter commented Feb 14, 2019 •

edited

Loading

Globegitter commented Feb 11, 2020 •

edited

Loading