From c2d7979d72f7fec041550e7421ca24a9ac27ef07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20S=C4=83nduleac?= Date: Thu, 17 Oct 2019 15:20:10 +0100 Subject: [PATCH] Restrict migration to safe-logging to source sets that have the library (#981) Stop migrating source sets to safe-logging, unless they already have the requisite library (`com.palantir.safe-logging:preconditions`). --- changelog/@unreleased/pr-981.v2.yml | 6 ++ .../BaselineErrorProneExtension.java | 1 - .../baseline/plugins/BaselineErrorProne.java | 66 ++++++++++++++++++- 3 files changed, 70 insertions(+), 3 deletions(-) create mode 100644 changelog/@unreleased/pr-981.v2.yml diff --git a/changelog/@unreleased/pr-981.v2.yml b/changelog/@unreleased/pr-981.v2.yml new file mode 100644 index 000000000..01d2bf53d --- /dev/null +++ b/changelog/@unreleased/pr-981.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Stop migrating source sets to safe-logging, unless they already have + the requisite library (`com.palantir.safe-logging:preconditions`). + links: + - https://github.com/palantir/gradle-baseline/pull/981 diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 1530dbb1b..a7957734a 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -21,7 +21,6 @@ import org.gradle.api.provider.ListProperty; public class BaselineErrorProneExtension { - private static final ImmutableList DEFAULT_PATCH_CHECKS = ImmutableList.of( // Baseline checks "ExecutorSubmitRunnableFutureIgnored", diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java index 297c52164..e786ac5bd 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java @@ -19,6 +19,8 @@ import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.common.collect.MoreCollectors; import com.palantir.baseline.extensions.BaselineErrorProneExtension; import com.palantir.baseline.tasks.CompileRefasterTask; import java.io.File; @@ -27,6 +29,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import net.ltgt.gradle.errorprone.CheckSeverity; @@ -36,12 +39,16 @@ import org.gradle.api.Plugin; import org.gradle.api.Project; import org.gradle.api.artifacts.Configuration; +import org.gradle.api.artifacts.component.ModuleComponentIdentifier; import org.gradle.api.file.FileCollection; import org.gradle.api.file.RegularFile; import org.gradle.api.logging.Logger; import org.gradle.api.logging.Logging; import org.gradle.api.plugins.ExtensionAware; +import org.gradle.api.plugins.JavaPluginConvention; import org.gradle.api.provider.Provider; +import org.gradle.api.specs.Spec; +import org.gradle.api.tasks.SourceSet; import org.gradle.api.tasks.compile.JavaCompile; import org.gradle.api.tasks.javadoc.Javadoc; import org.gradle.api.tasks.testing.Test; @@ -223,13 +230,20 @@ private static void configureErrorProneOptions( } if (isErrorProneRefactoring(project)) { + Optional maybeSourceSet = project + .getConvention() + .getPlugin(JavaPluginConvention.class) + .getSourceSets() + .matching(ss -> javaCompile.getName().equals(ss.getCompileJavaTaskName())) + .stream() + .collect(MoreCollectors.toOptional()); // TODO(gatesn): Is there a way to discover error-prone checks? // Maybe service-load from a ClassLoader configured with annotation processor path? // https://github.com/google/error-prone/pull/947 errorProneOptions.getErrorproneArgumentProviders().add(() -> { // Don't apply checks that have been explicitly disabled Stream errorProneChecks = getNotDisabledErrorproneChecks( - errorProneExtension, javaCompile, errorProneOptions); + project, errorProneExtension, javaCompile, maybeSourceSet, errorProneOptions); return ImmutableList.of( "-XepPatchChecks:" + Joiner.on(',').join(errorProneChecks.iterator()), "-XepPatchLocation:IN_PLACE"); @@ -239,9 +253,17 @@ private static void configureErrorProneOptions( } private static Stream getNotDisabledErrorproneChecks( + Project project, BaselineErrorProneExtension errorProneExtension, JavaCompile javaCompile, + Optional maybeSourceSet, ErrorProneOptions errorProneOptions) { + // If this javaCompile is associated with a source set, use it to figure out if it has preconditions or not. + Predicate filterOutPreconditions = maybeSourceSet + .map(ss -> filterOutPreconditions( + project.getConfigurations().getByName(ss.getCompileClasspathConfigurationName()))) + .orElse(check -> true); + return errorProneExtension.getPatchChecks().get().stream().filter(check -> { if (checkExplicitlyDisabled(errorProneOptions, check)) { log.info( @@ -250,10 +272,50 @@ private static Stream getNotDisabledErrorproneChecks( check); return false; } - return true; + return filterOutPreconditions.test(check); }); } + private static boolean hasDependenciesMatching(Configuration configuration, Spec spec) { + return !Iterables.isEmpty( + configuration + .getIncoming() + .artifactView(viewConfiguration -> viewConfiguration.componentFilter( + ci -> ci instanceof ModuleComponentIdentifier + && spec.isSatisfiedBy((ModuleComponentIdentifier) ci))) + .getArtifacts()); + } + + /** Filters out preconditions checks if the required libraries are not on the classpath. */ + public static Predicate filterOutPreconditions(Configuration compileClasspath) { + boolean hasPreconditions = + hasDependenciesMatching(compileClasspath, BaselineErrorProne::isSafeLoggingPreconditionsDep); + + return check -> { + if (!hasPreconditions) { + if (check.equals("PreferSafeLoggingPreconditions")) { + log.info( + "Disabling check PreferSafeLoggingPreconditions as " + + "'com.palantir.safe-logging:preconditions' missing from {}", + compileClasspath); + return false; + } + if (check.equals("PreferSafeLoggableExceptions")) { + log.info( + "Disabling check PreferSafeLoggableExceptions as 'com.palantir.safe-logging:preconditions' " + + "missing from {}", + compileClasspath); + return false; + } + } + return true; + }; + } + + private static boolean isSafeLoggingPreconditionsDep(ModuleComponentIdentifier mci) { + return mci.getGroup().equals("com.palantir.safe-logging") && mci.getModule().equals("preconditions"); + } + private static boolean isRefactoring(Project project) { return isRefasterRefactoring(project) || isErrorProneRefactoring(project); }