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

Update rules_jvm_external to use the Starlark version of aar_import after the native version was removed from Bazel #1149

Merged
merged 15 commits into from
Aug 7, 2024

Conversation

ahumesky
Copy link
Contributor

@ahumesky ahumesky commented May 24, 2024

The native version of aar_import has been removed from bazel at head, so use the Starlark version instead.

This PR:

  1. Updates rules_jvm_external to use the Starlark version of aar_import automatically if the version of Bazel used does not contain the native version of aar_import.

  2. Updates the version of Bazel in .bazelversion to 7.2.1 because the latest Starlark Android rules are only compatible with 7.2.1+

  3. Updates the version of rules_android used to the latest commit, because version 0.1.1 is basically just wrappers around the native versions of the rules. However, version 0.1.1 of rules_android is still used in the WORKSPACE file so that test that run with Bazel 5 and Bazel 6 use the native rules, and Bazel 7+ uses the Starlark rules via bzlmod / MODULE.bazel, because the latest version of the Starlark rules are only compatible with Bazel 7.2.1+.

  4. Updates the default label to import aar_import from, from @build_bazel_rules_android//android:rules.bzl to @rules_android//rules:rules.bzl, both to match the bzlmod style name of the repo ("rules_android") and to use the latest label inside the repo (//android:rules.bzl exists, but only for backwards compatibility).

  5. Fixes some dependency issues with aar_import_that_consumes_the_downloaded_file_directly and sceneform_rendering aar_imports, because the Starlark version of aar_import performs additional checks ("ImportDepsChecker") to ensure that the dependencies of every class in the imported aar are in the dependencies. Notably, the class com.google.ar.sceneform.assets.Loader is used somewhere in the aar or its dependencies, but I couldn't find this class anywhere in any aars on maven.google.com, so I ended up having to stub it out with an empty implementation to satisfy the check.

  6. Disables --experimental_sibling_repository_layout added in b6631f9 because the latest rules_android uses protos in go, and there is a problem compiling protos in go with --experimental_sibling_repository_layout: rules_go + protobufs + experimental_sibling_repository_layout fails to build rules_go#3947

  7. Adds --experimental_google_legacy_api to .bazelrc until Remove dependencies on --experimental_google_legacy_api and --experimental_enable_android_migration_apis bazelbuild/rules_android#219 is fixed

Fixes #1139.

@ahumesky
Copy link
Contributor Author

Oh, I just noticed the tests run on bazel as far back as bazel 5.4.0:
https://github.com/bazelbuild/rules_jvm_external/blob/71e08c82709b24d12e2d1040d86caa05bf6a8399/.bazelci/presubmit.yml#L40

The starlark Android rules will only work with 7.2.0 (releasing soon). Might need to segment the tests so that the tests with the starlark version of aar_import are only run with 7.2.0

@ahumesky
Copy link
Contributor Author

The windows tests will also be a problem because windows support for starlark rules_android is low priority

@shs96c
Copy link
Collaborator

shs96c commented May 25, 2024

We still need to support Windows users, so those tests will need to pass properly before we can land this PR.

@ahumesky
Copy link
Contributor Author

bazelbuild/rules_android#235 should fix up the windows problems

@jin
Copy link
Collaborator

jin commented May 30, 2024

Error looks legit:


(19:43:56) ERROR: C:/b/rxn2cr7v/external/rules_android~/tools/android/BUILD:273:6: in alias rule @@rules_android~//tools/android:dexsharder: target '@@bazel_tools//src/tools/android/java/com/google/devtools/build/android/dexer:DexFileSplitter' is not visible from target '@@rules_android~//tools/android:dexsharder'. Check the visibility declaration of the former target if you think the dependency is legitimate
--
  | (19:43:56) ERROR: C:/b/rxn2cr7v/external/rules_android~/tools/android/BUILD:273:6: Analysis of target '@@rules_android~//tools/android:dexsharder' failed
  | (19:43:57) ERROR: Analysis of target '//tests/unit/aar_import:starlark_aar_import_test' failed; build aborted: Analysis failed

from https://buildkite.com/bazel/rules-jvm-external/builds/4289#018fc5e0-bc05-4701-80b8-55fdf1f6ddb0 (and others)

@ahumesky
Copy link
Contributor Author

That's fixed in 7.2.0, specifically bazelbuild/bazel#22416, but there are other problems revealed after bazelbuild/rules_android#235

I'll make this a draft until CI is passing

@ahumesky ahumesky marked this pull request as draft May 30, 2024 15:44
ahumesky added 5 commits July 9, 2024 16:44
…utomatically

if the version of Bazel used does not contain the native version of aar_import.

