-
Notifications
You must be signed in to change notification settings - Fork 805
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
[bazel] Bump Bazel version to 6.2.1 #17021
[bazel] Bump Bazel version to 6.2.1 #17021
Conversation
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 did some testing earlier and it looks good. Passed all non-broken bazel tests we have IIRC. We need this for remote execution to let us tag fpga tests as "exclusive-if-local"
This might need users to update and should probably be scheduled and announced.
LGTM
Thanks, @drewmacrae! I saw your PR (#16949) and hoped I could separate this part of it. |
@timothytrippel Do you have concerns about the failing airgapped build? It looks like it hits |
Yes, we should probably test this in our airgapped environment before pushing this through. |
63d2cbd
to
e0724db
Compare
Looks like some more dependencies may need to be added to the fetch list in util/prep-bazel-airgapped-build.sh
|
e0724db
to
e3d1f57
Compare
I had to add a couple arguments to the |
I see that
Oddly, it repeatedly passes on my machine. I even tried running it a bunch of times, since it seems to be a randomized test.
I think I'll ask CI to try it again. |
I think we're on-track to have the extra-resources feature get cherry-picked to 6.1.0 so we'll probably want to upgrade when it's released. |
@drewmacrae Great! Let's proceed to upgrade to Bazel 6 and then upgrade to 6.1.0 when it's time :) @cfrantz Do you have any thoughts on the failure in |
Now that we have bazel with "extra resources" I'd prefer we use something later than 6.1.0 so I've added #17664 |
Do we want to revive this now that ES freeze is over? |
I'm biased, but I think this is a good idea! Bazel 6 adds support for Bzlmod, which may simplify our third-party dependencies. The new cquery flag The latest release, 6.2.1, also includes Drew's PR that I believe would help a single Bazel instance manage multiple FPGAs — basically a finer-grained control than the "exclusive" tag. There'd still be some integration work like teaching opentitantool how to select the correct JTAG adapter for a given FPGA. bazelbuild/bazel#17229 Release notes: https://github.com/bazelbuild/bazel/releases/tag/6.0.0 |
Yes I think we should be safe to upgrade. Once this PR gets rebased and is passing CI I can test it on our nightly regression infra to make sure the newer version works there too. |
Signed-off-by: Dan McArdle <[email protected]>
e3d1f57
to
4578cb8
Compare
Thanks, folks! Just rebased and changed from Bazel 6.0.0 to 6.2.1. I also created an experimental PR to see if we can omit the changes to the airgapped script: #19187. |
@timothytrippel CI is green! Could you give this a shot on the nightly infra? I'll wait for you to give the go-ahead before merging this PR. |
I tested this and it seems to work on our infrastructure, so I am merging. |
This PR experimentally bumps the Bazel version to
6.0.06.2.1, the latest release. Let's see what CI makes of it.CC @arunthomas