-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for pushing this through! The failing tests look legitimate. |
Thanks for taking a look, this draft is to run the CI to see what fails |
I have the failing tests down to:
|
…kotlin:inline_function_test with bazel 8.0.0
The |
There is not enough motivation to flip And bumping rules_kotlin version fixes the test failure that comes with removing the sibling repository layout flag:
|
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 ( Unfortunately updating rules_kotlin to 2.1.0 seems to cause a different (yet still path related) failure on windows:
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 |
Ah, but 2.0.0 still has the |
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: Maybe that code was not being used until bazelbuild/rules_kotlin@e51619c, not sure It seems that the java launcher separates these by semicolon: However changing the Starlark code to use semicolon still results in
And the code does appear to add these files to the inputs: |
I also noticed that rules_kotlin added these recently to its
but that didn't seem to help the problem |
@ahumesky what's the status of this and active blockers to move off rules_android 0.1.1? |
Thanks I should be able to pick this back up this week. |
…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.
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 |
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 |
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. |
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
I'm not entirely sure why these versions are getting mixed, since things for rules_android should be coming from rules_android |
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 |
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
Updated the PR to temporarily use a specific commit of rules_android to test protobuf 29.3 ... and windows tests are failing again... |
…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
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 |
Well I found the problem causing the windows tests to fail:
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. |
…ows. Discovered via bazel-contrib/rules_jvm_external#1297 PiperOrigin-RevId: 732146668 Change-Id: Ib109574191e26643b9ae092e5e3514cc2bcc5858
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) |
Reverts #1215 plus accounting for additional changes since ba7310c went in