This also updates the version of rules_android to the latest commit, because
version 0.1.1 is basically just wrappers around the native versions of the rules.

This also disbles `--experimental_sibling_repository_layout` added in
bazel-contrib@b6631f9
because the latest rules_android uses protos in go, and there is a problem compiling protos in go
with `--experimental_sibling_repository_layout`: bazel-contrib/rules_go#3947

Fixes bazel-contrib#1139.
@ahumesky
Copy link
Contributor Author

ahumesky commented Jul 9, 2024

The tests now work with the latest version of bazel:
https://buildkite.com/bazel/rules-jvm-external/builds/4383#01909944-3f4b-4b31-9bc5-128f223a586e

and the others are failing due to an older version of bazel being used and incompatible visibility on some targets in @bazel_tools. It might be tricky to work around these but I'll see what workarounds can be put into rules_android.

@shs96c
Copy link
Collaborator

shs96c commented Jul 10, 2024

We do need the CI tests to pass before we can land this. We're fine asking for the latest versions of Bazel 5 and 6, but our version 7 support is more flexible.

@ahumesky ahumesky changed the title Update rules_jvm_external to use the Starlark version of aar_import Update rules_jvm_external to use the Starlark version of aar_import after the native version was removed from Bazel Jul 10, 2024
@ahumesky
Copy link
Contributor Author

We do need the CI tests to pass before we can land this.

Right I wouldn't want to submit something that breaks CI :)

We're fine asking for the latest versions of Bazel 5 and 6, but our version 7 support is more flexible.

After going through most of the initial issues, it looks like the starlark android rules won't work with bazel 5 because at minimum they use visibility() for bzl visibility, which was introduced in bazel 6:

https://github.com/search?q=repo%3Abazelbuild%2Frules_android+%22visibility%28%22&type=code

I might be able to get the rules to work with bazel 6, but I'm not sure how long we can guarantee that (really we've only been developing for bazel 8 and backporting some things as needed to bazel 7).

Maybe I can arrange this so that the native android rules are used with the tests that use bazel 5 and bazel 6, and the starlark rules with latest bazel (8+). That might just amount to removing the use_starlark_android_rules = True and letting it auto select the starlark rules on latest bazel where native aar_import is deleted. There would still be problems with loading and registering the starlark android rules in the WORKSPACE file (e.g. visibility() above) -- but blzmod is used by default in bazel 7+, so I could perhaps just remove the starlark android rules from the WORKSPACE and leave them in MODULE.bazel.

@ahumesky
Copy link
Contributor Author

ahumesky commented Jul 11, 2024

This stubbed out action is failing on windows:
https://github.com/bazelbuild/rules_android/blob/main/src/validations/aar_import_checks/BUILD

So that will have to be another update in rules_android

But it looks like everything else is working: native android rules on bazel 5 / 6, starlark android rules on 7+

copybara-service bot pushed a commit to bazelbuild/rules_android that referenced this pull request Jul 11, 2024
Ran into this while getting rules_jvm_export and the rules_android working together: bazel-contrib/rules_jvm_external#1149 (comment)

PiperOrigin-RevId: 651557752
Change-Id: I30bbe6792863b7d5ed178f0d074a3bf73eb631db
@ahumesky
Copy link
Contributor Author

ahumesky commented Jul 12, 2024

Hm, well bazelbuild/rules_android@746589b should have fixed the windows tests, but looks like the ValidateAAR action is still failing... I'll have to try something else but out of time today. Really that action does nothing (it's just a stub action that actually does nothing) so I might just try to disable it

@ahumesky
Copy link
Contributor Author

ahumesky commented Aug 1, 2024

With bazelbuild/rules_android@f4b6edb all the tests are now passing

@ahumesky ahumesky marked this pull request as ready for review August 1, 2024 22:24
@ahumesky
Copy link
Contributor Author

ahumesky commented Aug 1, 2024

I updated the description and marked this ready for review

MODULE.bazel Outdated Show resolved Hide resolved
private/rules/coursier.bzl Outdated Show resolved Hide resolved
private/rules/maven_install.bzl Outdated Show resolved Hide resolved
tests/integration/override_targets/Loader.java Outdated Show resolved Hide resolved
@jin jin merged commit ba7310c into bazel-contrib:master Aug 7, 2024
9 checks passed
@ahumesky
Copy link
Contributor Author

ahumesky commented Aug 7, 2024

Thank you!

shs96c added a commit to shs96c/rules_jvm_external that referenced this pull request Aug 12, 2024
…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.
shs96c added a commit that referenced this pull request Aug 12, 2024
…import after the native version was removed from Bazel (#1149)" (#1215)

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.
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.

[Bazel CI] Build failed with errors: no native function or rule 'aar_import'
3 participants