diff options
Diffstat (limited to 'src')
3 files changed, 223 insertions, 2 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index 42c935e817..3f018d37eb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java @@ -33,13 +33,19 @@ import com.google.devtools.build.lib.analysis.config.DefaultsPackage; import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.graph.Digraph; import com.google.devtools.build.lib.graph.Node; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.NativeAspectClass; +import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; +import com.google.devtools.build.lib.packages.OutputFile; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClassProvider; +import com.google.devtools.build.lib.packages.RuleErrorConsumer; +import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory; import com.google.devtools.build.lib.rules.SkylarkModules; import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy; @@ -47,6 +53,7 @@ import com.google.devtools.build.lib.syntax.Environment; import com.google.devtools.build.lib.syntax.Environment.Extension; import com.google.devtools.build.lib.syntax.Environment.Phase; import com.google.devtools.build.lib.syntax.Mutability; +import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.common.options.OptionsClassProvider; import java.lang.reflect.Constructor; @@ -83,6 +90,90 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { RuleContext.Builder contextBuilder, ConfiguredTarget prerequisite, Attribute attribute); } + /** Validator to check for and warn on the deprecation of dependencies. */ + public static final class DeprecationValidator implements PrerequisiteValidator { + /** Checks if the given prerequisite is deprecated and prints a warning if so. */ + @Override + public void validate( + RuleContext.Builder contextBuilder, ConfiguredTarget prerequisite, Attribute attribute) { + validateDirectPrerequisiteForDeprecation( + contextBuilder, contextBuilder.getRule(), prerequisite); + } + + /** + * Returns whether two packages are considered the same for purposes of deprecation warnings. + * Dependencies within the same package do not print deprecation warnings; a package in the + * javatests directory may also depend on its corresponding java package without a warning. + */ + public static boolean isSameLogicalPackage( + PackageIdentifier thisPackage, PackageIdentifier prerequisitePackage) { + if (thisPackage.equals(prerequisitePackage)) { + // If the packages are equal, they are the same logical package (and just the same package). + return true; + } + if (!thisPackage.getRepository().equals(prerequisitePackage.getRepository())) { + // If the packages are in different repositories, they are not the same logical package. + return false; + } + // If the packages are in the same repository, it's allowed iff this package is the javatests + // companion to the prerequisite java package. + String thisPackagePath = thisPackage.getPackageFragment().getPathString(); + String prerequisitePackagePath = prerequisitePackage.getPackageFragment().getPathString(); + return thisPackagePath.startsWith("javatests/") + && prerequisitePackagePath.startsWith("java/") + && thisPackagePath.substring("javatests/".length()).equals( + prerequisitePackagePath.substring("java/".length())); + } + + /** Returns whether a deprecation warning should be printed for the prerequisite described. */ + private static boolean shouldEmitDeprecationWarningFor( + String thisDeprecation, PackageIdentifier thisPackage, + String prerequisiteDeprecation, PackageIdentifier prerequisitePackage) { + // Don't report deprecation edges from javatests to java or within a package; + // otherwise tests of deprecated code generate nuisance warnings. + // Don't report deprecation if the current target is also deprecated. + return (prerequisiteDeprecation != null + && !isSameLogicalPackage(thisPackage, prerequisitePackage) + && thisDeprecation == null); + } + + /** Checks if the given prerequisite is deprecated and prints a warning if so. */ + public static void validateDirectPrerequisiteForDeprecation( + RuleErrorConsumer errors, Rule rule, ConfiguredTarget prerequisite) { + Target prerequisiteTarget = prerequisite.getTarget(); + Label prerequisiteLabel = prerequisiteTarget.getLabel(); + PackageIdentifier thatPackage = prerequisiteLabel.getPackageIdentifier(); + PackageIdentifier thisPackage = rule.getLabel().getPackageIdentifier(); + + if (prerequisiteTarget instanceof Rule) { + Rule prerequisiteRule = (Rule) prerequisiteTarget; + String thisDeprecation = + NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING); + String thatDeprecation = + NonconfigurableAttributeMapper.of(prerequisiteRule).get("deprecation", Type.STRING); + if (shouldEmitDeprecationWarningFor( + thisDeprecation, thisPackage, thatDeprecation, thatPackage)) { + errors.ruleWarning("target '" + rule.getLabel() + "' depends on deprecated target '" + + prerequisiteLabel + "': " + thatDeprecation); + } + } + + if (prerequisiteTarget instanceof OutputFile) { + Rule generatingRule = ((OutputFile) prerequisiteTarget).getGeneratingRule(); + String thisDeprecation = + NonconfigurableAttributeMapper.of(rule).get("deprecation", Type.STRING); + String thatDeprecation = + NonconfigurableAttributeMapper.of(generatingRule).get("deprecation", Type.STRING); + if (shouldEmitDeprecationWarningFor( + thisDeprecation, thisPackage, thatDeprecation, thatPackage)) { + errors.ruleWarning("target '" + rule.getLabel() + "' depends on the output file " + + prerequisiteLabel + " of a deprecated rule " + generatingRule.getLabel() + + "': " + thatDeprecation); + } + } + } + } + /** * Builder for {@link ConfiguredRuleClassProvider}. */ diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 34cfdaa34e..90b064a929 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -20,6 +20,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Functions; import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.DeprecationValidator; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.PrerequisiteValidator; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.RuleContext; @@ -181,6 +182,8 @@ public class BazelRuleClassProvider { public void validate(RuleContext.Builder context, ConfiguredTarget prerequisite, Attribute attribute) { validateDirectPrerequisiteVisibility(context, prerequisite, attribute.getName()); + DeprecationValidator.validateDirectPrerequisiteForDeprecation( + context, context.getRule(), prerequisite); } private void validateDirectPrerequisiteVisibility( @@ -193,8 +196,6 @@ public class BazelRuleClassProvider { } Target prerequisiteTarget = prerequisite.getTarget(); Label prerequisiteLabel = prerequisiteTarget.getLabel(); - // We don't check the visibility of late-bound attributes, because it would break some - // features. if (!context.getRule().getLabel().getPackageIdentifier().equals( prerequisite.getTarget().getLabel().getPackageIdentifier()) && !context.isVisible(prerequisite)) { diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DeprecationValidatorTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DeprecationValidatorTest.java new file mode 100644 index 0000000000..053af1ec6c --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/analysis/DeprecationValidatorTest.java @@ -0,0 +1,129 @@ +// 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.analysis; + +import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for deprecation warnings in Bazel. */ +@RunWith(JUnit4.class) +public final class DeprecationValidatorTest extends BuildViewTestCase { + + @Test + public void noDeprecationWarningForTopLevelTarget() throws Exception { + scratchConfiguredTarget( + "a", + "a", + "filegroup(name='a', deprecation='ignored because this target is on the top level')"); + assertNoEvents(); + } + + @Test + public void noDeprecationWarningWithinPackage() throws Exception { + scratchConfiguredTarget( + "a", + "a", + "filegroup(name='a', srcs=[':b'])", + "filegroup(name='b', deprecation='ignored because depending target is in same package')"); + assertNoEvents(); + } + + @Test + public void noDeprecationWarningForDeprecatedTarget() throws Exception { + scratchConfiguredTarget( + "b", + "b", + "filegroup(name='b', deprecation='ignored because depending target is deprecated')"); + scratchConfiguredTarget( + "a", + "a", + "filegroup(name='a', srcs=['//b:b'], deprecation='ignored for a top level target')"); + assertNoEvents(); + } + + @Test + public void noDeprecationWarningForJavatestsCompanionOfJavaPackage() throws Exception { + scratchConfiguredTarget( + "java/a", + "b", + "filegroup(name='b', deprecation='ignored because depending target is javatests match')"); + scratchConfiguredTarget("javatests/a", "a", "filegroup(name='a', srcs=['//java/a:b'])"); + assertNoEvents(); + } + + @Test + public void deprecationWarningForJavaCompanionOfJavatestsPackage() throws Exception { + scratchConfiguredTarget( + "javatests/a", + "b", + "filegroup(name='b', deprecation='deprecation warning printed', testonly=0)"); + checkWarning( + "java/a", + "a", + "target '//java/a:a' depends on deprecated target '//javatests/a:b': " + + "deprecation warning printed", + "filegroup(name='a', srcs=['//javatests/a:b'])"); + } + + @Test + public void deprecationWarningForDifferentPackage() throws Exception { + scratchConfiguredTarget( + "b", "b", "filegroup(name='b', deprecation='deprecation warning printed')"); + checkWarning( + "a", + "a", + "target '//a:a' depends on deprecated target '//b:b': deprecation warning printed", + "filegroup(name='a', srcs=['//b:b'])"); + } + + @Test + public void deprecationWarningForSamePackageInDifferentRepository() throws Exception { + try (OutputStream output = scratch.resolve("WORKSPACE").getOutputStream(true /* append */)) { + output.write( + "\nlocal_repository(name = 'r', path = '/r')\n".getBytes(StandardCharsets.UTF_8)); + } + scratch.file("/r/WORKSPACE", "workspace(name = 'r')"); + scratch.file("/r/a/BUILD", "filegroup(name='b', deprecation='deprecation warning printed')"); + invalidatePackages(); + checkWarning( + "a", + "a", + "target '//a:a' depends on deprecated target '@r//a:b': deprecation warning printed", + "filegroup(name='a', srcs=['@r//a:b'])"); + } + + @Test + public void deprecationWarningForJavatestsCompanionOfJavaPackageInDifferentRepository() + throws Exception { + try (OutputStream output = scratch.resolve("WORKSPACE").getOutputStream(true /* append */)) { + output.write( + "\nlocal_repository(name = 'r', path = '/r')\n".getBytes(StandardCharsets.UTF_8)); + } + scratch.file("/r/WORKSPACE", "workspace(name = 'r')"); + scratch.file( + "/r/java/a/BUILD", "filegroup(name='b', deprecation='deprecation warning printed')"); + invalidatePackages(); + checkWarning( + "javatests/a", + "a", + "target '//javatests/a:a' depends on deprecated target '@r//java/a:b': " + + "deprecation warning printed", + "filegroup(name='a', srcs=['@r//java/a:b'])"); + } +} |