-
Notifications
You must be signed in to change notification settings - Fork 5
Upgrade to Bazel 8 #78
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
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.
This LGTM for the most part. I had a question or two, but nothing major.
Thanks for doing this!
.bazelrc_shared
Outdated
common --incompatible_strict_action_env | ||
|
||
# Unfortunately, this can't be enabled just yet because of | ||
# https://github.com/bazelbuild/rules_java/issues/264 |
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.
Reading through that issue it looks like this was fixed and we should be able to flip this now.
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.
Yup, I think it got resolved. Let me try re-enabling it.
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.
Unfortunately, we're still getting this error, despite CcInfo
being imported:
ERROR: Traceback (most recent call last):
File "/home/jpeterson/.cache/bazel/_bazel_jpeterson/b85aa227a3460f0c7b51d6bd8d9136a3/external/rules_java+/java/common/rules/java_runtime.bzl", line 197, column 48, in <toplevel>
"hermetic_static_libs": attr.label_list(
Error in label_list: Illegal argument: element in 'providers' is of unexpected type. Either all elements should be providers, or all elements should be lists of providers, but got an element of type com.google.devtools.build.lib.starlarkbuildapi.core.ContextAndFlagGuardedValue$1.
I'll have to keep it disabled for now. I'll update the comment, though.
.bazelversion
Outdated
@@ -1 +1 @@ | |||
7.4.1 | |||
8.0.0 |
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.
It took me so long to review this that 8.1.0 is out, so I think we could upgrade to that.
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.
Will do.
MODULE.bazel
Outdated
|
||
bazel_dep(name = "buildifier_prebuilt", version = "7.3.1", dev_dependency = True) | ||
|
||
bazel_dep(name = "protobuf", version = "29.3") | ||
bazel_dep(name = "rules_java", version = "7.12.2") |
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.
Any advantage to upgrading rules_java to the 8.x
versions now that we're on Bazel 8? 8.9.0
is the latest version.
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.
Yup, I upgraded to v8.9.0 in my other fixup!
commit 🙂
providers = g.out.providers, | ||
**dynamic | ||
) | ||
return g.out.providers |
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.
Are these equivalent? I'm having a hard time following the diff here.
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 believe so. Now that struct-provider syntax is disabled, I had to refactor this. Since I wasn't able to find anywhere where instrumented_files
or java
are used (and all the tests passed without them), I opted to remove them.
), | ||
replacements = replacements, | ||
) | ||
return struct(replacements = replacements) |
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.
Do we no longer need instrumented_files
?
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.
See my comment here. This was only used in phase_coda
, but I removed that reference to it. Therefore, it was no longer needed.
tests/.bazelversion
Outdated
@@ -1 +1 @@ | |||
7.4.1 | |||
8.0.0 |
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.
Same though there about upgrading to 8.1.0
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.
Done!
.bazelrc_shared
Outdated
# Other options | ||
build --experimental_use_hermetic_linux_sandbox | ||
build --experimental_worker_cancellation | ||
build --experimental_worker_multiplex_sandboxing |
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.
Looks like the CI build is failing because of the multiplex sandboxing bug. Which suggests that bug is either a bug in our rule set or still present in Bazel 8. We'll want to disable this for now.
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.
Yup, I'll re-disable this.
I implemented several of the changes I requested here, fixed a bug, and updated CI to be on Bazel 8. You can just cherry-pick my commits from this branch as it should save you some time https://github.com/lucidsoftware/rules_scala/pull/81/commits |
These were sourced from the Bazel 8 release page: https://github.com/bazelbuild/bazel/releases/tag/8.0.0
Bazel 8 deprecates the rulesets bundled with Bazel (which includes the Protobuf rules) in favor of their Starlark-ified equivalents. For that reason, we switch to the `protobuf` ruleset for `proto_library` and `java_proto_library`. In doing so, we can't use `com.github.os72:protoc-jar` because the version of protoc it bundles is extremely out of date and incompatible with the version of protobuf provided by the Starlark-ified protobuf ruleset. In its place, we use the protoc provided by that ruleset.
5900b37
to
0af5589
Compare
Squashed. |
d59d256
to
fdee388
Compare
Squashed. |
…ction_env The former is deprecated and has been replaced with the latter.
1f1068a
to
1daeb80
Compare
Squashed. |
This PR upgrades the ruleset to Bazel 8. It should also make the upgrade to future Bazel versions easier by enabling a number of
--incompatible_*
flags. These enforce that we're not using deprecated features that will be removed in a future major Bazel version.Note that I wasn't able to enable path mapping because the same bug that prevented us from enabling it with Bazel 7 seems to be pervasive in Bazel 8 as well. I'll look into that soon; I just wanted to get us upgraded first.