Add --experimental_java_classpath=bazel_no_fallback option

[rsalvador]

This change introduces a new option, --experimental_java_classpath=bazel_no_fallback, which operates similarly to --experimental_java_classpath=bazel but does not fall back to the full transitive classpath. Instead, the build fails if javac cannot compile with the reduced classpath.

Fixes #24875

[rsalvador]

test_build_hello_world_reduced_classpath_no_fallback in bazel_java_test.sh fails with:

java.lang.IllegalArgumentException: No enum constant com.google.devtools.build.buildjar.OptionsParser.ReduceClasspathMode.BAZEL_REDUCED_NO_FALLBACK
	at java.base/java.lang.Enum.valueOf(Enum.java:293)
	at com.google.devtools.build.buildjar.OptionsParser$ReduceClasspathMode.valueOf(OptionsParser.java:55)

does this mean that the change would need to be back-compatible with versions of java_tools that don't include the src/java_tools changes in this PR?.

[cushon]

I left some notes in the bug

[cushon]

Instead of adding the new --reduce_classpath_mode=BAZEL_REDUCED_NO_FALLBACK mode, did you consider having Bazel invoke JavaBuilder with a reduced classpath and pass --reduce_classpath_mode=NONE, and then handle not doing fallback on the Bazel side? I think that would provide equivalent behaviour without JavaBuilder changes.

[rsalvador]

Thx, that's much better. Pushed the change.

There is still 1 test failure, but seems unrelated to the change:

test_size_less_than_385MB FAILED: terminated because this command returned a non-zero status:
/private/var/tmp/_bazel_buildkite/1198ee8566c303641c6d358da6949c16/sandbox/darwin-sandbox/4799/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/src/test/shell/integration/minimal_jdk_test.runfiles/_main/src/test/shell/integration/minimal_jdk_test:50: in call to test_size_less_than_385MB

[dkashyn-sfdc]

@cushon could you please merge this?

[hvadehra]

Would it be possible to add a test for the scenario where the build fails when it otherwise would succeeded with fallback?

[rsalvador]

Would it be possible to add a test for the scenario where the build fails when it otherwise would succeeded with fallback?

Setting up a sample project that requires fallback to succeed would be challenging since fallback only occurs in rare compiler edge cases. Is there an existing Java test project that relies on fallback and that I could use?. The test for --experimental_java_classpath=bazel uses a sample project that doesn't require fallback:

function test_build_hello_world_reduced_classpath() {
  write_hello_library_files

  bazel build --experimental_java_classpath=bazel //java/main:main &> $TEST_log || fail "build failed"
}

If a test is necessary, I can look into creating one.

[fmeum]

bazel/src/test/shell/integration/config_stripped_outputs_test.sh

Line 144 in 2683cf0

