diff options
author | kmb <kmb@google.com> | 2018-05-07 15:28:53 -0700 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-05-07 15:30:23 -0700 |
commit | ee3cc87ca8382c9eda37012f7fc470326aad1521 (patch) | |
tree | 4b1f279d1383cc178675013405b3c89a35cdf049 /src/java_tools/import_deps_checker/java/com | |
parent | deda170f1d31bdb996055e2e9eaa0dfbadc0df3d (diff) |
Flag indirect dependencies in ImportDepsChecker tool
PiperOrigin-RevId: 195732395
Diffstat (limited to 'src/java_tools/import_deps_checker/java/com')
6 files changed, 167 insertions, 26 deletions
diff --git a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassCache.java b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassCache.java index 623863af8b..232e69b936 100644 --- a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassCache.java +++ b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassCache.java @@ -13,11 +13,13 @@ // limitations under the License. package com.google.devtools.build.importdeps; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; +import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -35,6 +37,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Objects; +import java.util.function.Predicate; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; import javax.annotation.Nullable; @@ -52,10 +55,11 @@ public final class ClassCache implements Closeable { public ClassCache( ImmutableList<Path> bootclasspath, + ImmutableList<Path> directClasspath, ImmutableList<Path> regularClasspath, ImmutableList<Path> inputJars) throws IOException { - lazyClasspath = new LazyClasspath(bootclasspath, regularClasspath, inputJars); + lazyClasspath = new LazyClasspath(bootclasspath, directClasspath, regularClasspath, inputJars); } public AbstractClassEntryState getClassState(String internalName) { @@ -81,16 +85,18 @@ public final class ClassCache implements Closeable { private final String internalName; private final ZipFile zipFile; private final Path jarPath; + private final boolean direct; /** * The state of this class entry. If {@literal null}, then this class has not been resolved yet. */ @Nullable private AbstractClassEntryState state = null; - private LazyClassEntry(String internalName, ZipFile zipFile, Path jarPath) { + private LazyClassEntry(String internalName, ZipFile zipFile, Path jarPath, boolean direct) { this.internalName = internalName; this.zipFile = zipFile; this.jarPath = jarPath; + this.direct = direct; } ZipFile getZipFile() { @@ -165,7 +171,8 @@ public final class ClassCache implements Closeable { } } } - ClassInfoBuilder classInfoBuilder = new ClassInfoBuilder(); + ClassInfoBuilder classInfoBuilder = + new ClassInfoBuilder().setJarPath(classEntry.jarPath).setDirect(classEntry.direct); classReader.accept(classInfoBuilder, ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES); if (resolutionFailurePath == null) { classEntry.state = @@ -211,14 +218,28 @@ public final class ClassCache implements Closeable { public LazyClasspath( ImmutableList<Path> bootclasspath, + ImmutableList<Path> directClasspath, ImmutableList<Path> regularClasspath, ImmutableList<Path> inputJars) throws IOException { - this.bootclasspath = new ClassIndex("boot classpath", bootclasspath); - this.regularClasspath = new ClassIndex("regular classpath", regularClasspath); - this.inputJars = new ClassIndex("input jars", inputJars); + checkArgument( + regularClasspath.containsAll(directClasspath), + "Some direct deps %s missing from transitive deps %s", + directClasspath, + regularClasspath); + this.bootclasspath = new ClassIndex("boot classpath", bootclasspath, Predicates.alwaysTrue()); + this.inputJars = new ClassIndex("input jars", inputJars, Predicates.alwaysTrue()); + this.regularClasspath = + new ClassIndex( + "regular classpath", + regularClasspath, + jar -> + bootclasspath.contains(jar) + || inputJars.contains(jar) + || directClasspath.contains(jar)); + // Reflect runtime resolution order, with input before classpath similar to javac this.orderedClasspath = - ImmutableList.of(this.bootclasspath, this.regularClasspath, this.inputJars); + ImmutableList.of(this.bootclasspath, this.inputJars, this.regularClasspath); this.orderedClasspath.forEach(closer::register); } @@ -255,10 +276,11 @@ public final class ClassCache implements Closeable { private final ImmutableMap<String, LazyClassEntry> classIndex; private final Closer closer; - public ClassIndex(String name, ImmutableList<Path> jarFiles) throws IOException { + public ClassIndex(String name, ImmutableList<Path> jarFiles, Predicate<Path> isDirect) + throws IOException { this.name = name; this.closer = Closer.create(); - classIndex = buildClassIndex(jarFiles, closer); + classIndex = buildClassIndex(jarFiles, closer, isDirect); } @Override @@ -290,9 +312,10 @@ public final class ClassCache implements Closeable { } private static ImmutableMap<String, LazyClassEntry> buildClassIndex( - ImmutableList<Path> jars, Closer closer) throws IOException { + ImmutableList<Path> jars, Closer closer, Predicate<Path> isDirect) throws IOException { HashMap<String, LazyClassEntry> result = new HashMap<>(); for (Path jarPath : jars) { + boolean jarIsDirect = isDirect.test(jarPath); try { ZipFile zipFile = closer.register(new ZipFile(jarPath.toFile())); zipFile @@ -305,7 +328,8 @@ public final class ClassCache implements Closeable { } String internalName = name.substring(0, name.lastIndexOf('.')); result.computeIfAbsent( - internalName, key -> new LazyClassEntry(key, zipFile, jarPath)); + internalName, + key -> new LazyClassEntry(key, zipFile, jarPath, jarIsDirect)); }); } catch (Throwable e) { throw new RuntimeException("Error in reading zip file " + jarPath, e); @@ -321,6 +345,8 @@ public final class ClassCache implements Closeable { private String internalName; private final ImmutableSet.Builder<MemberInfo> members = ImmutableSet.builder(); private ImmutableList<String> superClasses; + private Path jarPath; + private boolean directDep; public ClassInfoBuilder() { super(Opcodes.ASM6); @@ -353,6 +379,16 @@ public final class ClassCache implements Closeable { return null; } + public ClassInfoBuilder setJarPath(Path jarPath) { + this.jarPath = jarPath; + return this; + } + + public ClassInfoBuilder setDirect(boolean direct) { + this.directDep = direct; + return this; + } + public ClassInfo build(LazyClasspath lazyClasspath, boolean incomplete) { ImmutableList<ClassInfo> superClassInfos = superClasses @@ -368,7 +404,12 @@ public final class ClassCache implements Closeable { internalName, superClasses, superClassInfos); - return ClassInfo.create(checkNotNull(internalName), superClassInfos, members.build()); + return ClassInfo.create( + checkNotNull(internalName), + checkNotNull(jarPath), + directDep, + superClassInfos, + members.build()); } } } diff --git a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassInfo.java b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassInfo.java index e017e69252..0a1e921ad0 100644 --- a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassInfo.java +++ b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ClassInfo.java @@ -20,6 +20,7 @@ import com.google.auto.value.extension.memoized.Memoized; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import java.nio.file.Path; /** * Representation of a class. It maintains the internal name, declared members, as well as the super @@ -30,13 +31,19 @@ public abstract class ClassInfo { public static ClassInfo create( String internalName, + Path jarPath, + boolean directDep, ImmutableList<ClassInfo> superClasses, ImmutableSet<MemberInfo> declaredMembers) { - return new AutoValue_ClassInfo(internalName, superClasses, declaredMembers); + return new AutoValue_ClassInfo(internalName, jarPath, directDep, superClasses, declaredMembers); } public abstract String internalName(); + public abstract Path jarPath(); + + public abstract boolean directDep(); + /** * Returns all the available super classes. There may be more super classes (super class or * interfaces), but those do not exist on the classpath. diff --git a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/DepsCheckerClassVisitor.java b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/DepsCheckerClassVisitor.java index 2e04864aea..b57c273e10 100644 --- a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/DepsCheckerClassVisitor.java +++ b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/DepsCheckerClassVisitor.java @@ -172,13 +172,19 @@ public class DepsCheckerClassVisitor extends ClassVisitor { AbstractClassEntryState state = classCache.getClassState(internalName); if (state.isMissingState()) { resultCollector.addMissingOrIncompleteClass(internalName, state); - } else if (state.isIncompleteState()) { - String missingAncestor = state.asIncompleteState().getMissingAncestor(); - AbstractClassEntryState ancestorState = classCache.getClassState(missingAncestor); - checkState( - ancestorState.isMissingState(), "The ancestor should be missing. %s", ancestorState); - resultCollector.addMissingOrIncompleteClass(missingAncestor, ancestorState); - resultCollector.addMissingOrIncompleteClass(internalName, state); + } else { + if (state.isIncompleteState()) { + String missingAncestor = state.asIncompleteState().getMissingAncestor(); + AbstractClassEntryState ancestorState = classCache.getClassState(missingAncestor); + checkState( + ancestorState.isMissingState(), "The ancestor should be missing. %s", ancestorState); + resultCollector.addMissingOrIncompleteClass(missingAncestor, ancestorState); + resultCollector.addMissingOrIncompleteClass(internalName, state); + } + ClassInfo info = state.classInfo().get(); + if (!info.directDep()) { + resultCollector.addIndirectDep(info.jarPath()); + } } return state; } diff --git a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ImportDepsChecker.java b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ImportDepsChecker.java index 21089667c3..9276d92eb6 100644 --- a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ImportDepsChecker.java +++ b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ImportDepsChecker.java @@ -25,9 +25,13 @@ import java.io.Closeable; import java.io.IOError; import java.io.IOException; import java.io.InputStream; +import java.io.UncheckedIOException; import java.nio.file.Path; +import java.util.Objects; +import java.util.jar.JarFile; import java.util.stream.Collectors; import java.util.zip.ZipFile; +import javax.annotation.Nullable; import org.objectweb.asm.ClassReader; /** @@ -42,10 +46,11 @@ public final class ImportDepsChecker implements Closeable { public ImportDepsChecker( ImmutableList<Path> bootclasspath, + ImmutableList<Path> directClasspath, ImmutableList<Path> classpath, ImmutableList<Path> inputJars) throws IOException { - this.classCache = new ClassCache(bootclasspath, classpath, inputJars); + this.classCache = new ClassCache(bootclasspath, directClasspath, classpath, inputJars); this.resultCollector = new ResultCollector(); this.inputJars = inputJars; } @@ -90,6 +95,7 @@ public final class ImportDepsChecker implements Closeable { public Dependencies emitJdepsProto(String ruleLabel) { Dependencies.Builder builder = Dependencies.newBuilder(); ImmutableList<Path> paths = classCache.collectUsedJarsInRegularClasspath(); + // TODO(b/77723273): Consider "implicit" for Jars only needed to resolve supertypes paths.forEach( path -> builder.addDependency( @@ -99,7 +105,7 @@ public final class ImportDepsChecker implements Closeable { private static final String INDENT = " "; - public String computeResultOutput() { + public String computeResultOutput(String ruleLabel) { StringBuilder builder = new StringBuilder(); ImmutableList<String> missingClasses = resultCollector.getSortedMissingClassInternalNames(); for (String missing : missingClasses) { @@ -156,9 +162,51 @@ public final class ImportDepsChecker implements Closeable { .append(missingMembers.size()) .append('\n'); } + + ImmutableList<Path> indirectJars = resultCollector.getSortedIndirectDeps(); + if (!indirectJars.isEmpty()) { + ImmutableList<String> labels = extractLabels(indirectJars); + if (ruleLabel.isEmpty() || labels.isEmpty()) { + builder + .append( + "*** Missing strict dependencies on the following Jars which don't carry " + + "rule labels.\nPlease determine the originating rules, e.g., using Bazel's " + + "'query' command, and add them to the dependencies of ") + .append(ruleLabel.isEmpty() ? inputJars : ruleLabel) + .append('\n'); + for (Path jar : indirectJars) { + builder.append(jar).append('\n'); + } + } else { + builder.append("*** Missing strict dependencies. Run the following command to fix ***\n\n"); + builder.append(" add_dep "); + for (String indirectLabel : labels) { + builder.append(indirectLabel).append(" "); + } + builder.append(ruleLabel).append('\n'); + } + } return builder.toString(); } + private static ImmutableList<String> extractLabels(ImmutableList<Path> jars) { + return jars.parallelStream() + .map(ImportDepsChecker::extractLabel) + .filter(Objects::nonNull) + .distinct() + .sorted() + .collect(ImmutableList.toImmutableList()); + } + + @Nullable + private static String extractLabel(Path jarPath) { + try (JarFile jar = new JarFile(jarPath.toFile())) { + return jar.getManifest().getMainAttributes().getValue("Target-Label"); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + @Override public void close() throws IOException { classCache.close(); diff --git a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/Main.java b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/Main.java index 3726caed30..7c7c034c7e 100644 --- a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/Main.java +++ b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/Main.java @@ -19,6 +19,7 @@ import static com.google.common.io.MoreFiles.asCharSink; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.devtools.build.lib.view.proto.Deps.Dependencies; import com.google.devtools.common.options.Converter; import com.google.devtools.common.options.EnumConverter; @@ -37,6 +38,7 @@ import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; +import java.util.ArrayList; import java.util.List; /** @@ -61,6 +63,19 @@ public class Main { public List<Path> inputJars; @Option( + name = "directdep", + allowMultiple = true, + defaultValue = "", + category = "input", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + converter = ExistingPathConverter.class, + help = "Subset of Jars listed in --classpath_entry that --input Jars are allowed to depend " + + "on directly." + ) + public List<Path> directClasspath; + + @Option( name = "classpath_entry", allowMultiple = true, defaultValue = "", @@ -71,7 +86,7 @@ public class Main { help = "Ordered classpath (Jar) to resolve symbols in the --input jars, like javac's -cp flag." ) - public List<Path> classpath; + public List<Path> fullClasspath; @Option( name = "bootclasspath_entry", @@ -158,10 +173,14 @@ public class Main { try (ImportDepsChecker checker = new ImportDepsChecker( ImmutableList.copyOf(options.bootclasspath), - ImmutableList.copyOf(options.classpath), + // Consider everything direct if no direct classpath is given + options.directClasspath.isEmpty() + ? ImmutableList.copyOf(options.fullClasspath) + : ImmutableList.copyOf(options.directClasspath), + ImmutableList.copyOf(options.fullClasspath), ImmutableList.copyOf(options.inputJars))) { if (!checker.check() && options.checkingMode != CheckingMode.SILENCE) { - String result = checker.computeResultOutput(); + String result = checker.computeResultOutput(options.ruleLabel); checkState(!result.isEmpty(), "The result should NOT be empty."); exitCode = options.checkingMode == CheckingMode.ERROR ? DEPS_ERROR_EXIT_CODE : 0; printErrorMessage(result, options); @@ -213,6 +232,13 @@ public class Main { // Convert --fail_on_errors to --checking_mode options.checkingMode = options.failOnErrors ? CheckingMode.ERROR : CheckingMode.WARNING; } + if (!options.fullClasspath.containsAll(options.directClasspath)) { + ArrayList<Path> missing = Lists.newArrayList(options.directClasspath); + missing.removeAll(options.fullClasspath); + throw new IllegalArgumentException( + "--strictdeps must be a subset of --classpath_entry but has additional entries: " + + missing); + } return options; } diff --git a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ResultCollector.java b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ResultCollector.java index 734b24be2f..5288799f07 100644 --- a/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ResultCollector.java +++ b/src/java_tools/import_deps_checker/java/com/google/devtools/build/importdeps/ResultCollector.java @@ -21,6 +21,7 @@ import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableList; import com.google.devtools.build.importdeps.AbstractClassEntryState.IncompleteState; import com.google.devtools.build.importdeps.ClassInfo.MemberInfo; +import java.nio.file.Path; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -31,6 +32,7 @@ public class ResultCollector { private final HashSet<String> missingClasss = new HashSet<>(); private final HashMap<String, IncompleteState> incompleteClasses = new HashMap<>(); private final HashSet<MissingMember> missingMembers = new HashSet<>(); + private final HashSet<Path> indirectDeps = new HashSet<>(); public ResultCollector() {} @@ -56,13 +58,20 @@ public class ResultCollector { /** Returns {@literal true} if there is NO dependency issue, {@literal false} otherwise. */ public boolean isEmpty() { - return missingClasss.isEmpty() && incompleteClasses.isEmpty() && missingMembers.isEmpty(); + return missingClasss.isEmpty() + && incompleteClasses.isEmpty() + && missingMembers.isEmpty() + && indirectDeps.isEmpty(); } public void addMissingMember(String owner, MemberInfo member) { missingMembers.add(MissingMember.create(owner, member)); } + public void addIndirectDep(Path indirectDep) { + indirectDeps.add(indirectDep); + } + public ImmutableList<String> getSortedMissingClassInternalNames() { return ImmutableList.sortedCopyOf(missingClasss); } @@ -76,6 +85,10 @@ public class ResultCollector { return ImmutableList.sortedCopyOf(missingMembers); } + public ImmutableList<Path> getSortedIndirectDeps() { + return ImmutableList.sortedCopyOf(indirectDeps); + } + /** * A missing member on the classpath. This class does not contain the member name and description, * but also the owner of the member. |