From 14d9dec821852ab033fccf0caba7441c6ce3bc15 Mon Sep 17 00:00:00 2001 From: Christopher Peterson Sauer Date: Fri, 4 Mar 2022 18:58:54 -0800 Subject: [PATCH] Add toggle for ijar in java_import Problem: java_import has been unusably broken for years for JARs with Kotlin interfaces, since ijar strips out important information. This has caused multiple dependent projects (includng official Bazel ones) to abandon java_import in favor of rolling their own versions, which themselves contain issues that are getting fixed in java_import. Fragmentation is bad, and fragmentation of bugs and fixes is worse. For more, see https://github.com/bazelbuild/rules_jvm_external/blob/master/private/rules/jvm_import.bzl https://github.com/bazelbuild/bazel/issues/4549 https://github.com/bazelbuild/bazel/pull/14966 https://github.com/bazelbuild/rules_jvm_external/issues/672 Temporary solution: Until such time as ijar is fixed for Kotlin, this adds a toggle that enables/disables ijar on java_import. This should unblock use of java_import for libraries that might contain Kotlin, so implementations can reunify. It also restores java_import to a state where it works correctly by default. Per the user manual, ijars are a build performance optimization to allow caching of actions that use JARs whose implementations change frequenly [1]. Imported (externally compiled) JARs shouldn't be changing very often, meaning that the build performance cost of disabling ijars for these prebuilt JARs should be relatively low. Therefore, the ijar toggle is off by default, so the build is correct by default. But ijar is still made available though the toggle, just in case someone is importing a Java-interface-only JAR that they change all the time. [1] https://docs.bazel.build/versions/main/user-manual.html#flag--use_ijars --- .../build/lib/rules/java/JavaImport.java | 4 +++- .../lib/rules/java/JavaImportBaseRule.java | 7 +++++++ .../java/JavaImportConfiguredTargetTest.java | 20 +++++++++++++------ 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java index bfcc7b186fa6b1..9c4e92fd2d769c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImport.java @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider.JavaOutput; +import com.google.devtools.build.lib.packages.Type; import java.util.LinkedHashSet; import java.util.Set; @@ -202,7 +203,8 @@ private ImmutableList processWithIjarIfNeeded( RuleContext ruleContext, ImmutableMap.Builder compilationToRuntimeJarMap) { ImmutableList.Builder interfaceJarsBuilder = ImmutableList.builder(); - boolean useIjar = ruleContext.getFragment(JavaConfiguration.class).getUseIjars(); + boolean useIjar = ruleContext.getFragment(JavaConfiguration.class).getUseIjars() + && ruleContext.attributes().get("use_ijar", Type.BOOLEAN); for (Artifact jar : jars) { Artifact interfaceJar = useIjar diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImportBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImportBaseRule.java index 3193cbda6de6fd..e0d3cfd704ff4b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaImportBaseRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaImportBaseRule.java @@ -54,6 +54,13 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi for IDE plug-ins or tools.jar for anything running on a standard JDK. */ + .add(attr("use_ijar", BOOLEAN).value(false)) + /* + If you import an oft-changing JAR that doesn't have a Kotlin interface, + you can enable this as a caching optimization. + Temporarily available until ijar is updated to properly handle Kotlin. + For more, see https://github.com/bazelbuild/bazel/issues/4549. + */ .add(attr("neverlink", BOOLEAN).value(false)) /* Extra constraints imposed on this rule as a Java library. diff --git a/src/test/java/com/google/devtools/build/lib/view/java/JavaImportConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/view/java/JavaImportConfiguredTargetTest.java index f33783e8640c84..53588f66580a0d 100644 --- a/src/test/java/com/google/devtools/build/lib/view/java/JavaImportConfiguredTargetTest.java +++ b/src/test/java/com/google/devtools/build/lib/view/java/JavaImportConfiguredTargetTest.java @@ -55,10 +55,12 @@ public final void writeBuildFile() throws Exception { scratch.file( "java/jarlib/BUILD", "java_import(name = 'libraryjar',", - " jars = ['library.jar'])", + " jars = ['library.jar'],", + " use_ijar = True)", "java_import(name = 'libraryjar_with_srcjar',", " jars = ['library.jar'],", - " srcjar = 'library.srcjar')"); + " srcjar = 'library.srcjar',", + " use_ijar = True)"); } @Test @@ -130,11 +132,14 @@ public void testDeps() throws Exception { " jars = ['import.jar'],", " deps = ['//java/jarlib2:depjar'],", " exports = ['//java/jarlib2:exportjar'],", + " use_ijar = True", ")", "java_import(name = 'depjar',", - " jars = ['depjar.jar'])", + " jars = ['depjar.jar'],", + " use_ijar = True)", "java_import(name = 'exportjar',", - " jars = ['exportjar.jar'])"); + " jars = ['exportjar.jar'],", + " use_ijar = True)"); ConfiguredTarget importJar = getConfiguredTarget("//java/jarlib2:import-jar"); @@ -210,7 +215,8 @@ public void testFromGenrule() throws Exception { "java_import(name = 'library-jar',", " jars = [':generated_jar'],", " srcjar = ':generated_src_jar',", - " exports = ['//java/jarlib:libraryjar'])"); + " exports = ['//java/jarlib:libraryjar'],", + " use_ijar = True)"); ConfiguredTarget jarLib = getConfiguredTarget("//java/genrules:library-jar"); JavaCompilationArgsProvider compilationArgs = @@ -441,7 +447,8 @@ public void testTransitiveDependencies() throws Exception { " deps = ['//java/jarlib:libraryjar'])", "java_import(name = 'library2-jar',", " jars = ['library2.jar'],", - " exports = [':lib'])", + " exports = [':lib'],", + " use_ijar = True)", "java_library(name = 'javalib2',", " srcs = ['Other.java'],", " deps = [':library2-jar'])"); @@ -464,6 +471,7 @@ public void testRuntimeDepsAreNotOnClasspath() throws Exception { " name = 'import_dep',", " jars = ['import_compile.jar'],", " runtime_deps = ['import_runtime.jar'],", + " use_ijar = True", ")", "java_library(", " name = 'library_dep',",