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

Revert "Update rules_jvm_external to use the Starlark version of aar_import after the native version was removed from Bazel (#1149)" #1215

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

shs96c
Copy link
Collaborator

@shs96c shs96c commented Aug 12, 2024

This reverts commit ba7310c.

The default label for AAR imports is @rules_android//android:rules.bzl. This imports @rules_android//rules:providers.bzl, which calls @rules_android//rules/reexport_providers.bzl. This means that any downstream user of rules_jvm_external with a sufficiently recent version of Bazel needs to add build --experimental_google_legacy_api to their .bazelrc

That's a significantly breaking change, and one that it's not reasonable to ask our users to do, especially since the vast majority of projects don't use the Android rules.

…import after the native version was removed from Bazel (bazel-contrib#1149)"

This reverts commit ba7310c.

The default label for AAR imports is `@rules_android//android:rules.bzl`. This
imports `@rules_android//rules:providers.bzl`, which calls
`@rules_android//rules/reexport_providers.bzl`. This means that any downstream
user of `rules_jvm_external` with a sufficiently recent version of Bazel needs
to add `build --experimental_google_legacy_api` to their `.bazelrc`

That's a significantly breaking change, and one that it's not reasonable to ask
our users to do, especially since the vast majority of projects don't use the
Android rules.
@jin
Copy link
Collaborator

jin commented Aug 12, 2024

cc @ahumesky

@jin
Copy link
Collaborator

jin commented Aug 12, 2024

This looks like bazelbuild/rules_android#219, and we probably cannot roll forward on this until we drop the dependency on that flag.

@shs96c
Copy link
Collaborator Author

shs96c commented Aug 12, 2024

To show this in action:

git clone https://github.com/bazelbuild/rules_jvm_external.git
git clone https://github.com/bazel-contrib/rules_jvm.git
cd rules_jvm
bazel build //... --override_module=$(pwd)/../rules_jvm_external

With the rules_android update in place, this will fail. Without it, the build will succeed.

@shs96c shs96c merged commit 7b4512b into bazel-contrib:master Aug 12, 2024
9 checks passed
@shs96c shs96c deleted the rollback-rules-android-bump branch August 12, 2024 13:42
@ahumesky
Copy link
Contributor

Sorry for the trouble, @rules_android//rules/reexport_providers.bzl is actually a part of removing the need for --experimental_google_legacy_api and was added after starting on ba7310c, didn't realize this would require projects not using the android rules to add the flag.

@shs96c
Copy link
Collaborator Author

shs96c commented Aug 12, 2024

It happens, and I'm surprised it managed to slip through all the tests and checks. We know now, and we can fix things. Genuinely looking forwards to the time we can roll this forward again. Thank you for all your work making that happen!

@ahumesky
Copy link
Contributor

We've made significant progress on bazelbuild/rules_android#256 / bazelbuild/rules_android#219 Maybe this week or next we can try again with an updated version of ba7310c + updated version of rules_android

@shs96c
Copy link
Collaborator Author

shs96c commented Aug 27, 2024

I'd be happy to try it. Would be great to land this properly :)

@ahumesky
Copy link
Contributor

ahumesky commented Oct 11, 2024

With bazel 7.4.0rc1 created today, I tested reapplying ba7310ce4a1d8fb14434597fbc7440a4074f7695 and using the latest rules_android, and this all seems to work now without either --experimental_google_legacy_api nor --experimental_enable_android_migration_apis:

~/rules_jvm$ USE_BAZEL_VERSION=7.4.0rc1 bazelisk build //... --override_module=rules_jvm_external=$(pwd)/../rules_jvm_external --override_module=rules_android=$(pwd)/../rules_android

......

INFO: Found 256 targets...
INFO: Elapsed time: 45.499s, Critical Path: 41.73s
INFO: 2318 processes: 1013 internal, 1237 linux-sandbox, 68 worker.
INFO: Build completed successfully, 2318 total actions

@ahumesky
Copy link
Contributor

ahumesky commented Oct 22, 2024

We've been working to create rules_android version 0.6.0 this week and last, when that's out I'll resume getting all this committed again

@ahumesky
Copy link
Contributor

ahumesky commented Dec 7, 2024

Since we're getting close(r) to a 0.6.0 release of rules_android, I tried my PR again with rules_android at head, and get some new errors.

It looks like there are conflicting definitions of @maven from multiple places:

DEBUG: /usr/local/google/home/ahumesky/rules_jvm_external/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'rules_jvm_external' and now in 'protobuf'
DEBUG: /usr/local/google/home/ahumesky/rules_jvm_external/private/extensions/maven.bzl:155:14: The maven repository 'multiple_lock_files' is used in two different bazel modules, originally in 'rules_jvm_external' and now in 'bzlmod_lock_files'
DEBUG: /usr/local/google/home/ahumesky/rules_jvm_external/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'rules_jvm_external' and now in 'bazel_worker_java'
DEBUG: /usr/local/google/home/ahumesky/rules_jvm_external/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'rules_jvm_external' and now in 'bazel_worker_java'
DEBUG: /usr/local/google/home/ahumesky/rules_jvm_external/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'rules_jvm_external' and now in 'bazel_worker_java'
DEBUG: /usr/local/google/home/ahumesky/rules_jvm_external/private/extensions/maven.bzl:155:14: The maven repository 'maven' is used in two different bazel modules, originally in 'rules_jvm_external' and now in 'bazel_worker_java'

leading to (at least):

ERROR: /usr/local/google/home/ahumesky/.cache/bazel/_bazel_ahumesky/47f161388952f98f7ec077017d030a4d/external/_main~maven~maven/BUILD: no such target '@@_main~maven~maven//:com_google_protobuf_protobuf_java': target 'com_google_protobuf_protobuf_java' not declared in package '' defined by /usr/local/google/home/ahumesky/.cache/bazel/_bazel_ahumesky/47f161388952f98f7ec077017d030a4d/external/_main~maven~maven/BUILD
ERROR: /usr/local/google/home/ahumesky/.cache/bazel/_bazel_ahumesky/47f161388952f98f7ec077017d030a4d/external/bazel_worker_java~/src/main/java/com/google/devtools/build/lib/worker/BUILD.bazel:5:13: no such target '@@_main~maven~maven//:com_google_protobuf_protobuf_java': target 'com_google_protobuf_protobuf_java' not declared in package '' defined by /usr/local/google/home/ahumesky/.cache/bazel/_bazel_ahumesky/47f161388952f98f7ec077017d030a4d/external/_main~maven~maven/BUILD and referenced by '@@bazel_worker_java~//src/main/java/com/google/devtools/build/lib/worker:work_request_handlers'

@ahumesky
Copy link
Contributor

Looks like there are some comments about the 'maven' is used in two different bazel modules issue above here:

# Note: we use 27.2 in MODULE.bazel to avoid the warning:
# The maven repository 'maven' is used in two different bazel modules, originally in 'rules_jvm_external' and now in 'protobuf'
# But we use 21.7 in WORKSPACE because protobuf 27.2 doesn't work with Bazel 5.x
# https://github.com/protocolbuffers/protobuf/commit/a80daa2a2caaaac9ebe9ae6bb1b639c2771c5c55
# This should be ok because we only use the protobuf dep to pull in the google/protobuf/wrappers.proto for testing

Latest rules_android needs to use the latest protobuf, so the android test targets (e.g. //tests/integration/override_targets:override_targets) in rules_jvm_external will start to fail

@ahumesky
Copy link
Contributor

This seems to avoid the @maven conflicts between bazel_worker_java and rules_jvm_external: bazelbuild/bazel-worker-api#8

@ahumesky
Copy link
Contributor

Regarding #1215 (comment) apparently I just needed to do REPIN=1 bazel run @maven//:pin

@ahumesky
Copy link
Contributor

ahumesky commented Dec 12, 2024

I can get rules_android at head + rules_jvm_external at head + reapplying my change ba7310c (at #1297) working with {bazel 8.0.0, bazel 7.4.1} x {bzlmod, workspace}.

When I try bazel 6.5.0 with bzlmod, I get

ERROR: @@rules_jvm_external~override//:MODULE.bazel:110:13: name 'use_repo_rule' is not defined
ERROR: Error computing the main repository mapping: in module dependency chain <root> -> rules_jvm_external@_: error executing MODULE.bazel file for rules_jvm_external@_

So it looks like rules_jvm_external at head doesn't work with bazel 6.5.0 + bzlmod

When I try it with workspace, I get a lot of problem with rules_android requiring things that aren't in bazel 6.5.0 (configuration fields, some things in @bazel_toosl, etc). I can get rules_jvm_external at head + my change above working with the native Android rules however, which are still in bazel 6 and bazel 7. So I think it would be the best course to run any tests in rules_jvm_external that use bazel 6 with the native Android rules.

@jin
Copy link
Collaborator

jin commented Dec 18, 2024

So it looks like rules_jvm_external at head doesn't work with bazel 6.5.0 + bzlmod

This is fine. rules_jvm_external only supports bzlmod with Bazel 7+, not 6.5.0. We only support WORKSPACE with Bazel 6.5.0.

So I think it would be the best course to run any tests in rules_jvm_external that use bazel 6 with the native Android rules.

This works.

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.

None yet

3 participants