Use setup-bazel GHA for disk and repository caching

[tom-osika]

Towards #106

I tested the caching with a few different commits:

1. The initial commit (before I rebased). Note the expected cache misses here. Check out the Run bazel-contrib/setup-bazel@0.15.0 step
2. I modified a few things (including a better description of what setup-bazel is doing) and rebased those into my initial commit. There was an expected cache hit for both the repo and disk caches here
3. To ensure that the repository cache uses the same key regardless of modifications to files setup-bazel hashes by default, I added a temporary commit modifying MODULE.bazel. Expected cache hit there as well

We'll have to make sure the caching works on master, but I don't think we can test that until merging.

---

This change is 

[tom-osika]

Reviewable status: 1 unresolved discussion, platform LGTM missing (waiting on @jwnimmer-tri)

---

MODULE.bazel line 10 at r1 (raw file):

    "http_archive",
)
# TODO: Remove this line after GHA succeeds

FYI I left this in just so you could see how I tested the repository cache working, @jwnimmer-tri . I can remove this commit anytime.

---

.github/workflows/main.yml line 26 at r1 (raw file):

      - uses: bazel-contrib/setup-bazel@0.15.0
        with:
          # Store build cache globally

Note that we expect PR runs to overwrite the main caches

[jwnimmer-tri]

@jwnimmer-tri reviewed 3 files and all commit messages, made 6 comments, and resolved 1 discussion.
Reviewable status: 7 unresolved discussions, platform LGTM missing (waiting on @tom-osika).

---

.github/workflows/main.yml line 18 at r1 (raw file):

    env:
      # This is where bazelisk caches its downloads of Bazel.
      BAZELISK_HOME: /home/runner/.cache/bazel_ci/bazelisk

This probably needs to change. This directory .cache/bazel_ci is where we were manually caching things with our GHA custom yaml stuff (that has now been removed).

At best, we should just delete these lines and the setup-bazel already handles bazel download caching (IIRC yes it does, but can you confirm?). If setup-bazel doesn't handle it, we should keep around code that does (and/or upstream the fix to setup-bazel to do so).

Code quote:

      # This is where bazelisk caches its downloads of Bazel.
      BAZELISK_HOME: /home/runner/.cache/bazel_ci/bazelisk

---

.github/workflows/main.yml line 23 at r1 (raw file):

      - name: Install Ubuntu dependencies
        run: .github/ci_setup.bash
      # Use setup-bazel for caching

nit Missing period at end of sentence.

---

.github/workflows/main.yml line 26 at r1 (raw file):

      - uses: bazel-contrib/setup-bazel@0.15.0
        with:
          # Store build cache globally

nit For the sanity of future readers, we should provide a thorough exposition the effect of this option and why we've chosen it.

(Also missing period at end of sentence.)

Suggestion:

# Enable the disk cache and share it across all workflows
# (i.e., across master and pull requests).
# We want all builds to start from a warm cache, and we're not too
# concerend about pull request builds "polluting" master builds.

---

.github/workflows/main.yml line 31 at r1 (raw file):

          # it uses the hash of a few bazel files as the key for the cache. You can set
          # your own list of files to use to calculate the key by passing them below.
          # For now, we pass an empty string. This tells setup-bazel to use the same key

nit We can elide the sense of time from this phrasing:

Suggestion:

Passing an empty string tells

---

.github/workflows/main.yml line 36 at r1 (raw file):

      - name: Report cache sizes
        run: |
          du -ms ~/.cache/* || true

These line should go away as well? Leftovers from hand-rolled caching, I think.

Code quote:

      - name: Report cache sizes
        run: |
          du -ms ~/.cache/* || true

---

.github/ci.bazelrc line 6 at r1 (raw file):

# Dump configuration details to the log.
common --announce_rc=yes

Now that this rcfile is so small, do you think it would be clearer to use the bazelrc option in setup-bazel to specify it, instead of a separate file?

[tom-osika]

@tom-osika made 3 comments.
Reviewable status: 7 unresolved discussions, platform LGTM missing (waiting on @jwnimmer-tri).

---

.github/ci.bazelrc line 6 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Now that this rcfile is so small, do you think it would be clearer to use the bazelrc option in setup-bazel to specify it, instead of a separate file?

Yes, I think that's better. See the latest run here. I used the bazelrc option and removed the ci.bazelrc file. You can see that the --announce_rc=yes still gets propagated to bazel in the Bazel Test step.

---

.github/workflows/main.yml line 18 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This probably needs to change. This directory .cache/bazel_ci is where we were manually caching things with our GHA custom yaml stuff (that has now been removed).

At best, we should just delete these lines and the setup-bazel already handles bazel download caching (IIRC yes it does, but can you confirm?). If setup-bazel doesn't handle it, we should keep around code that does (and/or upstream the fix to setup-bazel to do so).

Yes, good call. Luckily setup-bazel does handle this with the bazelisk-cache parameter.

In the latest run, I use this parameter. In the Run bazel-contrib step, you can see that there was a cache hit for Bazelisk (as I made a few commits after the initial commit adding this parameter).

---

.github/workflows/main.yml line 36 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

These line should go away as well? Leftovers from hand-rolled caching, I think.

This is leftover from the old caching code, but I originally updated it to report the size of the new cache directory (but maybe we don't care about reporting the size anymore). I can remove it, leave it, or update it to report the size of the disk and repository separately. Whichever is preferred

[jwnimmer-tri]

@jwnimmer-tri reviewed 3 files and all commit messages, made 2 comments, and resolved 5 discussions.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @tom-osika).

---

MODULE.bazel line 10 at r1 (raw file):

Previously, tom-osika (Tom Osika) wrote…

FYI I left this in just so you could see how I tested the repository cache working, @jwnimmer-tri . I can remove this commit anytime.

Probably can go away now.

---

.github/workflows/main.yml line 36 at r1 (raw file):

Previously, tom-osika (Tom Osika) wrote…

This is leftover from the old caching code, but I originally updated it to report the size of the new cache directory (but maybe we don't care about reporting the size anymore). I can remove it, leave it, or update it to report the size of the disk and repository separately. Whichever is preferred

I think to keep things simple, let's fully remove it. We can add it back as-needed in case we ever need to profile a too-large cache.

[tom-osika]

@tom-osika made 2 comments.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri).

---

.github/workflows/main.yml line 36 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think to keep things simple, let's fully remove it. We can add it back as-needed in case we ever need to profile a too-large cache.

Removed

---

MODULE.bazel line 10 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Probably can go away now.

Removed

[jwnimmer-tri]

@jwnimmer-tri reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @tom-osika).

[jwnimmer-tri]