Skip to content
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

Added .bazelversion for bazelisk #2075

Closed
wants to merge 1 commit into from

Conversation

corco
Copy link
Collaborator

@corco corco commented Jan 15, 2024

This is so bazelisk users will use the right bazel version instead of the latest (which right now is 7.0.0 and does not work with verible)

This also affects devcontainer users (with PR #2068)

I put version 6.4.0 there, to me it's either that or 5.0

@hzeller
Copy link
Collaborator

hzeller commented Jan 22, 2024

I don't really like the hard pinning on a particular version.
Is it possible to give .bazelversion a range of working versions ? That way bazelisk can check if the system-installed version is already there.

If choose a version, it should be the oldest supported one, so something 5.x

@hzeller
Copy link
Collaborator

hzeller commented Jan 28, 2024

So I played around with this, but neither bazel nor bazelisk understand semantic versioning, i.e. it is not possible to tell it 'use version 5' and it will just use any version 5 if it is on the system. bazel will insist that it can't do anything as not exactly version 5.0.0 is installed and bazelisk will attempt to download some binary from somewhere, which is also not something acceptable in any kind of hermetic environment.

As-is, this pull request can't be put in.
We should probably ask the bazel and bazelisk folks to understand the concept of semantic versioning.

Until then, for us the first step would be to only include the .bazelversion or .basliskrc for the devcontainers if that is possible for there.

Second step would be to make verible compile with all versions, including 7.x. It seems that at least it has issues the install target, and possibly protobufs.

@corco
Copy link
Collaborator Author

corco commented Jan 28, 2024

I also played around with it. If a user has bazelisk installed, it doesn't have any bazel version per se. Bazelisk will always use the .bazelversion if available, or use the latest version otherwise. Not having a .bazelversion means using bazel will use a 7.x version Today, and will use 8.x as soon as it is available.

I thinks the following option are available:

  • Use a fixed bazelversion like the current state of this PR and update it in way similar to the WORKSPACE file. In that case I think best to use the latest supported version or the oldest supported version.
  • Bazelisk has semantic versioning by specifying "5.x" or "6.x" in .bazelversion, which will download the latest version prior to running. Again, a choice between latest or oldest series supported.
  • Delete the .bazelversion and use an environment variable in .devcontainer. I don't like this as currently the build fails for bazelisk user and it is easily fixed by adding the .bazelversion.

Supporting bazel 7 would be best, which means adding bzlmod and (I think) removing support for bazel 5

@hzeller
Copy link
Collaborator

hzeller commented Jan 29, 2024

The semantic versioning like 5.x in bazelisk is actually 'download the latest 5' instead of 'use whatever 5 you find on the system, only then consider downloading a random binary from the Internet'. If .bazelversion is something like 5.x, then if invoked with regular bazel, it is complaining that it is not version 5.x (that string).

I agree the way forward would be to see what needs to be done with to get it going with bazel 7 (I think it does not require bazelmod yet, so we still can see if we can keep it compatible with 5).

@corco
Copy link
Collaborator Author

corco commented Jan 31, 2024

Closing in favor of adding environment variable USE_BAZEL_VERSION in devcontainer

@corco corco closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants