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 to use Starlark rules_android. Reapply ba7310ce4a1d8fb14434597fbc7440a4074f7695 #1297

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

ahumesky
Copy link
Contributor

@ahumesky ahumesky commented Dec 13, 2024

Reverts #1215 plus accounting for additional changes since ba7310c went in

@jin
Copy link
Collaborator

jin commented Dec 18, 2024

Thanks for pushing this through! The failing tests look legitimate.

@ahumesky
Copy link
Contributor Author

ahumesky commented Jan 8, 2025

Thanks for taking a look, this draft is to run the CI to see what fails

@ahumesky
Copy link
Contributor Author

ahumesky commented Jan 9, 2025

I have the failing tests down to:

…kotlin:inline_function_test with bazel 8.0.0
@ahumesky
Copy link
Contributor Author

ahumesky commented Jan 9, 2025

The //tests/unit/kotlin:inline_function_test failure with bazel 8.0.0 is due to disabling --experimental_sibling_repository_layout, and of course restoring that causes the original failures with the go rules (bazel-contrib/rules_go#3947)

@jin
Copy link
Collaborator

jin commented Jan 13, 2025

There is not enough motivation to flip --experimental_sibling_repository_layout to true so we should be ok to remove it from rules_jvm_external's bazelrc (bazelbuild/bazel#20500 (comment))

And bumping rules_kotlin version fixes the test failure that comes with removing the sibling repository layout flag:

INFO: Analyzed target //tests/unit/kotlin:inline_function_test (114 packages loaded, 6285 targets configured).
INFO: Found 1 test target...
Target //tests/unit/kotlin:inline_function_test up-to-date:
  bazel-bin/tests/unit/kotlin/inline_function_test.jar
  bazel-bin/tests/unit/kotlin/inline_function_test.jdeps
INFO: Elapsed time: 43.089s, Critical Path: 2.71s
INFO: 12 processes: 40 action cache hit, 8 internal, 2 darwin-sandbox, 2 worker.
INFO: Build completed successfully, 12 total actions
//tests/unit/kotlin:inline_function_test                                 PASSED in 0.3s

Executed 1 out of 1 test: 1 test passes.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.


jingwen@jingwen-mac rules_jvm_external % git diff
diff --git a/.bazelrc b/.bazelrc
index d137de4..6f82835 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -8,7 +8,7 @@ build --experimental_strict_java_deps=strict
 build --explicit_java_test_deps

 # Re-enable once https://github.com/bazelbuild/rules_go/issues/3947 is addressed.
-build --experimental_sibling_repository_layout
+# build --experimental_sibling_repository_layout

 # Make sure we get something helpful when tests fail
 test --verbose_failures
