From dbf779869751cc893ba240402d352c6e70be2978 Mon Sep 17 00:00:00 2001 From: cushon Date: Wed, 14 Mar 2018 18:53:33 -0700 Subject: Emit SJD errors even if we don't know the label of a dependency Fixes bazelbuild/bazel#4846 PiperOrigin-RevId: 189123353 --- .../google/devtools/build/buildjar/JarOwner.java | 31 +++++------- .../javac/plugins/dependency/DependencyModule.java | 14 +++--- .../plugins/dependency/StrictJavaDepsPlugin.java | 55 ++++++++++++---------- 3 files changed, 52 insertions(+), 48 deletions(-) (limited to 'src/java_tools/buildjar/java/com/google/devtools') diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JarOwner.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JarOwner.java index eeec099310..f2908a503e 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JarOwner.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/JarOwner.java @@ -15,8 +15,8 @@ package com.google.devtools.build.buildjar; import com.google.auto.value.AutoValue; -import com.google.common.base.Function; -import javax.annotation.Nullable; +import java.nio.file.Path; +import java.util.Optional; /** * Holds information about the Bazel rule that created a certain jar. @@ -27,26 +27,21 @@ import javax.annotation.Nullable; @AutoValue public abstract class JarOwner { - /** A long way to say 'JarOwner::label'. */ - public static final Function LABEL = - new Function() { - @Nullable - @Override - public String apply(@Nullable JarOwner jarOwner) { - return jarOwner == null ? null : jarOwner.label(); - } - }; + public abstract Path jar(); - public abstract String label(); + public abstract Optional label(); - @Nullable - public abstract String aspect(); + public abstract Optional aspect(); - public static JarOwner create(String label) { - return new AutoValue_JarOwner(label, null); + public static JarOwner create(Path jar) { + return new AutoValue_JarOwner(jar, Optional.empty(), Optional.empty()); } - public static JarOwner create(String label, String aspect) { - return new AutoValue_JarOwner(label, aspect); + public static JarOwner create(Path jar, String label, Optional aspect) { + return new AutoValue_JarOwner(jar, Optional.of(label), aspect); + } + + public JarOwner withLabel(Optional label) { + return new AutoValue_JarOwner(jar(), label, aspect()); } } diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java index d2d9e8f8ca..f6be3fc94e 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/DependencyModule.java @@ -15,10 +15,12 @@ package com.google.devtools.build.buildjar.javac.plugins.dependency; import static com.google.common.collect.ImmutableList.toImmutableList; +import static java.util.stream.Collectors.joining; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Streams; import com.google.devtools.build.buildjar.JarOwner; import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin; import com.google.devtools.build.lib.view.proto.Deps.Dependencies; @@ -37,6 +39,7 @@ import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.stream.Stream; import javax.tools.Diagnostic; import javax.tools.JavaFileObject; @@ -340,12 +343,11 @@ public final class DependencyModule { private static class DefaultFixMessage implements FixMessage { @Override public String get(Iterable missing, String recipient, DependencyModule depModule) { - StringBuilder missingTargetsStr = new StringBuilder(); - for (JarOwner owner : missing) { - missingTargetsStr.append(owner.label()); - missingTargetsStr.append(" "); - } - + // TODO(cushon): remove the extra whitespace at the end, and fix local_repository_test_jdk8 + String missingTargetsStr = + Streams.stream(missing) + .flatMap(owner -> owner.label().map(Stream::of).orElse(Stream.empty())) + .collect(joining(" ", "", " ")); return String.format( "%1$s ** Please add the following dependencies:%2$s \n %3$s to %4$s \n" + "%1$s ** You can use the following buildozer command:%2$s " diff --git a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java index 58ebbcfcbc..952d4ce5e8 100644 --- a/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java +++ b/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/javac/plugins/dependency/StrictJavaDepsPlugin.java @@ -14,13 +14,13 @@ package com.google.devtools.build.buildjar.javac.plugins.dependency; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule.StrictJavaDeps.ERROR; import com.google.auto.value.AutoValue; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Ordering; import com.google.devtools.build.buildjar.JarOwner; import com.google.devtools.build.buildjar.javac.plugins.BlazeJavaCompilerPlugin; import com.google.devtools.build.buildjar.javac.plugins.dependency.DependencyModule.StrictJavaDeps; @@ -45,12 +45,12 @@ import java.io.InputStream; import java.io.PrintWriter; import java.io.UncheckedIOException; import java.nio.file.Path; -import java.text.MessageFormat; import java.util.ArrayList; +import java.util.Comparator; import java.util.HashSet; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Properties; import java.util.Set; import java.util.jar.Attributes; @@ -209,14 +209,18 @@ public final class StrictJavaDepsPlugin extends BlazeJavaCompilerPlugin { ? null // we don't use the target mapping for the target, just the missing deps : canonicalizeTarget(dependencyModule.getTargetLabel()); - LinkedHashSet canonicalizedMissing = new LinkedHashSet<>(); - for (JarOwner owner : - Ordering.natural().onResultOf(JarOwner.LABEL).immutableSortedCopy(missingTargets)) { - // for dependencies that are missing we canonicalize and remap the target so we don't - // suggest private build labels. - String actualTarget = canonicalizeTarget(remapTarget(owner.label())); - canonicalizedMissing.add(JarOwner.create(actualTarget, owner.aspect())); - } + Set canonicalizedMissing = + missingTargets + .stream() + .filter(owner -> owner.label().isPresent()) + .sorted(Comparator.comparing((JarOwner owner) -> owner.label().get())) + // for dependencies that are missing we canonicalize and remap the target so we don't + // suggest private build labels. + .map( + owner -> + owner.withLabel( + owner.label().map(label -> canonicalizeTarget(remapTarget(label))))) + .collect(toImmutableSet()); errWriter.print( dependencyModule .getFixMessage() @@ -231,10 +235,6 @@ public final class StrictJavaDepsPlugin extends BlazeJavaCompilerPlugin { */ private static class CheckingTreeScanner extends TreeScanner { - private static final String TRANSITIVE_DEP_MESSAGE = - "[strict] Using {0} from an indirect dependency (TOOL_INFO: \"{1}\"). " - + "See command below **"; - private final ImmutableSet directJars; /** Strict deps diagnostics. */ @@ -308,20 +308,27 @@ public final class StrictJavaDepsPlugin extends BlazeJavaCompilerPlugin { // IO cost here is fine because we only hit this path for an explicit dependency // _not_ in the direct jars, i.e. an error JarOwner owner = readJarOwnerFromManifest(jarPath); - if (owner != null && seenTargets.add(owner)) { + if (seenTargets.add(owner)) { // owner is of the form "//label/of:rule " where is // optional. - String canonicalTargetName = canonicalizeTarget(remapTarget(owner.label())); + Optional canonicalTargetName = + owner.label().map(label -> canonicalizeTarget(remapTarget(label))); missingTargets.add(owner); String toolInfo = - owner.aspect() == null - ? canonicalTargetName - : String.format("%s wrapped in %s", canonicalTargetName, owner.aspect()); + owner.aspect().isPresent() + ? String.format( + "%s wrapped in %s", canonicalTargetName.get(), owner.aspect().get()) + : canonicalTargetName.isPresent() + ? canonicalTargetName.get() + : owner.jar().toString(); String used = sym.getSimpleName().contentEquals("package-info") ? "package " + sym.getEnclosingElement() : "type " + sym; - String message = MessageFormat.format(TRANSITIVE_DEP_MESSAGE, used, toolInfo); + String message = + String.format( + "[strict] Using %s from an indirect dependency (TOOL_INFO: \"%s\").%s", + used, toolInfo, (owner.label().isPresent() ? " See command below **" : "")); diagnostics.add(SjdDiagnostic.create(node.pos, message, source)); } } @@ -346,15 +353,15 @@ public final class StrictJavaDepsPlugin extends BlazeJavaCompilerPlugin { try (JarFile jarFile = new JarFile(jarPath.toFile())) { Manifest manifest = jarFile.getManifest(); if (manifest == null) { - return null; + return JarOwner.create(jarPath); } Attributes attributes = manifest.getMainAttributes(); String label = (String) attributes.get(TARGET_LABEL); if (label == null) { - return null; + return JarOwner.create(jarPath); } String injectingRuleKind = (String) attributes.get(INJECTING_RULE_KIND); - return JarOwner.create(label, injectingRuleKind); + return JarOwner.create(jarPath, label, Optional.ofNullable(injectingRuleKind)); } catch (IOException e) { // This jar file pretty much has to exist, we just used it in the compiler. Throw unchecked. throw new UncheckedIOException(e); -- cgit v1.2.3