-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Use bazelisk to download bazels for tests, instead of trying to download #181
Conversation
I'm a bit nervous about the mac and windows detection here. |
looks like I needed to test with bzlmod |
Ah, because I didn't update the bzlmod bazel_binaries extension. |
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.
Overall, looks good. I left a couple of small comments in the code. We should also add an example that uses this new functionality and include it in the integration test suite.
This is more of a thought and not an action item. Have you thought about whether we could just use the bazelisk
installed on the client's system, if it is detected? Typically, I am all about downloading the tools that we need. However, bazelisk
is special in that it is the bootstrap for launching Bazel. 🤔
As far as integration tests, all the tests and examples are now using this. Do we need further tests? I was trying to not add any behavior differences. Using the local bazelisk sounds very similar to using a local bazel, maybe this is part of #100? |
That is fair. Do we need any doc updates to let folks know that they can use the full range of Bazel version specifications that Bazelisk supports? In other words, how do you want to advertise this new capability?
Maybe. We do not need to worry about using the local |
Rebased to main and added documentation (and a TODO issue for the bazelisk version). |
Any chance this can be added to a release? |
directly.
Fixes #67.