diff --git a/MODULE.bazel b/MODULE.bazel
index 60a0dcd..1c6a015 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -30,7 +30,7 @@ bazel_dep(
 )
 bazel_dep(
     name = "rules_kotlin",
-    version = "1.9.6",
+    version = "2.1.0",
 )
 bazel_dep(
     name = "rules_shell",

@ahumesky
Copy link
Contributor Author

ahumesky commented Jan 14, 2025

Awesome thanks for taking a look at this -- I was initially looking at rules_java since the error was about the path to the java binary (/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/sandbox/linux-sandbox/1594/execroot/_main/bazel-out/k8-fastbuild/bin/tests/unit/kotlin/inline_function_test.runfiles/_main/external/rules_java++toolchains+remotejdk11_linux/bin/java: No such file or directory)

Unfortunately updating rules_kotlin to 2.1.0 seems to cause a different (yet still path related) failure on windows:

set USE_BAZEL_VERSION=8.0.0
bazelisk build //tests/integration/kt_jvm_export:test-lib

LAUNCHER ERROR: Rlocation failed on _main/external/rules_kotlin++rules_kotlin_extensions+com_github_jetbrains_kotlin_git/lib/annotations-13.0.jar:external/rules_kotlin++rules_kotlin_extensions+com_github_jetbrains_kotlin_git/lib/kotlin-stdlib.jar:external/rules_kotlin++rules_kotlin_extensions+com_github_jetbrains_kotlin_git/lib/kotlin-stdlib-jdk7.jar:external/rules_kotlin++rules_kotlin_extensions+com_github_jetbrains_kotlin_git/lib/kotlin-stdlib-jdk8.jar, path doesn't exist in MANIFEST file

Switching to rules_kotlin 2.0.0 seems to avoid that error, so it must be one of the changes between 2.0.0 and 2.1.0: bazelbuild/rules_kotlin@v2.0.0...v2.1.0

@ahumesky
Copy link
Contributor Author

Ah, but 2.0.0 still has the No such file or directory problem with bin/java on linux: https://buildkite.com/bazel/rules-jvm-external/builds/4767#01946284-2045-4179-af17-cec4ce769182. Bummer. I'll try to find which commit between 2.0.0 and 2.1.0 breaks things on windows tomorrow

@ahumesky
Copy link
Contributor Author

ahumesky commented Jan 15, 2025

The issue with rules_kotlin appears to be from this change: bazelbuild/rules_kotlin@e51619c

Although the error is coming from code that was introduced earlier than that commit:
https://github.com/bazelbuild/rules_kotlin/blob/f20ef32db6988fa1b346b1af12f3a3edd545bc47/src/main/starlark/core/compile/cli/compile.bzl#L33-L35

Maybe that code was not being used until bazelbuild/rules_kotlin@e51619c, not sure

It seems that the java launcher separates these by semicolon:
https://github.com/bazelbuild/bazel/blob/bebe20ed5078edf123b7d528931658168df2cb4c/src/tools/launcher/java_launcher.cc#L328-L331

However changing the Starlark code to use semicolon still results in

LAUNCHER ERROR: Rlocation failed on _main/external/rules_kotlin++rules_kotlin_extensions+com_github_jetbrains_kotlin_git/lib/annotations-13.0.jar, path doesn't exist in MANIFEST file

And the code does appear to add these files to the inputs:
https://github.com/bazelbuild/rules_kotlin/blob/f20ef32db6988fa1b346b1af12f3a3edd545bc47/src/main/starlark/core/compile/cli/compile.bzl#L28

@ahumesky
Copy link
Contributor Author

I also noticed that rules_kotlin added these recently to its .bazelrc:

# Required for windows ci
startup --windows_enable_symlinks
common --enable_runfiles

https://github.com/bazelbuild/rules_kotlin/blob/f20ef32db6988fa1b346b1af12f3a3edd545bc47/.bazelrc#L12-L14

but that didn't seem to help the problem

@jin
Copy link
Collaborator

jin commented Feb 6, 2025

@ahumesky what's the status of this and active blockers to move off rules_android 0.1.1?

@ahumesky
Copy link
Contributor Author

Thanks I should be able to pick this back up this week.

ahumesky added a commit to ahumesky/rules_kotlin that referenced this pull request Feb 13, 2025
…vm_external#1297: 1) Use platform-specific path separators. 2) Add Kotlin standard library jars to the data of tools so that the Windows Java launcher can find them in the runfiles manifest.
@ahumesky
Copy link
Contributor Author

ahumesky commented Feb 13, 2025

I have some fixes to rules_kotlin that gets the Windows tests in rules_jvm_external to pass: bazelbuild/rules_kotlin#1274

Unfortunately as-is it breaks a bunch of other stuff so I'll have to see how to fix that fix...

This also means we'll need to wait for a rules_kotlin release that includes these fixes.

It's not clear to me offhand how supported Windows is in rules_kotlin, the tests running on Windows are disabled: https://github.com/bazelbuild/rules_kotlin/blob/096170ffd892817044d56e8959c5e73a5856a98d/.bazelci/presubmit.yml#L10

@ahumesky
Copy link
Contributor Author

Temporarily brought my fork of rules_kotlin into this PR (c97dd30) to test bazelbuild/rules_kotlin#1274 and it appears to be working.

The last thing to fix in this PR is to not try to run the Starlark Android rules with bazel 6.4.0

Then we'll have to wait for a rules_kotlin release with bazelbuild/rules_kotlin#1274

@ahumesky
Copy link
Contributor Author

All the tests are passing now (including those on bazel 6.4.0 which will use the native android rules) with my update to rules_kotlin (bazelbuild/rules_kotlin#1274). I'll leave this as a draft until there's a release of rules_kotlin with my PR.

@ahumesky
Copy link
Contributor Author

Pulling in the latest changes, 0b5d9da updates rules_jvm_external to protobuf 29.3, which seems to conflict with the version rules_android uses: https://github.com/bazelbuild/rules_android/blob/de5a97fa897ce244fec8468a9eb411f8d88a8e0e/defs.bzl#L70-L71

resulting in

Exception in thread "main" java.lang.ExceptionInInitializerError
	at com.google.devtools.build.android.AndroidDataSerializer.flushTo(AndroidDataSerializer.java:71)
	at com.google.devtools.build.android.AndroidResourceParsingAction.main(AndroidResourceParsingAction.java:80)
	at com.google.devtools.build.android.ResourceProcessorBusyBox$Tool$2.call(ResourceProcessorBusyBox.java:74)
	at com.google.devtools.build.android.ResourceProcessorBusyBox.processRequest(ResourceProcessorBusyBox.java:238)
	at com.google.devtools.build.android.ResourceProcessorBusyBox.main(ResourceProcessorBusyBox.java:175)
Caused by: com.google.protobuf.RuntimeVersion$ProtobufRuntimeVersionException: Detected incompatible Protobuf Gencode/Runtime versions when loading com.google.devtools.build.android.proto.SerializeFormat$Header: gencode 4.29.3, runtime 4.29.0. Runtime version cannot be older than the linked gencode version.
	at com.google.protobuf.RuntimeVersion.validateProtobufGencodeVersionImpl(RuntimeVersion.java:117)
	at com.google.protobuf.RuntimeVersion.validateProtobufGencodeVersion(RuntimeVersion.java:71)
	at com.google.devtools.build.android.proto.SerializeFormat$Header.<clinit>(SerializeFormat.java:86)
	... 5 more
Target //tests/unit/aar_import:starlark_aar_import_test failed to build

I'm not entirely sure why these versions are getting mixed, since things for rules_android should be coming from rules_android

@ahumesky
Copy link
Contributor Author

Updating rules_android to 29.3 and testing locally seems to get the tests working again, so we'll need a release of rules_android as well

copybara-service bot pushed a commit to bazelbuild/rules_android that referenced this pull request Feb 19, 2025
Matches rules_jvm_external update to 29.3: bazel-contrib/rules_jvm_external@0b5d9da

One blocker for bazel-contrib/rules_jvm_external#1297

PiperOrigin-RevId: 728683174
Change-Id: I36d9e3e87a7a63203fa9823b6d5a007489b7def1
@ahumesky
Copy link
Contributor Author

ahumesky commented Feb 19, 2025

Updated the PR to temporarily use a specific commit of rules_android to test protobuf 29.3 ... and windows tests are failing again...

restingbull pushed a commit to bazelbuild/rules_kotlin that referenced this pull request Feb 21, 2025
…ternal to use Starlark Android rules (#1274)

* Fixes for Windows issues found in the course of bazel-contrib/rules_jvm_external#1297: 1) Use platform-specific path separators. 2) Add Kotlin standard library jars to the data of tools so that the Windows Java launcher can find them in the runfiles manifest.

* Test using path instead of short_path

* Run buildifier

* Consolidate KOTLIN_STDLIBS to //kotlin/compiler:compiler.bzl
@ahumesky ahumesky changed the title Update rules_android. Reapply ba7310ce4a1d8fb14434597fbc7440a4074f7695 Update to use Starlark rules_android. Reapply ba7310ce4a1d8fb14434597fbc7440a4074f7695 Feb 26, 2025
@ahumesky
Copy link
Contributor Author

We've released rules_android 0.6.2 for aligning the protobuf versions to 29.3, and I've updated this PR to use rules_android 0.6.2

Now we just need a release of rules_kotlin with bazelbuild/rules_kotlin#1274 (@Bencodes @restingbull) and to fix the windows tests (again)

@Bencodes
Copy link
Contributor

We've released rules_android 0.6.2 for aligning the protobuf versions to 29.3, and I've updated this PR to use rules_android 0.6.2

Now we just need a release of rules_kotlin with bazelbuild/rules_kotlin#1274 (@Bencodes @restingbull) and to fix the windows tests (again)

@ahumesky I just cut a new release for you to use bazelbuild/bazel-central-registry#3893

@ahumesky
Copy link
Contributor Author

Well I found the problem causing the windows tests to fail:
https://github.com/bazelbuild/rules_android/blob/4269050ab06041b0ca0ffb491ceff021c273601f/tools/android/aar_embedded_jars_extractor.py#L103-L105

output_dir and build_target are swapped in the call to main:
https://github.com/bazelbuild/rules_android/blob/4269050ab06041b0ca0ffb491ceff021c273601f/tools/android/aar_embedded_jars_extractor.py#L78-L79

Not sure at the moment how that happened... or why it didn't break just about everything... or why it's only failing on windows.... but at least it's an easy fix. This will require another release of rules_android.

copybara-service bot pushed a commit to bazelbuild/rules_android that referenced this pull request Feb 28, 2025
…ows.

Discovered via bazel-contrib/rules_jvm_external#1297

PiperOrigin-RevId: 732146668
Change-Id: Ib109574191e26643b9ae092e5e3514cc2bcc5858
@ahumesky
Copy link
Contributor Author

Not sure at the moment why it's only failing on windows.... but at least it's an easy fix. This will require another release of rules_android.

This was broken only on windows because the error was in a windows-only codepath. That's fixed now, and we just need now the new version of rules_kotlin to be available on BCR. All the tests are (finally) passing with the new version of rules_android and rules_kotlin (temporarily pulled in through git_override in the module file)

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.

3 participants