Uncomment android_sdk/ndk_repository on Windows

[rongjiecomputer]

Revive #5023 (by @philwo) so that we can try to revive #4797.

shell_commands will put the whole cmd string into an array ([cmd]) before passing it to subprocess.run. This will make subprocess.run thinks that the first element of the array is the full path of the program, hence the weird error in #5023. We work around it with batch_commands which pass the cmd string directly into subprocess.run and let subprocess.run do the heavy-lifting.

/cc @philwo @meteorcloudy

[rongjiecomputer]

I use Get-content WORKSPACE to verify that the two lines are indeed modified.

ERROR: D:/b/bk-worker-windows-java8-q650/bazel/bazel-bazel-github-presubmit/WORKSPACE:57:1: no such package '@androidsdk//': D:/tmp/dky55tp6/external/androidsdk/extras/android/m2repository (Not a directory) and referenced by '//external:android/dx_jar_import'
--
  | WARNING: D:/b/bk-worker-windows-java8-q650/bazel/bazel-bazel-github-presubmit/src/test/java/com/google/devtools/build/lib/blackbox/bazel/BUILD:24:1: in java_library rule //src/test/java/com/google/devtools/build/lib/blackbox/bazel:common_tools_deps: target '//src/test/java/com/google/devtools/build/lib/blackbox/bazel:common_tools_deps' depends on deprecated target '@bazel_tools//tools/runfiles:java-runfiles': Depend on @bazel_tools//tools/java/runfiles instead. This target will be deleted in Bazel 0.19.0
  | WARNING: errors encountered while analyzing target '//src/test/java/com/google/devtools/build/android/dexer:AllTests': it will not be built
  | INFO: Analysis succeeded for only 217 of 218 top-level targets
  | INFO: Analysed 218 targets (152 packages loaded, 13214 targets configured).
  | INFO: Found 217 test targets...
  | WARNING: D:/tmp/dky55tp6/execroot/io_bazel/src/test/java/com/google/devtools/build/android/testing/manifestmerge/mergeeOne/AndroidManifest.xml was modified during execution
  | INFO: From SkylarkAction external/googleapis/google_watch_v1_java_grpc_srcs.jar:
  | google/watcher/v1/watch.proto: warning: Import google/protobuf/empty.proto but not used.
  | INFO: From Generating Java (Immutable) proto_library @googleapis//:google_watch_v1_proto:
  | google/watcher/v1/watch.proto: warning: Import google/protobuf/empty.proto but not used.
  | ERROR: command succeeded, but there were loading phase errors

I think androidsdk repo is triggered, but D:/tmp/dky55tp6/external/androidsdk/extras/android/m2repository is not found. Might be an Android SDK installation issue in CI machine?

[laszlocsomor]

@rongjiecomputer : What's the current state of this PR?

[rongjiecomputer]

@laszlocsomor Blocked by bazelbuild/continuous-integration#386, waiting for a new BuildKite image that contains bazelbuild/continuous-integration@5d2516a

[rongjiecomputer]

Android dexer tests failed with this exception:

java.io.FileNotFoundException: D:\b\p2qmnrx5\execroot\io_bazel\bazel-out\x64_windows-fastbuild\bin\src\test\java\com\google\devtools\build\android\dexer\AllTests.exe.runfiles\io_bazel\src\test\java\com\google\devtools\build\android\dexer\libtests.jar (The system cannot find the path specified)

Without looking at the test code, my initial guess is this is due to the test not using Bazel runfiles library for Java.

[laszlocsomor]

Yes, that looks right.

testinputjar is passed here:

bazel/src/test/java/com/google/devtools/build/android/dexer/BUILD

Line 58 in 607207c

"-Dtestinputjar=$(location :tests)",

The test should use the Java runfiles library and pass System.getProperty("testinputjar")'s result to to Rlocation:

bazel/src/test/java/com/google/devtools/build/android/dexer/DexBuilderTest.java

Line 43 in 607207c

options.inputJar = WORKING_DIR.resolve(System.getProperty("testinputjar"));

[rongjiecomputer]

@laszlocsomor The Android dexer test now failed in all platforms. I think I am not using the runfiles library correctly. Can you help?

Does this path looks correctly for Linux (getting java.nio.file.NoSuchFileException)?

/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/4e446d3526a2434e779ddb26ad7508a7/sandbox/linux-sandbox/1505/execroot/io_bazel/bazel-out/k8-fastbuild/bin/src/test/java/com/google/devtools/build/android/dexer/AllTests.runfiles/src/test/java/com/google/devtools/build/android/dexer/libtests.jar

• Linux test log
• Windows test log

[laszlocsomor]

What does System.getProperty("testinputjar") return? I suspect it's missing the repository's name. You always have to prefix the rlocation argument with the repo name, e.g. io_bazel/src/bazel.exe.

[rongjiecomputer]

@laszlocsomor Prepending io_bazel/ fixed the issue (totally forgot about the prefix thing). Thanks so much!

[laszlocsomor]

Cool!

[laszlocsomor]

bazelbuild/continuous-integration#386 is resolved and you solved the rlocation issue. Is this PR ready for review?

[rongjiecomputer]

Is this PR ready for review?

Yep.

[laszlocsomor]

LGTM. Thanks for all your work!
@philwo / @buchgr / @fweikert , could one of you take a look please?

[rongjiecomputer]

Ping.

[laszlocsomor]

Actually, ping @jin and @ahumesky

[jin]

lgtm, I'll import this change!

[laszlocsomor]

@rongjiecomputer : I'm one of the internal reviewers of this commit. It all looks good, but we have to push an internal change before we can import this one. Sorry about that the delay!