Pass isExternal field to blaze ide info proto
Roll forward after fixing tests. Changes from rolled-back CL:
Native aspect now determines 'isExternal' identically to skylark aspect.
RepositoryName.isMain was unreliable (at least in testing environment),
because unknown repos (occurring due to some race condition) are flagged
as being in the 'default' repo, distinct from the 'main' repo.
We now bypass RepositoryName entirely, and use the same label string
heuristic as used in the Skylark aspect.
--
PiperOrigin-RevId: 141314075
MOS_MIGRATED_REVID=141314075
diff --git a/src/main/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspect.java b/src/main/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspect.java
index ea7ae87..4c2dc81 100644
--- a/src/main/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspect.java
@@ -19,6 +19,7 @@
import static com.google.devtools.build.lib.packages.Attribute.attr;
import static com.google.devtools.build.lib.packages.BuildType.LABEL;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Function;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
@@ -328,7 +329,7 @@
outputBuilder.setLabel(base.getLabel().toString());
outputBuilder.setBuildFileArtifactLocation(
- makeArtifactLocation(ruleContext.getRule().getPackage()));
+ makeArtifactLocation(ruleContext.getRule().getPackage(), ruleContext.getLabel()));
outputBuilder.setKindString(ruleContext.getRule().getRuleClass());
@@ -620,21 +621,25 @@
}
public static ArtifactLocation makeArtifactLocation(Artifact artifact) {
- return makeArtifactLocation(artifact.getRoot(), artifact.getRootRelativePath());
+ return makeArtifactLocation(
+ artifact.getRoot(), artifact.getRootRelativePath(), isExternal(artifact.getOwner()));
}
- private static ArtifactLocation makeArtifactLocation(Package pkg) {
+ private static ArtifactLocation makeArtifactLocation(Package pkg, Label label) {
Root root =
Root.asSourceRoot(pkg.getSourceRoot(), pkg.getPackageIdentifier().getRepository().isMain());
PathFragment relativePath = pkg.getBuildFile().getPath().relativeTo(root.getPath());
- return makeArtifactLocation(root, relativePath);
+ return makeArtifactLocation(root, relativePath, isExternal(label));
}
- private static ArtifactLocation makeArtifactLocation(Root root, PathFragment relativePath) {
+ @VisibleForTesting
+ static ArtifactLocation makeArtifactLocation(
+ Root root, PathFragment relativePath, boolean isExternal) {
return ArtifactLocation.newBuilder()
.setRootExecutionPathFragment(root.getExecPath().toString())
.setRelativePath(relativePath.toString())
.setIsSource(root.isSourceRoot())
+ .setIsExternal(isExternal)
.build();
}
@@ -643,9 +648,15 @@
.setRootExecutionPathFragment(resourceDir.getRootExecutionPathFragment().toString())
.setRelativePath(resourceDir.getRelativePath().toString())
.setIsSource(resourceDir.isSource())
+ .setIsExternal(false)
.build();
}
+ /** Returns whether this {@link Label} refers to an external repository. */
+ private static boolean isExternal(Label label) {
+ return label.getWorkspaceRoot().startsWith("external");
+ }
+
private JavaIdeInfo makeJavaIdeInfo(
ConfiguredTarget base,
RuleContext ruleContext,
diff --git a/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTest.java b/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTest.java
index 6d157ed..26219b0 100644
--- a/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTest.java
@@ -20,7 +20,9 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.ObjectArrays;
+import com.google.devtools.build.lib.actions.Root;
import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.intellij.ideinfo.IntellijIdeInfo.ArtifactLocation;
import com.google.devtools.intellij.ideinfo.IntellijIdeInfo.CIdeInfo;
import com.google.devtools.intellij.ideinfo.IntellijIdeInfo.CToolchainIdeInfo;
@@ -60,6 +62,7 @@
assertThat(Paths.get(location.getRelativePath()).toString())
.isEqualTo(Paths.get("com/google/example/BUILD").toString());
assertThat(location.getIsSource()).isTrue();
+ assertThat(location.getIsExternal()).isFalse();
assertThat(targetIdeInfo.getKindString()).isEqualTo("java_library");
assertThat(relativePathsForJavaSourcesOf(targetIdeInfo))
.containsExactly("com/google/example/simple/Simple.java");
@@ -97,7 +100,6 @@
} else {
assertEquals(packageManifest.getRelativePath(), "com/google/example/simple.java-manifest");
}
-
}
@Test
@@ -1812,6 +1814,59 @@
buildIdeInfo("//com/google/example:foo");
}
+ @Test
+ public void testExternalRootCorrectlyIdentified() throws Exception {
+ ArtifactLocation location =
+ AndroidStudioInfoAspect.makeArtifactLocation(
+ Root.asSourceRoot(outputBase, false), new PathFragment("external/foo/bar.jar"), true);
+ assertThat(location.getIsExternal()).isTrue();
+ }
+
+ @Test
+ public void testNonExternalRootCorrectlyIdentified() throws Exception {
+ ArtifactLocation location =
+ AndroidStudioInfoAspect.makeArtifactLocation(
+ Root.asSourceRoot(rootDirectory, true), new PathFragment("foo/bar.jar"), false);
+ assertThat(location.getIsExternal()).isFalse();
+ }
+
+ @Test
+ public void testExternalTarget() throws Exception {
+ scratch.file(
+ "/r/BUILD", "java_import(", " name = 'junit',", " jars = ['junit.jar'],", ")");
+ scratch.file("/r/junit.jar");
+
+ // AnalysisMock adds required toolchains, etc. to WORKSPACE, so retain the previous contents.
+ String oldContents = scratch.readFile("WORKSPACE");
+ scratch.overwriteFile("WORKSPACE", oldContents + "\nlocal_repository(name='r', path='/r')");
+ invalidatePackages();
+
+ scratch.file(
+ "com/google/example/BUILD",
+ "java_library(",
+ " name = 'junit',",
+ " exports = ['@r//:junit'],",
+ ")");
+
+ Map<String, TargetIdeInfo> targetIdeInfos = buildIdeInfo("//com/google/example:junit");
+ assertThat(
+ getTargetIdeInfoAndVerifyLabel("//com/google/example:junit", targetIdeInfos)
+ .getBuildFileArtifactLocation()
+ .getIsExternal())
+ .isFalse();
+
+ TargetIdeInfo targetInfo = getTargetIdeInfoAndVerifyLabel("@r//:junit", targetIdeInfos);
+ assertThat(targetInfo.getBuildFileArtifactLocation().getIsExternal()).isTrue();
+ assertThat(targetInfo.getBuildFileArtifactLocation().getRelativePath()).startsWith("external");
+
+ JavaIdeInfo javaInfo = targetInfo.getJavaIdeInfo();
+ assertThat(javaInfo.getJarsList()).hasSize(1);
+ ArtifactLocation jar = javaInfo.getJars(0).getJar();
+ assertThat(jar.getIsSource()).isTrue();
+ assertThat(jar.getIsExternal()).isTrue();
+ assertThat(jar.getRelativePath()).isEqualTo("external/r/junit.jar");
+ }
+
/**
* Returns true if we are testing the native aspect, not the Skylark one. Eventually Skylark
* aspect will be equivalent to a native one, and this method will be removed.
diff --git a/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTestBase.java b/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTestBase.java
index 2bbc0ce..e954169 100644
--- a/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/ideinfo/AndroidStudioInfoAspectTestBase.java
@@ -72,22 +72,27 @@
StringBuilder stringBuilder = new StringBuilder();
if (libraryArtifact.hasJar()) {
stringBuilder.append("<jar:");
- stringBuilder.append(libraryArtifact.getJar().getRelativePath());
+ stringBuilder.append(artifactLocationPath(libraryArtifact.getJar()));
stringBuilder.append(">");
}
if (libraryArtifact.hasInterfaceJar()) {
stringBuilder.append("<ijar:");
- stringBuilder.append(libraryArtifact.getInterfaceJar().getRelativePath());
+ stringBuilder.append(artifactLocationPath(libraryArtifact.getInterfaceJar()));
stringBuilder.append(">");
}
if (libraryArtifact.hasSourceJar()) {
stringBuilder.append("<source:");
- stringBuilder.append(libraryArtifact.getSourceJar().getRelativePath());
+ stringBuilder.append(artifactLocationPath(libraryArtifact.getSourceJar()));
stringBuilder.append(">");
}
return stringBuilder.toString();
}
+
+ private String artifactLocationPath(ArtifactLocation artifact) {
+ String relativePath = artifact.getRelativePath();
+ return artifact.getIsExternal() ? relativePath + "[external]" : relativePath;
+ }
};
protected ConfiguredAspect configuredAspect;
diff --git a/tools/ide/intellij_info.bzl b/tools/ide/intellij_info.bzl
index 12ef9cd..d4b69eb 100644
--- a/tools/ide/intellij_info.bzl
+++ b/tools/ide/intellij_info.bzl
@@ -67,12 +67,34 @@
"""Creates an ArtifactLocation proto from a File."""
if file == None:
return None
+
return struct_omit_none(
- relative_path = file.short_path,
+ relative_path = get_relative_path(file),
is_source = file.is_source,
+ is_external = is_external(file.owner),
root_execution_path_fragment = file.root.path if not file.is_source else None,
)
+def is_external(label):
+ """Determines whether a label corresponds to an external artifact."""
+ return label.workspace_root.startswith("external")
+
+def get_relative_path(artifact):
+ """A temporary workaround to find the root-relative path from an artifact.
+
+ This is required because 'short_path' is incorrect for external source artifacts.
+
+ Args:
+ artifact: the input artifact
+ Returns:
+ string: the root-relative path for this artifact.
+ """
+ # TODO(bazel-team): remove this workaround when Artifact::short_path is fixed.
+ if artifact.is_source and is_external(artifact.owner) and artifact.short_path.startswith(".."):
+ # short_path is '../repo_name/path', we want 'external/repo_name/path'
+ return "external" + artifact.short_path[2:]
+ return artifact.short_path
+
def source_directory_tuple(resource_file):
"""Creates a tuple of (source directory, is_source, root execution path)."""
return (
@@ -94,11 +116,12 @@
root_execution_path_fragment = root_execution_path_fragment)
for (relative_path, is_source, root_execution_path_fragment) in source_directory_tuples]
-def build_file_artifact_location(build_file_path):
+def build_file_artifact_location(ctx):
"""Creates an ArtifactLocation proto representing a location of a given BUILD file."""
return struct(
- relative_path = build_file_path,
+ relative_path = ctx.build_file_path,
is_source = True,
+ is_external = is_external(ctx.label)
)
def library_artifact(java_output):
@@ -562,7 +585,7 @@
kind_string = ctx.rule.kind,
dependencies = list(compiletime_deps),
runtime_deps = list(runtime_deps),
- build_file_artifact_location = build_file_artifact_location(ctx.build_file_path),
+ build_file_artifact_location = build_file_artifact_location(ctx),
c_ide_info = c_ide_info,
c_toolchain_ide_info = c_toolchain_ide_info,
java_ide_info = java_ide_info,
