From 816cdba2a57647ab86fda14740aacadbfb957ccf Mon Sep 17 00:00:00 2001 From: Dan Sanduleac Date: Thu, 17 Oct 2019 12:13:10 +0100 Subject: [PATCH 1/6] First pass --- .../BaselineErrorProneExtension.java | 36 +++++++++++++++++++ .../baseline/plugins/BaselineErrorProne.java | 2 +- 2 files changed, 37 insertions(+), 1 deletion(-) 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..187db4cf9 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 @@ -17,10 +17,18 @@ package com.palantir.baseline.extensions; import com.google.common.collect.ImmutableList; +import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.annotation.CheckReturnValue; import org.gradle.api.Project; +import org.gradle.api.file.FileCollection; +import org.gradle.api.logging.Logger; +import org.gradle.api.logging.Logging; import org.gradle.api.provider.ListProperty; public class BaselineErrorProneExtension { + private static final Logger log = Logging.getLogger(BaselineErrorProneExtension.class); private static final ImmutableList DEFAULT_PATCH_CHECKS = ImmutableList.of( // Baseline checks @@ -52,4 +60,32 @@ public BaselineErrorProneExtension(Project project) { public final ListProperty getPatchChecks() { return patchChecks; } + + /** + * Filters down the patch checks, removing checks that depend on certain libraries being present on the compile + * class path. + */ + public final List getFilteredPatchChecks(FileCollection compileClasspath) { + boolean hasSafeLogging = !compileClasspath.filter(file -> file.getName().startsWith("safe-logging-")).isEmpty(); + boolean hasPreconditions = + // The real 'preconditions' brings in 'safe-logging'. Because of inaccurate jar name checks, we + // use that fact to ensure we're not picking up another jar named 'preconditions' by mistake. + hasSafeLogging + && !compileClasspath.filter(file -> file.getName().startsWith("preconditions-")).isEmpty(); + Stream checksStream = patchChecks.get().stream(); + if (!hasPreconditions) { + checksStream = disableCheck(compileClasspath, checksStream, "PreferSafeLoggingPreconditions"); + } + if (!hasSafeLogging) { + checksStream = disableCheck(compileClasspath, checksStream, "PreferSafeLoggableExceptions"); + } + return checksStream.collect(Collectors.toList()); + } + + @CheckReturnValue + private static Stream disableCheck( + FileCollection compileClasspath, Stream checksStream, String checkName) { + log.info("Disabling check " + checkName + " as library missing from source set for {}", compileClasspath); + return checksStream.filter(check -> !check.equals(checkName)); + } } 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..8108bb841 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 @@ -242,7 +242,7 @@ private static Stream getNotDisabledErrorproneChecks( BaselineErrorProneExtension errorProneExtension, JavaCompile javaCompile, ErrorProneOptions errorProneOptions) { - return errorProneExtension.getPatchChecks().get().stream().filter(check -> { + return errorProneExtension.getFilteredPatchChecks(javaCompile.getClasspath()).stream().filter(check -> { if (checkExplicitlyDisabled(errorProneOptions, check)) { log.info( "Task {}: not applying errorprone check {} because it has severity OFF in errorProneOptions", From eebdd074e635846843812702d3b8cc1e6e031cb6 Mon Sep 17 00:00:00 2001 From: Dan Sanduleac Date: Thu, 17 Oct 2019 13:47:11 +0100 Subject: [PATCH 2/6] simplify, move logic into plugin --- .../BaselineErrorProneExtension.java | 37 ------------------- .../baseline/plugins/BaselineErrorProne.java | 35 +++++++++++++++++- 2 files changed, 33 insertions(+), 39 deletions(-) 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 187db4cf9..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 @@ -17,19 +17,10 @@ package com.palantir.baseline.extensions; import com.google.common.collect.ImmutableList; -import java.util.List; -import java.util.stream.Collectors; -import java.util.stream.Stream; -import javax.annotation.CheckReturnValue; import org.gradle.api.Project; -import org.gradle.api.file.FileCollection; -import org.gradle.api.logging.Logger; -import org.gradle.api.logging.Logging; import org.gradle.api.provider.ListProperty; public class BaselineErrorProneExtension { - private static final Logger log = Logging.getLogger(BaselineErrorProneExtension.class); - private static final ImmutableList DEFAULT_PATCH_CHECKS = ImmutableList.of( // Baseline checks "ExecutorSubmitRunnableFutureIgnored", @@ -60,32 +51,4 @@ public BaselineErrorProneExtension(Project project) { public final ListProperty getPatchChecks() { return patchChecks; } - - /** - * Filters down the patch checks, removing checks that depend on certain libraries being present on the compile - * class path. - */ - public final List getFilteredPatchChecks(FileCollection compileClasspath) { - boolean hasSafeLogging = !compileClasspath.filter(file -> file.getName().startsWith("safe-logging-")).isEmpty(); - boolean hasPreconditions = - // The real 'preconditions' brings in 'safe-logging'. Because of inaccurate jar name checks, we - // use that fact to ensure we're not picking up another jar named 'preconditions' by mistake. - hasSafeLogging - && !compileClasspath.filter(file -> file.getName().startsWith("preconditions-")).isEmpty(); - Stream checksStream = patchChecks.get().stream(); - if (!hasPreconditions) { - checksStream = disableCheck(compileClasspath, checksStream, "PreferSafeLoggingPreconditions"); - } - if (!hasSafeLogging) { - checksStream = disableCheck(compileClasspath, checksStream, "PreferSafeLoggableExceptions"); - } - return checksStream.collect(Collectors.toList()); - } - - @CheckReturnValue - private static Stream disableCheck( - FileCollection compileClasspath, Stream checksStream, String checkName) { - log.info("Disabling check " + checkName + " as library missing from source set for {}", compileClasspath); - return checksStream.filter(check -> !check.equals(checkName)); - } } 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 8108bb841..7f920464e 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 @@ -27,6 +27,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; @@ -242,7 +243,10 @@ private static Stream getNotDisabledErrorproneChecks( BaselineErrorProneExtension errorProneExtension, JavaCompile javaCompile, ErrorProneOptions errorProneOptions) { - return errorProneExtension.getFilteredPatchChecks(javaCompile.getClasspath()).stream().filter(check -> { + + Predicate filterOutPreconditions = filterOutPreconditions(javaCompile.getClasspath()); + + return errorProneExtension.getPatchChecks().get().stream().filter(check -> { if (checkExplicitlyDisabled(errorProneOptions, check)) { log.info( "Task {}: not applying errorprone check {} because it has severity OFF in errorProneOptions", @@ -250,10 +254,37 @@ private static Stream getNotDisabledErrorproneChecks( check); return false; } - return true; + return filterOutPreconditions.test(check); }); } + /** Filters out preconditions checks if the required libraries are not on the classpath. */ + public static Predicate filterOutPreconditions(FileCollection compileClasspath) { + boolean hasSafeLogging = !compileClasspath.filter(file -> file.getName().startsWith("safe-logging-")).isEmpty(); + boolean hasPreconditions = + // The real 'preconditions' brings in 'safe-logging'. Because of inaccurate jar name checks, we + // use that fact to ensure we're not picking up another jar named 'preconditions' by mistake. + hasSafeLogging + && !compileClasspath.filter(file -> file.getName().startsWith("preconditions-")).isEmpty(); + return check -> { + if (!hasPreconditions && check.equals("PreferSafeLoggingPreconditions")) { + log.info( + "Disabling check PreferSafeLoggingPreconditions as 'com.palantir.safe-logging:safe-logging' " + + "missing from source set for {}", + compileClasspath); + return false; + } + if (!hasSafeLogging && check.equals("PreferSafeLoggableExceptions")) { + log.info( + "Disabling check PreferSafeLoggableExceptions as 'com.palantir.safe-logging:preconditions' " + + "missing from source set for {}", + compileClasspath); + return false; + } + return true; + }; + } + private static boolean isRefactoring(Project project) { return isRefasterRefactoring(project) || isErrorProneRefactoring(project); } From c3b0e97bbbf5504ad8813e62dc31992af19ec598 Mon Sep 17 00:00:00 2001 From: Dan Sanduleac Date: Thu, 17 Oct 2019 12:47:11 +0000 Subject: [PATCH 3/6] Add generated changelog entries --- changelog/@unreleased/pr-981.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) 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..d00d8634b --- /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 libraries (`com.palantir.safe-logging:{preconditions,safe-logging}`). + links: + - https://github.com/palantir/gradle-baseline/pull/981 From c9efbf702705d14fcbcf9029d54663e4b5e68608 Mon Sep 17 00:00:00 2001 From: Dan Sanduleac Date: Thu, 17 Oct 2019 15:00:09 +0100 Subject: [PATCH 4/6] use real configuration to look for dependency --- .../baseline/plugins/BaselineErrorProne.java | 75 +++++++++++++------ 1 file changed, 53 insertions(+), 22 deletions(-) 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 7f920464e..22709a582 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; @@ -37,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; @@ -224,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"); @@ -240,11 +253,16 @@ private static void configureErrorProneOptions( } private static Stream getNotDisabledErrorproneChecks( + Project project, BaselineErrorProneExtension errorProneExtension, JavaCompile javaCompile, + Optional maybeSourceSet, ErrorProneOptions errorProneOptions) { - - Predicate filterOutPreconditions = filterOutPreconditions(javaCompile.getClasspath()); + // 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( + ss, project.getConfigurations().getByName(ss.getCompileClasspathConfigurationName()))) + .orElse(check -> true); return errorProneExtension.getPatchChecks().get().stream().filter(check -> { if (checkExplicitlyDisabled(errorProneOptions, check)) { @@ -258,33 +276,46 @@ private static Stream getNotDisabledErrorproneChecks( }); } + 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(FileCollection compileClasspath) { - boolean hasSafeLogging = !compileClasspath.filter(file -> file.getName().startsWith("safe-logging-")).isEmpty(); + public static Predicate filterOutPreconditions(SourceSet sourceSet, Configuration compileClasspath) { boolean hasPreconditions = - // The real 'preconditions' brings in 'safe-logging'. Because of inaccurate jar name checks, we - // use that fact to ensure we're not picking up another jar named 'preconditions' by mistake. - hasSafeLogging - && !compileClasspath.filter(file -> file.getName().startsWith("preconditions-")).isEmpty(); + hasDependenciesMatching(compileClasspath, BaselineErrorProne::isSafeLoggingPreconditionsDep); + return check -> { - if (!hasPreconditions && check.equals("PreferSafeLoggingPreconditions")) { - log.info( - "Disabling check PreferSafeLoggingPreconditions as 'com.palantir.safe-logging:safe-logging' " - + "missing from source set for {}", - compileClasspath); - return false; - } - if (!hasSafeLogging && check.equals("PreferSafeLoggableExceptions")) { - log.info( - "Disabling check PreferSafeLoggableExceptions as 'com.palantir.safe-logging:preconditions' " - + "missing from source set for {}", - compileClasspath); - return false; + if (!hasPreconditions) { + if (check.equals("PreferSafeLoggingPreconditions")) { + log.info( + "Disabling check PreferSafeLoggingPreconditions as 'com.palantir.safe-logging:safe-logging'" + + " missing from {}", + sourceSet); + return false; + } + if (check.equals("PreferSafeLoggableExceptions")) { + log.info( + "Disabling check PreferSafeLoggableExceptions as 'com.palantir.safe-logging:preconditions' " + + "missing from {}", + sourceSet); + 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); } From 0ba8bfc5eeb4fcb15424c4936d6dde91aa461bef Mon Sep 17 00:00:00 2001 From: Dan Sanduleac Date: Thu, 17 Oct 2019 15:09:03 +0100 Subject: [PATCH 5/6] simplify and print configuration in log, as it includes project path --- .../baseline/plugins/BaselineErrorProne.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 22709a582..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 @@ -261,7 +261,7 @@ private static Stream getNotDisabledErrorproneChecks( // 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( - ss, project.getConfigurations().getByName(ss.getCompileClasspathConfigurationName()))) + project.getConfigurations().getByName(ss.getCompileClasspathConfigurationName()))) .orElse(check -> true); return errorProneExtension.getPatchChecks().get().stream().filter(check -> { @@ -287,7 +287,7 @@ private static boolean hasDependenciesMatching(Configuration configuration, Spec } /** Filters out preconditions checks if the required libraries are not on the classpath. */ - public static Predicate filterOutPreconditions(SourceSet sourceSet, Configuration compileClasspath) { + public static Predicate filterOutPreconditions(Configuration compileClasspath) { boolean hasPreconditions = hasDependenciesMatching(compileClasspath, BaselineErrorProne::isSafeLoggingPreconditionsDep); @@ -295,16 +295,16 @@ public static Predicate filterOutPreconditions(SourceSet sourceSet, Conf if (!hasPreconditions) { if (check.equals("PreferSafeLoggingPreconditions")) { log.info( - "Disabling check PreferSafeLoggingPreconditions as 'com.palantir.safe-logging:safe-logging'" - + " missing from {}", - sourceSet); + "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 {}", - sourceSet); + compileClasspath); return false; } } From f3ceb6d2055e9507613533da9d64117f438d0dbf Mon Sep 17 00:00:00 2001 From: Dan Sanduleac Date: Thu, 17 Oct 2019 14:09:03 +0000 Subject: [PATCH 6/6] Add generated changelog entries --- changelog/@unreleased/pr-981.v2.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/@unreleased/pr-981.v2.yml b/changelog/@unreleased/pr-981.v2.yml index d00d8634b..01d2bf53d 100644 --- a/changelog/@unreleased/pr-981.v2.yml +++ b/changelog/@unreleased/pr-981.v2.yml @@ -1,6 +1,6 @@ type: improvement improvement: description: Stop migrating source sets to safe-logging, unless they already have - the requisite libraries (`com.palantir.safe-logging:{preconditions,safe-logging}`). + the requisite library (`com.palantir.safe-logging:preconditions`). links: - https://github.com/palantir/gradle-baseline/pull/981