java_proto_library strict deps: add attributes for gradual migration Control strict-deps through a rule-level and a package-level attribute, allowing finer-grained migration in conjunction with a global flag. RELNOTES: java_proto_library: control strict-deps through a rule-level and a package-level attribute. -- MOS_MIGRATED_REVID=128542363
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoLibraryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoLibraryRule.java index bf822f4..217db48 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoLibraryRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/java/proto/BazelJavaProtoLibraryRule.java
@@ -17,6 +17,7 @@ import static com.google.devtools.build.lib.packages.Aspect.INJECTING_RULE_KIND_PARAMETER_KEY; import static com.google.devtools.build.lib.packages.Attribute.attr; import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST; +import static com.google.devtools.build.lib.syntax.Type.BOOLEAN; import com.google.common.base.Function; import com.google.devtools.build.lib.analysis.BaseRuleClasses; @@ -25,7 +26,7 @@ import com.google.devtools.build.lib.packages.AspectParameters; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; - +import com.google.devtools.build.lib.rules.java.JavaConfiguration; import javax.annotation.Nullable; /** Declaration of the {@code java_proto_library} rule. */ @@ -53,6 +54,7 @@ return builder // This rule isn't ready for use yet. .setUndocumented() + .requiresConfigurationFragments(JavaConfiguration.class) /* <!-- #BLAZE_RULE(java_proto_library).ATTRIBUTE(deps) --> The list of <a href="protocol-buffer.html#proto_library"><code>proto_library</code></a> rules to generate Java code for. @@ -62,6 +64,7 @@ .allowedRuleClasses("proto_library") .allowedFileTypes() .aspect(javaProtoAspect, ASPECT_PARAMETERS)) + .add(attr("strict_deps", BOOLEAN).undocumented("for migration")) .build(); }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 537866e..3a01bc3 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -38,7 +38,6 @@ import com.google.devtools.build.lib.vfs.Canonicalizer; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; - import java.io.PrintStream; import java.util.ArrayList; import java.util.Collection; @@ -47,7 +46,6 @@ import java.util.List; import java.util.Map; import java.util.Set; - import javax.annotation.Nullable; /** @@ -141,10 +139,10 @@ */ private String defaultHdrsCheck; - /** - * Default copts for cc_* rules. The rules' individual copts will append to - * this value. - */ + /** See getter. */ + private TriState defaultStrictDepsJavaProtos = TriState.AUTO; + + /** Default copts for cc_* rules. The rules' individual copts will append to this value. */ private ImmutableList<String> defaultCopts; /** @@ -590,8 +588,15 @@ } /** - * Gets the default header checking mode. + * Default for 'strict_deps' of Java proto rules, when they aren't explicitly specified. + * + * <p>A value of AUTO is returned when the package didn't itself explicitly specify this value. */ + public TriState getDefaultStrictDepsJavaProtos() { + return defaultStrictDepsJavaProtos; + } + + /** Gets the default header checking mode. */ public String getDefaultHdrsCheck() { return defaultHdrsCheck != null ? defaultHdrsCheck : "strict"; } @@ -907,9 +912,12 @@ return this; } - /** - * Sets the default value of copts. Rule-level copts will append to this. - */ + public Builder setDefaultStrictDepsJavaProtos(TriState value) { + pkg.defaultStrictDepsJavaProtos = value; + return this; + } + + /** Sets the default value of copts. Rule-level copts will append to this. */ public Builder setDefaultCopts(List<String> defaultCopts) { this.defaultCopts = defaultCopts; return this;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index 9a575ed..e53ca57 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -68,7 +68,6 @@ import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.UnixGlob; - import java.io.IOException; import java.util.ArrayList; import java.util.Collection; @@ -81,7 +80,6 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; - import javax.annotation.Nullable; /** @@ -264,6 +262,21 @@ } } + /** + * Declares the package() attribute specifying the default value for + * java_proto_library.strict_deps. + */ + private static class DefaultStrictDepsJavaProtos extends PackageArgument<Boolean> { + private DefaultStrictDepsJavaProtos() { + super("default_strict_deps_java_protos", Type.BOOLEAN); + } + + @Override + protected void process(Package.Builder pkgBuilder, Location location, Boolean value) { + pkgBuilder.setDefaultStrictDepsJavaProtos(value ? TriState.YES : TriState.NO); + } + } + public static final String PKG_CONTEXT = "$pkg_context"; // Used outside of Bazel! @@ -457,14 +470,15 @@ private ImmutableMap<String, PackageArgument<?>> createPackageArguments() { ImmutableList.Builder<PackageArgument<?>> arguments = ImmutableList.<PackageArgument<?>>builder() - .add(new DefaultDeprecation()) - .add(new DefaultDistribs()) - .add(new DefaultLicenses()) - .add(new DefaultTestOnly()) - .add(new DefaultVisibility()) - .add(new Features()) - .add(new DefaultCompatibleWith()) - .add(new DefaultRestrictedTo()); + .add(new DefaultDeprecation()) + .add(new DefaultStrictDepsJavaProtos()) + .add(new DefaultDistribs()) + .add(new DefaultLicenses()) + .add(new DefaultTestOnly()) + .add(new DefaultVisibility()) + .add(new Features()) + .add(new DefaultCompatibleWith()) + .add(new DefaultRestrictedTo()); for (EnvironmentExtension extension : environmentExtensions) { arguments.addAll(extension.getPackageArguments());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java index 1879231..c7eec04 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
@@ -30,9 +30,7 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.common.options.TriState; - import java.util.List; - import javax.annotation.Nullable; /** A java compiler configuration containing the flags required for compilation. */ @@ -131,7 +129,7 @@ private final boolean useHeaderCompilation; private final boolean optimizeHeaderCompilationAnnotationProcessing; private final boolean generateJavaDeps; - private final boolean javaProtoLibraryDepsAreStrict; + private final boolean strictDepsJavaProtos; private final JavaClasspathMode javaClasspath; private final ImmutableList<String> javaWarns; private final ImmutableList<String> defaultJvmFlags; @@ -173,7 +171,7 @@ this.javaToolchain = javaToolchain; this.javaOptimizationMode = javaOptions.javaOptimizationMode; this.legacyBazelJavaTest = javaOptions.legacyBazelJavaTest; - this.javaProtoLibraryDepsAreStrict = javaOptions.javaProtoLibraryDepsAreStrict; + this.strictDepsJavaProtos = javaOptions.strictDepsJavaProtos; ImmutableList.Builder<Label> translationsBuilder = ImmutableList.builder(); for (String s : javaOptions.translationTargets) { @@ -339,8 +337,7 @@ return legacyBazelJavaTest; } - // TODO(b/29867858): Replace with a BUILD-level attribute. - public boolean javaProtoLibraryDepsAreStrict() { - return javaProtoLibraryDepsAreStrict; + public boolean strictDepsJavaProtos() { + return strictDepsJavaProtos; } }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java index d5306c2..d337bbe 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaLibraryHelper.java
@@ -25,7 +25,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.rules.java.JavaConfiguration.JavaClasspathMode; import com.google.devtools.build.lib.util.Preconditions; - import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -112,9 +111,17 @@ } /** - * Sets the mode that determines how strictly dependencies are checked. + * When in strict mode, compiling the source-jars passed to this JavaLibraryHelper will break if + * they depend on classes not in any of the {@link + * JavaCompilationArgsProvider#javaCompilationArgs} passed in {@link #addDep}, even if they do + * appear in {@link JavaCompilationArgsProvider#recursiveJavaCompilationArgs}. That is, depending + * on a class requires a direct dependency on it. + * + * <p>Contrast this with the strictness-parameter to {@link #buildCompilationArgsProvider}, which + * controls whether others depending on the result of this compilation, can perform strict-deps + * checks at all. */ - public JavaLibraryHelper setStrictDepsMode(StrictDepsMode strictDepsMode) { + public JavaLibraryHelper setCompilationStrictDepsMode(StrictDepsMode strictDepsMode) { this.strictDepsMode = strictDepsMode; return this; } @@ -155,18 +162,25 @@ /** * Returns a JavaCompilationArgsProvider that fully encapsulates this compilation, based on the - * result of a call to build(). - * (that is, it contains the compile-time and runtime jars, separated by direct vs transitive - * jars). + * result of a call to build(). (that is, it contains the compile-time and runtime jars, separated + * by direct vs transitive jars). + * + * @param isReportedAsStrict if true, the result's direct JavaCompilationArgs only contain classes + * resulting from compiling the source-jars. If false, the direct JavaCompilationArgs contain + * both these classes, as well as any classes from transitive dependencies. A value of 'false' + * means this compilation cannot be checked for strict-deps, by any consumer (depending) + * compilation. Contrast this with {@link #setCompilationStrictDepsMode}. */ - public JavaCompilationArgsProvider buildCompilationArgsProvider(JavaCompilationArgs directArgs) { - JavaCompilationArgs transitiveArgs = JavaCompilationArgs.builder() - .addTransitiveArgs(directArgs, BOTH) - .addTransitiveDependencies(deps, true /* recursive */) + public JavaCompilationArgsProvider buildCompilationArgsProvider( + JavaCompilationArgs directArgs, boolean isReportedAsStrict) { + JavaCompilationArgs transitiveArgs = + JavaCompilationArgs.builder() + .addTransitiveArgs(directArgs, BOTH) + .addTransitiveDependencies(deps, true /* recursive */) .build(); return new JavaCompilationArgsProvider( - isStrict() ? directArgs : transitiveArgs, transitiveArgs); + isReportedAsStrict ? directArgs : transitiveArgs, transitiveArgs); } private void addDepsToAttributes(JavaTargetAttributes.Builder attributes) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java index ea17d1e..65049fe 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
@@ -351,17 +351,17 @@ public boolean legacyBazelJavaTest; @Option( - name = "java_proto_library_deps_are_strict", - defaultValue = "false", + name = "strict_deps_java_protos", + defaultValue = "true", category = "undocumented", help = - "This only applies to java_proto_library. " - + "If true: (1) if a Java file uses proto Foo, it must depend on a " - + "java_{lite,...}_proto_library that directly depends on a proto_library that has Foo " - + "in its srcs. (2) strict-deps violations are reported for the proto_library rules " - + "themselves." + "When 'strict-deps' is on, .java files that depend on classes not declared in their rule's " + + "'deps' fail to build. In other words, it's forbidden to depend on classes obtained " + + "transitively. This flag controls the behavior of Java proto rules when their " + + "'strict_deps' attribute is unspecified, and its containing package doesn't specify " + + "'default_strict_deps_java_protos'." ) - public boolean javaProtoLibraryDepsAreStrict; + public boolean strictDepsJavaProtos; @Override public FragmentOptions getHost(boolean fallback) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java index 5760507..54ccb03 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java
@@ -174,7 +174,6 @@ private final RuleContext ruleContext; private final SupportData supportData; - private final boolean isStrictDeps; private final String protoRuntimeAttr; private final JavaSemantics javaSemantics; @@ -197,9 +196,6 @@ this.protoCompilerPluginOptions = protoCompilerPluginOptions; this.javaSemantics = javaSemantics; - isStrictDeps = - ruleContext.getFragment(JavaConfiguration.class).javaProtoLibraryDepsAreStrict(); - dependencyCompilationArgs = JavaCompilationArgsProvider.merge( Iterables.<JavaCompilationArgsAspectProvider, JavaCompilationArgsProvider>transform( @@ -312,13 +308,14 @@ .setOutput(outputJar) .addSourceJars(sourceJar) .setJavacOpts(constructJavacOpts()); - helper.addDep(dependencyCompilationArgs); helper + .addDep(dependencyCompilationArgs) .addDep( ruleContext.getPrerequisite( protoRuntimeAttr, Mode.TARGET, JavaCompilationArgsProvider.class)) - .setStrictDepsMode(isStrictDeps ? StrictDepsMode.WARN : StrictDepsMode.OFF); - return helper.buildCompilationArgsProvider(helper.build(javaSemantics)); + .setCompilationStrictDepsMode(StrictDepsMode.OFF); + return helper.buildCompilationArgsProvider( + helper.build(javaSemantics), true /* isReportedAsStrict */); } private Artifact getSourceJarArtifact() {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoLibrary.java index 4ca16ae..cddb929 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoLibrary.java
@@ -50,6 +50,10 @@ ruleContext, JavaCompilationArgsAspectProvider.class), JavaCompilationArgsAspectProvider.GET_PROVIDER)); + if (!StrictDepsUtils.isStrictDepsJavaProtoLibrary(ruleContext)) { + dependencyArgsProviders = StrictDepsUtils.makeNonStrict(dependencyArgsProviders); + } + Runfiles runfiles = new Runfiles.Builder(ruleContext.getWorkspaceName()) .addArtifacts(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/StrictDepsUtils.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/StrictDepsUtils.java new file mode 100644 index 0000000..d184539 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/StrictDepsUtils.java
@@ -0,0 +1,68 @@ +// Copyright 2016 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.rules.java.proto; + +import static com.google.devtools.build.lib.rules.java.JavaCompilationArgs.ClasspathType.BOTH; + +import com.google.devtools.build.lib.analysis.RuleContext; +import com.google.devtools.build.lib.packages.AttributeContainer; +import com.google.devtools.build.lib.packages.TriState; +import com.google.devtools.build.lib.rules.java.JavaCompilationArgs; +import com.google.devtools.build.lib.rules.java.JavaCompilationArgsProvider; +import com.google.devtools.build.lib.rules.java.JavaConfiguration; + +public class StrictDepsUtils { + + /** + * Returns true iff 'ruleContext' should enforce strict-deps. + * + * <ol> + * <li>If the rule explicitly specifies the 'strict_deps' attribute, returns its value. + * <li>Otherwise, if the package explicitly specifies 'default_strict_deps_java_proto_library', + * returns that value. + * <li>Otherwise, returns the value of the --strict_deps_java_proto_library flag. + * </ol> + * + * Using this method requires requesting the JavaConfiguration fragment. + */ + public static boolean isStrictDepsJavaProtoLibrary(RuleContext ruleContext) { + AttributeContainer attributeContainer = ruleContext.getRule().getAttributeContainer(); + if (attributeContainer.isAttributeValueExplicitlySpecified("strict_deps")) { + return (boolean) attributeContainer.getAttr("strict_deps"); + } + TriState defaultJavaProtoLibraryStrictDeps = + ruleContext.getRule().getPackage().getDefaultStrictDepsJavaProtos(); + if (defaultJavaProtoLibraryStrictDeps == TriState.AUTO) { + return ruleContext.getFragment(JavaConfiguration.class).strictDepsJavaProtos(); + } + return defaultJavaProtoLibraryStrictDeps == TriState.YES; + } + + /** + * Returns a new JavaCompilationArgsProvider whose direct-jars part is the union of both the + * direct and indirect jars of 'provider'. + */ + public static JavaCompilationArgsProvider makeNonStrict(JavaCompilationArgsProvider provider) { + JavaCompilationArgs.Builder directCompilationArgs = JavaCompilationArgs.builder(); + directCompilationArgs + .addTransitiveArgs(provider.getJavaCompilationArgs(), BOTH) + .addTransitiveArgs(provider.getRecursiveJavaCompilationArgs(), BOTH); + return new JavaCompilationArgsProvider( + directCompilationArgs.build(), + provider.getRecursiveJavaCompilationArgs(), + provider.getCompileTimeJavaDependencyArtifacts(), + provider.getRunTimeJavaDependencyArtifacts()); + } +}