From 1d49184b071b281c83939416d7eead671729c8b5 Mon Sep 17 00:00:00 2001 From: cushon Date: Wed, 25 Apr 2018 09:49:17 -0700 Subject: Make java_common.compile's javacopt handling consistent with native Java rules Compilations performed by java_common.compile now use the javacopts in the java_toolchain by default, instead of requiring them to be explicitly retrived using java_common.default_javac_opts, for consistency with the native rules. java_common.compile(javac_opts=...) can still be used to pass additional javacopts. RELNOTES: Make java_common.compile now uses java_toolchain javacopts by default; explicitly retrieving them using java_common.default_javac_opts is unnecessary. PiperOrigin-RevId: 194254098 --- .../bazel/rules/android/BazelAndroidSemantics.java | 4 +- .../lib/bazel/rules/java/BazelJavaSemantics.java | 6 ++- .../build/lib/rules/android/AndroidCommon.java | 8 ++-- .../build/lib/rules/android/AndroidSemantics.java | 6 +-- .../build/lib/rules/android/DataBinding.java | 2 +- .../devtools/build/lib/rules/java/JavaCommon.java | 56 +++++++++++----------- .../build/lib/rules/java/JavaInfoBuildHelper.java | 15 +++++- .../build/lib/rules/java/JavaSemantics.java | 8 +++- .../build/lib/rules/java/JavaSkylarkCommon.java | 5 +- .../rules/java/proto/JavaProtoSkylarkCommon.java | 9 ++-- .../build/lib/rules/java/proto/ProtoJavacOpts.java | 11 ++++- 11 files changed, 76 insertions(+), 54 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java index 4d957a9e19..cf7ba3c48e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/BazelAndroidSemantics.java @@ -43,13 +43,11 @@ public class BazelAndroidSemantics implements AndroidSemantics { } @Override - public ImmutableList getJavacArguments(RuleContext ruleContext) { + public ImmutableList getCompatibleJavacOptions(RuleContext ruleContext) { ImmutableList.Builder javacArgs = new ImmutableList.Builder<>(); - if (!ruleContext.getFragment(AndroidConfiguration.class).desugarJava8()) { javacArgs.add("-source", "7", "-target", "7"); } - return javacArgs.build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java index 4efc58365b..ecb65b4b7a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/BazelJavaSemantics.java @@ -62,6 +62,7 @@ import com.google.devtools.build.lib.rules.java.JavaRuleOutputJarsProvider; import com.google.devtools.build.lib.rules.java.JavaSemantics; import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider; import com.google.devtools.build.lib.rules.java.JavaTargetAttributes; +import com.google.devtools.build.lib.rules.java.JavaToolchainProvider; import com.google.devtools.build.lib.rules.java.JavaUtil; import com.google.devtools.build.lib.rules.java.proto.GeneratedExtensionRegistryProvider; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -553,8 +554,9 @@ public class BazelJavaSemantics implements JavaSemantics { } @Override - public Iterable getExtraJavacOpts(RuleContext ruleContext) { - return ImmutableList.of(); + public ImmutableList getCompatibleJavacOptions( + RuleContext ruleContext, JavaToolchainProvider toolchain) { + return ImmutableList.of(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java index 7240e132c3..a89910839d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidCommon.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.rules.android; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.analysis.AnalysisUtils; @@ -466,13 +465,14 @@ public class AndroidCommon { bootclasspath = ImmutableList.of(AndroidSdkProvider.fromRuleContext(ruleContext).getAndroidJar()); } - Iterable javacopts = androidSemantics.getJavacArguments(ruleContext); + ImmutableList.Builder javacopts = ImmutableList.builder(); + javacopts.addAll(androidSemantics.getCompatibleJavacOptions(ruleContext)); if (DataBinding.isEnabled(ruleContext)) { - javacopts = Iterables.concat(javacopts, DataBinding.getJavacopts(ruleContext, isBinary)); + javacopts.addAll(DataBinding.getJavacOpts(ruleContext, isBinary)); } JavaTargetAttributes.Builder attributes = javaCommon - .initCommon(idlHelper.getIdlGeneratedJavaSources(), javacopts) + .initCommon(idlHelper.getIdlGeneratedJavaSources(), javacopts.build()) .setBootClassPath(bootclasspath); if (DataBinding.isEnabled(ruleContext)) { DataBinding.addAnnotationProcessor(ruleContext, attributes); diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java index 87130b7351..3726808cb5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidSemantics.java @@ -69,10 +69,10 @@ public interface AndroidSemantics { * Returns the command line options to be used when compiling Java code for {@code android_*} * rules. * - *

These will come after the default options specified by the toolchain and the ones in the - * {@code javacopts} attribute. + *

