8000 Bazel example (Take #2️⃣) by vorburger · Pull Request #1132 · actions/cache · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Bazel example (Take #2️⃣) #1132

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 21, 2023
Merged

Bazel example (Take #2️⃣) #1132

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4b381be
Add example for Bazel
davidB Apr 29, 2021
ef11f54
Fix example for Bazel
vorburger Mar 11, 2023
4b8460c
Create separate Linux/macOS examples for Bazel
vorburger Mar 13, 2023
6f1f1e1
Clarify that macos-latest image has bazelisk
vorburger Mar 13, 2023
8f2671f
Merge branch 'main' into bazel-example
vorburger Mar 13, 2023
bd9b49b
Merge branch 'main' into bazel-example
Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions examples.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
- [Swift, Objective-C - CocoaPods](#swift-objective-c---cocoapods)
- [Swift - Swift Package Manager](#swift---swift-package-manager)
- [Swift - Mint](#swift---mint)
- [* - Bazel](#---bazel)

## C# - NuGet

Expand Down Expand Up @@ -657,3 +658,35 @@ steps:
restore-keys: |
${{ runner.os }}-mint-
```

## * - Bazel

[`bazelisk`](https://github.com/bazelbuild/bazelisk) does not have be to separately downloaded and installed because it's already included in GitHub's `ubuntu-latest` and `macos-latest` base images.

### Linux

```yaml
- name: Cache Bazel
uses: actions/cache@v3
with:
path: |
~/.cache/bazel
key: ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion', '.bazelrc', 'WORKSPACE', 'WORKSPACE.bazel', 'MODULE.bazel') }}
restore-keys: |
${{ runner.os }}-bazel-
Comment on lines +675 to +676
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that we shouldn't do partial restore with Bazel (at least not that wide), since most of the restored files won't be used anyway, and they are going to be saved in the new cache, wasting cache space and increasing restore times.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsvipul I'm reluctant to make changes to a PR after a LGTM.. before I do, could you comment if you want this to be removed or not? Personally I'm not sure I see the harm in this being there, and in my project it does seem to help quite a bit to speed up things on small dependency changes. It also does not seem consistent, to me, for Bazel to be the only example where there is not restore-keys? Isn't what @PiotrSikora alludes to something that could be said for all examples and that is not specific to Bazel per se?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimming through examples, deno doesn't have restore-keys either.

But if you want to retain it here, I'd probably add more layers of restore-keys, e.g.:

restore-keys: |
    ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion', '.bazelrc', 'WORKSPACE', 'WORKSPACE.bazel') }}
    ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion', '.bazelrc') }}
    ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion') }}
    ${{ runner.os }}-bazel-

Copy link
Contributor
@vsvipul vsvipul Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its upto you @vorburger . But in most cases we don't include the hashFiles in restore-keys the key with the hashFiles is already accounted for in an exact match. Also, in some cases we don't keep restore-keys where the restored cache might not be very useful. I don't have much clue about bazel so can't comment on that. would leave the decision to you.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the restore-keys above is wrong, since those hashFiles would all evaluate to different hashes, even if the other files didin't change, and there would be no partial match. It should be this instead:

key: ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion') }}-${{ hashFiles('.bazelrc') }}-${{ hashFiles('WORKSPACE', 'WORKSPACE.bazel') }}-${{ hashFiles('MODULE.bazel') }}
restore-keys: |
    ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion') }}-${{ hashFiles('.bazelrc') }}-${{ hashFiles('WORKSPACE', 'WORKSPACE.bazel') }}
    ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion') }}-${{ hashFiles('.bazelrc') }}
    ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion') }}
    ${{ runner.os }}-bazel-

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Piotr, I believe I now finally better understand what you are after. You mean that there is a sort of "hierarchy" involved here, yes? Like this, a .bazelversion change causes the entire cache to be thrown away (which would make sense to me as well, yes), but a .bazrlrc version causes some cache reuse, etc.? That's a nice idea - but I'm personally not familiar enough with the cache action to be sure that this will really work like this. Vipul, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that i need to know when each of these files change '.bazelversion', '.bazelrc', 'WORKSPACE', 'WORKSPACE.bazel', 'MODULE.bazel' to allow some kind of reuse for cache and I have never used bazel myself so i'm hesitant to merge something which I'm not sure about. I feel the current state of the PR is good enough for normal users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, I don't mind if this gets merged as-is. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsvipul can I do anything else to now see this get merged as-is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. this lgtm.

- run: bazelisk test //...
```

### macOS

```yaml
- name: Cache Bazel
uses: actions/cache@v3
with:
path: |
/private/var/tmp/_bazel_runner/
key: ${{ runner.os }}-bazel-${{ hashFiles('.bazelversion', '.bazelrc', 'WORKSPACE', 'WORKSPACE.bazel', 'MODULE.bazel') }}
restore-keys: |
${{ runner.os }}-bazel-
Comment on lines +689 to +690
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

- run: bazelisk test //...
```
0