function write_java_classpath_reduction_files() {

creates such a project

[rsalvador]

Would it be possible to add a test for the scenario where the build fails when it otherwise would succeeded with fallback?

@hvadehra added the test, the window's failures seem unrelated to my change

[hvadehra]

Thanks for adding the test!

the window's failures seem unrelated to my change

But the failures are all in the newly added test_build_reduced_classpath_no_fallback? (eg: https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/01954c95-250f-418c-a45c-7f496a64fda4/src%5Ctest%5Cshell%5Cbazel%5Cbazel_java_test_jdk11_toolchain_head%5Cshard_2_of_2%5Ctest.log)

[rsalvador]

But the failures are all in the newly added test_build_reduced_classpath_no_fallback? (eg: https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/01954c95-250f-418c-a45c-7f496a64fda4/src%5Ctest%5Cshell%5Cbazel%5Cbazel_java_test_jdk11_toolchain_head%5Cshard_2_of_2%5Ctest.log)

Oh sorry, I got confused looking only at the main log. The test failure is because on windows the reduced classpath compilation fails with:

test_build_reduced_classpath_no_fallback\java\hello\A.java:3: error: cannot access C
  public void f(B b) { b.getC().getD(); }

instead of with:

test_build_reduced_classpath_no_fallback\java\hello\A.java:3: error: cannot access D
  public void f(B b) { b.getC().getD(); }

this seems to imply that the reduced classpath calculation in windows may be incorrect in general, e.g. that for windows --experimental_java_classpath=bazel fallsback when it doesn't need to, but that would be unrelated of this PR??. I don't have a windows machine and can't test that.

[fmeum]

@rsalvador Great find! Looks like this line is problematic on Windows:

https://cs.opensource.google/bazel/bazel/+/master:src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java;l=346;drc=15a3526621559170c2a28e9dae5e9d7c72a82f02;bpv=1;bpt=1

[rsalvador]

Tried submitting a windows fix, but still fails with the same problem. I don't have a windows machine and can't debug this.

[fmeum]

Could you try this patch? Some tests should still fail as they need a new rules_java / java_tools release.

---a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java
+++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java
@@ -46,6 +46,7 @@ import com.sun.tools.javac.util.Log;
 import com.sun.tools.javac.util.Log.WriterKind;
 import com.sun.tools.javac.util.Name;
 import com.sun.tools.javac.util.Names;
+import java.io.File;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.UncheckedIOException;
@@ -337,13 +338,11 @@ public final class StrictJavaDepsPlugin extends BlazeJavaCompilerPlugin {
         // Also update the dependency proto
         Dependency dep =
             Dependency.newBuilder()
-                // Path.toString uses the platform separator (`\` on Windows) which may not
-                // match the format in params files (which currently always use `/`, see
-                // bazelbuild/bazel#4108). JavaBuilder should always parse Path strings into
-                // java.nio.file.Paths before comparing them.
+                // Path.toString uses the platform separator (`\` on Windows), but the proto must
+                // use the same separator as in the arguments.
                 //
                 // An empty path is OK in the cases we produce it. See readJarOwnerFromManifest.
-                .setPath(jar.pathOrEmpty().toString())
+                .setPath(jar.pathOrEmpty().toString().replace(File.separatorChar, '/'))
                 .setKind(Dependency.Kind.EXPLICIT)
                 .build();
         directDependenciesMap.put(jar.pathOrEmpty(), dep);

[rsalvador]

Could you try this patch? Some tests should still fail as they need a new rules_java / java_tools release.

It is failing with the same errors.

[fmeum]

I ran this with some debug prints on a separate branch (#25435) and got this output:

direct: []
transitive: []
reduced: []
direct: [bazel-out/x64_windows-fastbuild/bin/test_build_reduced_classpath_no_fallback/java/hello/libd-hjar.jar]
transitive: [File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libd-hjar.jar]
reduced: [File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libd-hjar.jar]
direct: [bazel-out\x64_windows-fastbuild\bin\test_build_reduced_classpath_no_fallback\java\hello\libd-hjar.jar, bazel-out/x64_windows-fastbuild/bin/test_build_reduced_classpath_no_fallback/java/hello/libc-hjar.jar]
transitive: [File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libc-hjar.jar, File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libd-hjar.jar]
reduced: [File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libc-hjar.jar]
direct: [bazel-out/x64_windows-fastbuild/bin/test_build_reduced_classpath_no_fallback/java/hello/libb-hjar.jar, bazel-out\x64_windows-fastbuild\bin\test_build_reduced_classpath_no_fallback\java\hello\libc-hjar.jar]
transitive: [File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libb-hjar.jar, File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libc-hjar.jar, File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libd-hjar.jar]
reduced: [File:[[<execution_root>]bazel-out/x64_windows-fastbuild/bin]test_build_reduced_classpath_no_fallback/java/hello/libb-hjar.jar]

Some paths correctly use forward slashes, others don't. I wonder whether that is because of this line in Turbine.

[fmeum]

google/turbine#363

[cushon]

re: the removed 'JavaBuilder should always parse Path strings into java.nio.file.Paths before comparing them' logic

To check my understanding, is the issue we were seeing here that it wasn't doing that, or that the logic in Bazel that doing reduced classpaths would have needed to do that but wasn't?

Either way, normalizing the jdeps to be consistent and to match the format in params files, instead of having to work around it downstream, seems like a good thing to do.

[fmeum]

To check my understanding, is the issue we were seeing here that it wasn't doing that, or that the logic in Bazel that doing reduced classpaths would have needed to do that but wasn't?

The comment only referred to the reduced classpath logic in JavaBuilder, which does parse the string into a Path. Bazel itself doesn't, but it's also not able to do so easily as this would require going by the execution OS of the action rather than the OS of the host that runs Bazel. At that point normalizing the jdeps file seems simpler.

[fmeum]

@rsalvador I sent #24876 with the Windows fixes. Once it has been merged, your new test should pass after you rebase and add this snippet to the top:

  if [[ "${JAVA_TOOLS_ZIP}" == released ]]; then
      # TODO: Enable test after the next java_tools release.
      return 0
  fi

After the next rules_java update in Bazel, you could then send a follow-up PR to drop these lines.

[fmeum]

@bazel-io fork 8.2.0