These will come after the default options specified by the toolchain, and before the ones in + * the {@code javacopts} attribute. */ - ImmutableList getJavacArguments(RuleContext ruleContext); + ImmutableList getCompatibleJavacOptions(RuleContext ruleContext); /** * Configures the builder for generating the output jar used to configure the main dex file. diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java b/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java index 338494eb4f..9f6ac28ea6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/DataBinding.java @@ -169,7 +169,7 @@ public final class DataBinding { } /** The javac flags that are needed to configure data binding's annotation processor. */ - static ImmutableList getJavacopts(RuleContext ruleContext, boolean isBinary) { + static ImmutableList getJavacOpts(RuleContext ruleContext, boolean isBinary) { ImmutableList.Builder flags = ImmutableList.builder(); String metadataOutputDir = getDataBindingExecPath(ruleContext).getPathString(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java index 2545d99b90..26ba4441ea 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java @@ -61,7 +61,6 @@ import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; -import java.util.stream.Stream; import javax.annotation.Nullable; /** @@ -108,6 +107,7 @@ public class JavaCommon { private final RuleContext ruleContext; private final JavaSemantics semantics; + private final JavaToolchainProvider javaToolchain; private JavaCompilationHelper javaCompilationHelper; public JavaCommon(RuleContext ruleContext, JavaSemantics semantics) { @@ -144,6 +144,7 @@ public class JavaCommon { ImmutableList runtimeDeps, ImmutableList bothDeps) { this.ruleContext = ruleContext; + this.javaToolchain = JavaToolchainProvider.from(ruleContext); this.semantics = semantics; this.sources = sources; this.targetsTreatedAsDeps = ImmutableMap.of( @@ -451,39 +452,34 @@ public class JavaCommon { public final void initializeJavacOpts() { Preconditions.checkState(javacOpts == null); - javacOpts = computeJavacOpts(semantics.getExtraJavacOpts(ruleContext)); + javacOpts = computeJavacOpts(getCompatibleJavacOptions()); } - /** - * For backwards compatibility, this method allows multiple calls to set the Javac opts. Do not - * use this. - */ - @Deprecated - public final void initializeJavacOpts(Iterable extraJavacOpts) { - javacOpts = computeJavacOpts(extraJavacOpts); + /** Computes javacopts for the current rule. */ + private ImmutableList computeJavacOpts(Collection extraRuleJavacOpts) { + return ImmutableList.builder() + .addAll(computeToolchainJavacOpts(ruleContext, javaToolchain)) + .addAll(extraRuleJavacOpts) + .addAll(ruleContext.getExpander().withDataLocations().tokenized("javacopts")) + .build(); } - private ImmutableList computeJavacOpts(Iterable extraJavacOpts) { + /** + * Returns the toolchain javacopts for the current rule, including global defaults as well as any + * per-package configuration. + */ + public static ImmutableList computeToolchainJavacOpts( + RuleContext ruleContext, JavaToolchainProvider toolchain) { return Streams.concat( - toolchainJavacOpts(ruleContext), - Streams.stream(extraJavacOpts), - ruleContext.getExpander().withDataLocations().tokenized("javacopts").stream()) + toolchain.getJavacOptions().stream(), + toolchain + .packageConfiguration() + .stream() + .filter(p -> p.matches(ruleContext.getLabel())) + .flatMap(p -> p.javacopts().stream())) .collect(toImmutableList()); } - private Stream toolchainJavacOpts(RuleContext ruleContext) { - JavaToolchainProvider toolchain = JavaToolchainProvider.from(ruleContext); - return Stream.concat( - toolchain.getJavacOptions().stream(), - // Enable any javacopts from java_toolchain.packages that are configured for the current - // package. - toolchain - .packageConfiguration() - .stream() - .filter(p -> p.matches(ruleContext.getLabel())) - .flatMap(p -> p.javacopts().stream())); - } - public static PathFragment getHostJavaExecutable(RuleContext ruleContext) { return JavaRuntimeInfo.forHost(ruleContext).javaBinaryExecPath(); } @@ -600,7 +596,11 @@ public class JavaCommon { } public JavaTargetAttributes.Builder initCommon() { - return initCommon(ImmutableList.of(), semantics.getExtraJavacOpts(ruleContext)); + return initCommon(ImmutableList.of(), getCompatibleJavacOptions()); + } + + private ImmutableList getCompatibleJavacOptions() { + return semantics.getCompatibleJavacOptions(ruleContext, javaToolchain); } /** @@ -614,7 +614,7 @@ public class JavaCommon { public JavaTargetAttributes.Builder initCommon( Collection extraSrcs, Iterable extraJavacOpts) { Preconditions.checkState(javacOpts == null); - javacOpts = computeJavacOpts(extraJavacOpts); + javacOpts = computeJavacOpts(ImmutableList.copyOf(extraJavacOpts)); activePlugins = collectPlugins(); JavaTargetAttributes.Builder javaTargetAttributes = new JavaTargetAttributes.Builder(semantics); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java index faede121b7..9c98907be2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaInfoBuildHelper.java @@ -451,6 +451,8 @@ final class JavaInfoBuildHelper { throw new EvalException(null, "'host_javabase' must point to a Java runtime"); } + JavaToolchainProvider toolchainProvider = getJavaToolchainProvider(javaToolchain); + JavaLibraryHelper helper = new JavaLibraryHelper(skylarkRuleContext.getRuleContext()) .setOutput(outputJar) @@ -458,7 +460,16 @@ final class JavaInfoBuildHelper { .addSourceFiles(sourceFiles) .addResources(resources) .setSourcePathEntries(sourcepathEntries) - .setJavacOpts(javacOpts); + .setJavacOpts( + ImmutableList.builder() + .addAll( + JavaCommon.computeToolchainJavacOpts( + skylarkRuleContext.getRuleContext(), toolchainProvider)) + .addAll( + javaSemantics.getCompatibleJavacOptions( + skylarkRuleContext.getRuleContext(), toolchainProvider)) + .addAll(javacOpts) + .build()); List depsCompilationArgsProviders = JavaInfo.fetchProvidersFromList(deps, JavaCompilationArgsProvider.class); @@ -486,7 +497,7 @@ final class JavaInfoBuildHelper { JavaCompilationArtifacts artifacts = helper.build( javaSemantics, - getJavaToolchainProvider(javaToolchain), + toolchainProvider, javaRuntimeInfo, SkylarkList.createImmutable(ImmutableList.of()), outputJarsBuilder, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java index 5ee66560c1..d6b7d60527 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSemantics.java @@ -364,9 +364,13 @@ public interface JavaSemantics { void addRunfilesForLibrary(RuleContext ruleContext, Runfiles.Builder runfilesBuilder); /** - * Returns the additional options to be passed to javac. + * Returns the command line options to be used when compiling Java code for {@code java_*} rules. + * + *

These will come after the default options specified by the toolchain, and before the ones in + * the {@code javacopts} attribute. */ - Iterable getExtraJavacOpts(RuleContext ruleContext); + ImmutableList getCompatibleJavacOptions( + RuleContext ruleContext, JavaToolchainProvider toolchain); /** * Add additional targets to be treated as direct dependencies. diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java index 9d46f886ab..6570c5e771 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkCommon.java @@ -524,8 +524,9 @@ public class JavaSkylarkCommon { mandatoryPositionals = 1, parameters = { @Param(name = "java_toolchain_attr", positional = false, named = true, type = String.class) - } - ) + }) + // TODO(b/78512644): migrate callers to passing explicit javacopts or using custom toolchains, and + // delete public static ImmutableList getDefaultJavacOpts( SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) throws EvalException { RuleContext ruleContext = skylarkRuleContext.getRuleContext(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java index 08482be9e2..e74d5a005f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoSkylarkCommon.java @@ -23,7 +23,6 @@ import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleContext; import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider; import com.google.devtools.build.lib.rules.java.JavaInfo; -import com.google.devtools.build.lib.rules.java.JavaSemantics; import com.google.devtools.build.lib.rules.java.JavaToolchainProvider; import com.google.devtools.build.lib.rules.proto.ProtoCompileActionBuilder; import com.google.devtools.build.lib.rules.proto.ProtoLangToolchainProvider; @@ -136,7 +135,8 @@ public class JavaProtoSkylarkCommon { @Param(name = "java_toolchain_attr", positional = false, named = true, type = String.class) } ) - // TODO(elenairina): Consider a nicer way of returning this, taking in a JavaToolchainProvider. + // TODO(b/78512644): migrate callers to passing explicit proto javacopts or using custom + // toolchains, and delete public static ImmutableList getJavacOpts( SkylarkRuleContext skylarkRuleContext, String javaToolchainAttr) throws EvalException { ConfiguredTarget javaToolchainConfigTarget = @@ -144,10 +144,7 @@ public class JavaProtoSkylarkCommon { JavaToolchainProvider toolchain = checkNotNull(JavaToolchainProvider.from(javaToolchainConfigTarget)); - return ImmutableList.builder() - .addAll(toolchain.getJavacOptions()) - .addAll(toolchain.getCompatibleJavacOptions(JavaSemantics.PROTO_JAVACOPTS_KEY)) - .build(); + return ProtoJavacOpts.constructJavacOpts(skylarkRuleContext.getRuleContext(), toolchain); } private static ProtoLangToolchainProvider getProtoToolchainProvider( diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java index 5717828c98..8aea8daa4f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/ProtoJavacOpts.java @@ -30,7 +30,16 @@ public class ProtoJavacOpts { *

See java_toolchain.compatible_javacopts for the javacopts required for protos. */ public static ImmutableList constructJavacOpts(RuleContext ruleContext) { - JavaToolchainProvider toolchain = JavaToolchainProvider.from(ruleContext); + return constructJavacOpts(ruleContext, JavaToolchainProvider.from(ruleContext)); + } + + /** + * Returns javacopts for compiling the Java source files generated by the proto compiler. + * + *

See java_toolchain.compatible_javacopts for the javacopts required for protos. + */ + public static ImmutableList constructJavacOpts( + RuleContext ruleContext, JavaToolchainProvider toolchain) { return ImmutableList.builder() .addAll(toolchain.getJavacOptions()) .addAll(toolchain.getCompatibleJavacOptions(JavaSemantics.PROTO_JAVACOPTS_KEY)) -- cgit v1.2.3