-
Notifications
You must be signed in to change notification settings - Fork 544
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
deps: updates for bazel 8 compatibility #2379
Conversation
2eb3ccd
to
d7b780a
Compare
dcf2d27
to
bfe467c
Compare
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.
The only real question is the defs.bzl
usage from rules_cc
. I remember there was a PR here #2293, but I can still find usage of defs.bzl
in our git repo in this PR.
@@ -59,3 +59,6 @@ pip.parse( | |||
|
|||
# example test dependencies | |||
bazel_dep(name = "rules_shell", version = "0.2.0", dev_dependency = True) | |||
|
|||
# Only needed to make rules_python's CI happy | |||
bazel_dep(name = "rules_java", version = "8.3.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.
This is a little bit mysterious. Will we be able to remove it at some point?
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 updated the comment to explain why it's here.
The example sets --java_runtime_version=remotejdk_11
, which is broken with Bazel 8 until rules_java 8.3.
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.
And if I remember correctly this is only to make the coverage collection not depend on system Java. So technically all of our consumers will need to add this dep if they want coverage.
srcs = [ | ||
# It appears something in the CC rules loads cc_proto_library, but | ||
# where isn't clear. | ||
"@com_google_protobuf//:bzl_srcs", |
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.
Should we remove the usage of defs.bzl
, maybe that will fix it?
$ git grep "defs.bzl.*cc" **/*.bzl
python/private/common_bazel.bzl:load("@rules_cc//cc:defs.bzl", "CcInfo", "cc_common")
python/private/hermetic_runtime_repo_setup.bzl:load("@rules_cc//cc:defs.bzl", "cc_import", "cc_library")
python/private/local_runtime_repo_setup.bzl:load("@rules_cc//cc:defs.bzl", "cc_library")
python/private/py_executable.bzl:load("@rules_cc//cc:defs.bzl", "cc_common")
python/private/runtime_env_toolchain.bzl:load("@rules_cc//cc:defs.bzl", "cc_library")
python/private/whl_filegroup/whl_filegroup.bzl:load("@rules_cc//cc:defs.bzl", "cc_library")
tests/support/cc_toolchains/fake_cc_toolchain_config.bzl:load("@rules_cc//cc:defs.bzl", "cc_common")
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.
Hm, yeah, removing defs.bzl might fix this (depends on if the more specific targets properly specify their bzl_libraries 😅 )
I wrote this comment before (1) I unwound the whole defs.bzl removing cc_proto_library thing, and (2) I gave up trying to make bazel 6 with bzlmod work.
I've updated the comment to better reflect this.
Ah, I read the PR I linked - I somewhat think that maybe we could couple the PRs together and see if we can remove the extra protobuf deps that we had to add. |
nit: I think we should also resolve the following warnings in the
|
Yeah, this whole part is a mess. Our usage of rules_cc/defs.bzl is OK, sort of -- deprecated, but not broken, per-se. The problem comes when other dependencies come into play. Basically, rules_cc removed a symbol (cc_proto_library) from defs.bzl. However, some things still loaded that symbol, so fixes were sent out. While all this is going on, various versions of modules were released with various problems with Bazel 6, Bazel 8, and/or the removal of that symbol. So, our usage of rules_cc/defs.bzl is only a problem depending on what combinations of what dependencies you end up with. Us switching off rules_cc/defs.bzl makes it less likely to occur (since we wouldn't be a potential cause of loading that file when an incompatible dependency is being used). re: coupling this PR and the defs.bzl removal: Well, I was going to say this PR has been a chore and I don't want to expand it's scope, but now CI is failing with yet-another-rules-cc-defs.bzl looking failure. So maybe it'll be easier to put those in one PR. I'll have a look at that failure to see which is gonna be easier. re: module versions:
|
The failure was: Bazel 9 with workspace was failing because Bazel 9 removes I've "fixed" this by removing the bazel-in-bazel integration tests for rolling, A bzlmod invocation is OK because it uses protobuf 29.0-rc1+, which has a fix for that. However, Bazel 9 with workspace is using an earlier protobuf version. I tried updating that, but it looks like it has further dependencies that need to be setup and/or upgraded. Given that Bazel 9 is a ways off, I think it makes sense to temporarily disable that test. I'll need some time to try and figure out the necessary WORKSPACE updates that need to be made. |
Thanks for all of this untangling of the mess. This is sure a bumpy one - I remember bazel 7 not being nearly as bad. |
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.
Approving since the CI is green and we can go bit by bit fixing all of the remaining things. My main concerns are:
- Can we remove some of the deps added here by switching away from
rules_cc:defs.bzl
. - The
java
flag in thebazelrc
was there forbazel coverage
support and I don't understand if we can safely remove it and the coverage will just work or we have to document it to the example code readers, that they need to have that dep as well?
Regardless of the outcome of the above, this is moving in the right direction.
Various changes to support Bazel 8. An important note is dependencies have forced
us to change the versions of Bazel we support.
Summary of changes:
some things. Things seemed to have settled by 0.0.14.
respects (and thus fails), while other platforms ignore.
dependencies.
last_rc
Bazel (currently the 8.x RC).once native.sh_binary reference in def.bzl blocks Bazel 8 integration bazel-contrib/bazel-gazelle#1959 is fixed and released.
$(rpathlocation)
call in bootstrap tests.bzl sources changed in rules_cc and protobuf in these newer versions.
Sorting this all out and finding the right combination of dependency versions was
fairly involved. The details of that are in
#2378.
Work towards #2378, #2387