Skip to content

Commit

Permalink
Restrict migration to safe-logging to source sets that have the libra…
Browse files Browse the repository at this point in the history
…ry (#981)

Stop migrating source sets to safe-logging, unless they already have the requisite library (`com.palantir.safe-logging:preconditions`).
  • Loading branch information
dansanduleac authored and bulldozer-bot[bot] committed Oct 17, 2019
1 parent 1485a20 commit c2d7979
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 3 deletions.
6 changes: 6 additions & 0 deletions changelog/@unreleased/pr-981.v2.yml
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.gradle.api.provider.ListProperty;

public class BaselineErrorProneExtension {

private static final ImmutableList<String> DEFAULT_PATCH_CHECKS = ImmutableList.of(
// Baseline checks
"ExecutorSubmitRunnableFutureIgnored",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -223,13 +230,20 @@ private static void configureErrorProneOptions(
}

if (isErrorProneRefactoring(project)) {
Optional<SourceSet> 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<String> errorProneChecks = getNotDisabledErrorproneChecks(
errorProneExtension, javaCompile, errorProneOptions);
project, errorProneExtension, javaCompile, maybeSourceSet, errorProneOptions);
return ImmutableList.of(
"-XepPatchChecks:" + Joiner.on(',').join(errorProneChecks.iterator()),
"-XepPatchLocation:IN_PLACE");
Expand All @@ -239,9 +253,17 @@ private static void configureErrorProneOptions(
}

private static Stream<String> getNotDisabledErrorproneChecks(
Project project,
BaselineErrorProneExtension errorProneExtension,
JavaCompile javaCompile,
Optional<SourceSet> maybeSourceSet,
ErrorProneOptions errorProneOptions) {
// If this javaCompile is associated with a source set, use it to figure out if it has preconditions or not.
Predicate<String> 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(
Expand All @@ -250,10 +272,50 @@ private static Stream<String> getNotDisabledErrorproneChecks(
check);
return false;
}
return true;
return filterOutPreconditions.test(check);
});
}

private static boolean hasDependenciesMatching(Configuration configuration, Spec<ModuleComponentIdentifier> 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<String> 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);
}
Expand Down

0 comments on commit c2d7979

Please sign in to comment.