Don't include absolute paths in blaze IDE artifacts

RELNOTES: Don't include absolute paths in blaze IDE artifacts

--
MOS_MIGRATED_REVID=114550121

4 files changed, 311 insertions, 62 deletions

diff --git a/src/test/java/com/google/devtools/build/android/ideinfo/ArtifactLocationConverterTest.java b/src/test/java/com/google/devtools/build/android/ideinfo/ArtifactLocationConverterTest.java
new file mode 100644
index 0000000000..0ac6311857
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/android/ideinfo/ArtifactLocationConverterTest.java
@@ -0,0 +1,96 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.android.ideinfo;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+
+import com.google.common.base.Joiner;
+import com.google.devtools.build.lib.ideinfo.androidstudio.PackageManifestOuterClass.ArtifactLocation;
+import com.google.devtools.common.options.OptionsParsingException;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests {@link ArtifactLocationConverter}.
+ */
+@RunWith(JUnit4.class)
+public class ArtifactLocationConverterTest {
+
+  private ArtifactLocationConverter converter;
+
+  @Before
+  public final void init() throws Exception  {
+    converter = new ArtifactLocationConverter();
+  }
+
+  @Test
+  public void testConverterSourceArtifact() throws Exception {
+    ArtifactLocation parsed = converter.convert(
+        Joiner.on(",").join("", "test.java", "/usr/local/code")
+    );
+    assertThat(parsed).isEqualTo(
+        ArtifactLocation.newBuilder()
+        .setRootPath("/usr/local/code")
+        .setRelativePath("test.java")
+        .setIsSource(true)
+        .build());
+  }
+
+  @Test
+  public void testConverterDerivedArtifact() throws Exception {
+    ArtifactLocation parsed = converter.convert(
+        Joiner.on(",").join("bin", "java/com/test.java", "/usr/local/_tmp/code/bin")
+    );
+    assertThat(parsed).isEqualTo(
+        ArtifactLocation.newBuilder()
+            .setRootPath("/usr/local/_tmp/code/bin")
+            .setRootExecutionPathFragment("bin")
+            .setRelativePath("java/com/test.java")
+            .setIsSource(false)
+            .build());
+  }
+
+  @Test
+  public void testInvalidFormatFails() throws Exception {
+    assertFails("/root", "Expected either 2 or 3 comma-separated paths");
+    assertFails("/root,exec,rel,extra", "Expected either 2 or 3 comma-separated paths");
+  }
+
+  @Test
+  public void testFutureFormat() throws Exception {
+    ArtifactLocation parsed = converter.convert("bin/out,java/com/test.java");
+    assertThat(parsed).isEqualTo(
+        ArtifactLocation.newBuilder()
+            .setRootExecutionPathFragment("bin/out")
+            .setRelativePath("java/com/test.java")
+            .setIsSource(false)
+            .build());
+  }
+
+
+  private void assertFails(String input, String expectedError) {
+    try {
+      new ArtifactLocationConverter().convert(input);
+      fail();
+    } catch (OptionsParsingException e) {
+      assertThat(e).hasMessage(expectedError);
+    }
+  }
+}
+
diff --git a/src/test/java/com/google/devtools/build/android/ideinfo/BUILD b/src/test/java/com/google/devtools/build/android/ideinfo/BUILD
index 79f75dd8a3..aa917ed80a 100644
--- a/src/test/java/com/google/devtools/build/android/ideinfo/BUILD
+++ b/src/test/java/com/google/devtools/build/android/ideinfo/BUILD
@@ -1,8 +1,9 @@
 java_test(
     name = "PackageParserTest",
     size = "small",
-    srcs = glob(["*.java"]),
+    srcs = ["com.google.devtools.build.android.ideinfo.PackageParserTest"],
     deps = [
+        "//src/main/java/com/google/devtools/common/options",
         "//src/main/protobuf:package_manifest_proto",
         "//src/tools/android/java/com/google/devtools/build/android/ideinfo:package_parser_lib",
         "//third_party:guava",
@@ -12,3 +13,18 @@ java_test(
         "//third_party:truth",
     ],
 )
+
+java_test(
+    name = "ArtifactLocationConverterTest",
+    size = "small",
+    srcs = ["com.google.devtools.build.android.ideinfo.ArtifactLocationConverterTest"],
+    deps = [
+        "//src/main/java/com/google/devtools/common/options",
+        "//src/main/protobuf:package_manifest_proto",
+        "//src/tools/android/java/com/google/devtools/build/android/ideinfo:package_parser_lib",
+        "//third_party:guava",
+        "//third_party:junit4",
+        "//third_party:protobuf",
+        "//third_party:truth",
+    ],
+)
diff --git a/src/test/java/com/google/devtools/build/android/ideinfo/PackageParserTest.java b/src/test/java/com/google/devtools/build/android/ideinfo/PackageParserTest.java
index a02ac30baf..3f466d6d79 100644
--- a/src/test/java/com/google/devtools/build/android/ideinfo/PackageParserTest.java
+++ b/src/test/java/com/google/devtools/build/android/ideinfo/PackageParserTest.java
@@ -17,9 +17,11 @@ package com.google.devtools.build.android.ideinfo;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.fail;
 
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
+import com.google.devtools.build.lib.ideinfo.androidstudio.PackageManifestOuterClass.ArtifactLocation;
 import com.google.protobuf.MessageLite;
 
 import org.junit.Before;
@@ -35,6 +37,7 @@ import java.io.InputStreamReader;
 import java.io.StringWriter;
 import java.io.UnsupportedEncodingException;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.List;
@@ -50,12 +53,16 @@ public class PackageParserTest {
 
   private static class MockPackageParserIoProvider extends PackageParserIoProvider {
     private final Map<Path, InputStream> sources = Maps.newHashMap();
+    private final List<ArtifactLocation> sourceLocations = Lists.newArrayList();
     private StringWriter writer = new StringWriter();
 
-    public MockPackageParserIoProvider addSource(String filePath, String javaSrc) {
+    public MockPackageParserIoProvider addSource(ArtifactLocation source, String javaSrc) {
       try {
-        sources.put(Paths.get(filePath), new ByteArrayInputStream(javaSrc.getBytes("UTF-8")));
-      } catch (UnsupportedEncodingException e) {
+        Path path = Paths.get(source.getRootExecutionPathFragment(), source.getRelativePath());
+        sources.put(path, new ByteArrayInputStream(javaSrc.getBytes("UTF-8")));
+        sourceLocations.add(source);
+
+      } catch (UnsupportedEncodingException | InvalidPathException e) {
         fail(e.getMessage());
       }
       return this;
@@ -63,11 +70,12 @@ public class PackageParserTest {
 
     public void reset() {
       sources.clear();
+      sourceLocations.clear();
       writer = new StringWriter();
     }
 
-    public List<Path> getPaths() {
-      return Lists.newArrayList(sources.keySet());
+    public List<ArtifactLocation> getSourceLocations() {
+      return Lists.newArrayList(sourceLocations);
     }
 
     @Nonnull
@@ -83,6 +91,21 @@ public class PackageParserTest {
     }
   }
 
+  private static final ArtifactLocation DUMMY_SOURCE_ARTIFACT =
+      ArtifactLocation.newBuilder()
+          .setRootPath("/usr/local/google/code")
+          .setRelativePath("java/com/google/Foo.java")
+          .setIsSource(true)
+          .build();
+
+  private static final ArtifactLocation DUMMY_DERIVED_ARTIFACT =
+      ArtifactLocation.newBuilder()
+          .setRootPath("/usr/local/_tmp/code/bin")
+          .setRootExecutionPathFragment("bin")
+          .setRelativePath("java/com/google/Bla.java")
+          .setIsSource(false)
+          .build();
+
   private MockPackageParserIoProvider mockIoProvider;
   private PackageParser parser;
 
@@ -92,9 +115,9 @@ public class PackageParserTest {
     parser = new PackageParser(mockIoProvider);
   }
 
-  private Map<Path, String> parsePackageStrings() throws Exception {
-    List<Path> paths = mockIoProvider.getPaths();
-    return parser.parsePackageStrings(paths, paths);
+  private Map<ArtifactLocation, String> parsePackageStrings() throws Exception {
+    List<ArtifactLocation> sources = mockIoProvider.getSourceLocations();
+    return parser.parsePackageStrings(sources);
   }
 
   @Test
@@ -102,91 +125,205 @@ public class PackageParserTest {
     String[] args = new String[] {
         "--output_manifest",
         "/tmp/out.manifest",
-        "--sources_absolute_paths",
-        "/path/test1.java:/path/test2.java",
-        "--sources_execution_paths",
-        "/path/test1.java:/path/test2.java"
-    };
+        "--sources",
+        Joiner.on(":").join(
+          ",java/com/google/Foo.java,/usr/local/google/code",
+          "bin,java/com/google/Bla.java,/usr/local/_tmp/code/bin"
+        )};
     PackageParser.PackageParserOptions options = PackageParser.parseArgs(args);
     assertThat(options.outputManifest.toString()).isEqualTo("/tmp/out.manifest");
-    assertThat(options.sourcesAbsolutePaths).hasSize(2);
-    assertThat(options.sourcesExecutionPaths).hasSize(2);
-    assertThat(options.sourcesAbsolutePaths.get(0).toString()).isEqualTo("/path/test1.java");
-    assertThat(options.sourcesAbsolutePaths.get(1).toString()).isEqualTo("/path/test2.java");
+    assertThat(options.sources).hasSize(2);
+    assertThat(options.sources.get(0)).isEqualTo(
+        ArtifactLocation.newBuilder()
+            .setRootPath("/usr/local/google/code")
+            .setRelativePath("java/com/google/Foo.java")
+            .setIsSource(true)
+            .build());
+    assertThat(options.sources.get(1)).isEqualTo(
+        ArtifactLocation.newBuilder()
+            .setRootPath("/usr/local/_tmp/code/bin")
+            .setRootExecutionPathFragment("bin")
+            .setRelativePath("java/com/google/Bla.java")
+            .setIsSource(false)
+            .build());
   }
 
   @Test
   public void testReadNoSources() throws Exception {
-    Map<Path, String> map = parsePackageStrings();
+    Map<ArtifactLocation, String> map = parsePackageStrings();
     assertThat(map).isEmpty();
   }
 
   @Test
   public void testSingleRead() throws Exception {
     mockIoProvider
-        .addSource("java/com/google/Bla.java",
-            "package com.test;\n public class Bla {}\"");
-    Map<Path, String> map = parsePackageStrings();
+        .addSource(
+            DUMMY_SOURCE_ARTIFACT,
+            "package com.google;\n public class Bla {}\"");
+    Map<ArtifactLocation, String> map = parsePackageStrings();
     assertThat(map).hasSize(1);
-    assertThat(map).containsEntry(Paths.get("java/com/google/Bla.java"), "com.test");
+    assertThat(map).containsEntry(
+        DUMMY_SOURCE_ARTIFACT,
+        "com.google");
   }
 
   @Test
   public void testMultiRead() throws Exception {
     mockIoProvider
-        .addSource("java/com/google/Bla.java",
+        .addSource(
+            DUMMY_SOURCE_ARTIFACT,
             "package com.test;\n public class Bla {}\"")
-        .addSource("java/com/other/Foo.java",
+        .addSource(
+            DUMMY_DERIVED_ARTIFACT,
             "package com.other;\n public class Foo {}\"");
-    Map<Path, String> map = parsePackageStrings();
+    Map<ArtifactLocation, String> map = parsePackageStrings();
     assertThat(map).hasSize(2);
-    assertThat(map).containsEntry(Paths.get("java/com/google/Bla.java"), "com.test");
-    assertThat(map).containsEntry(Paths.get("java/com/other/Foo.java"), "com.other");
+    assertThat(map).containsEntry(
+        DUMMY_SOURCE_ARTIFACT,
+        "com.test");
+    assertThat(map).containsEntry(
+        DUMMY_DERIVED_ARTIFACT,
+        "com.other");
   }
 
   @Test
   public void testReadSomeInvalid() throws Exception {
     mockIoProvider
-        .addSource("java/com/google/Bla.java",
+        .addSource(
+            DUMMY_SOURCE_ARTIFACT,
             "package %com.test;\n public class Bla {}\"")
-        .addSource("java/com/other/Foo.java",
+        .addSource(
+            DUMMY_DERIVED_ARTIFACT,
             "package com.other;\n public class Foo {}\"");
-    Map<Path, String> map = parsePackageStrings();
+    Map<ArtifactLocation, String> map = parsePackageStrings();
     assertThat(map).hasSize(1);
-    assertThat(map).containsEntry(Paths.get("java/com/other/Foo.java"), "com.other");
+    assertThat(map).containsEntry(
+        DUMMY_DERIVED_ARTIFACT,
+        "com.other");
   }
 
   @Test
   public void testReadAllInvalid() throws Exception {
     mockIoProvider
-        .addSource("java/com/google/Bla.java",
+        .addSource(
+            DUMMY_SOURCE_ARTIFACT,
             "#package com.test;\n public class Bla {}\"")
-        .addSource("java/com/other/Foo.java",
+        .addSource(
+            DUMMY_DERIVED_ARTIFACT,
             "package com.other\n public class Foo {}\"");
-    Map<Path, String> map = parsePackageStrings();
+    Map<ArtifactLocation, String> map = parsePackageStrings();
     assertThat(map).isEmpty();
   }
 
   @Test
   public void testWriteEmptyMap() throws Exception {
     parser.writeManifest(
-        Maps.<Path, String> newHashMap(), Paths.get("/java/com/google/test.manifest"));
+        Maps.<ArtifactLocation, String> newHashMap(),
+        Paths.get("/java/com/google/test.manifest"),
+        false);
     assertThat(mockIoProvider.writer.toString()).isEmpty();
   }
 
   @Test
   public void testWriteMap() throws Exception {
-    Map<Path, String> map = ImmutableMap.of(
-        Paths.get("/java/com/google/Bla.java"), "com.google",
-        Paths.get("/java/com/other/Foo.java"), "com.other"
+    Map<ArtifactLocation, String> map = ImmutableMap.of(
+        DUMMY_SOURCE_ARTIFACT,
+        "com.google",
+        DUMMY_DERIVED_ARTIFACT,
+        "com.other"
     );
-    parser.writeManifest(map, Paths.get("/java/com/google/test.manifest"));
+    parser.writeManifest(map, Paths.get("/java/com/google/test.manifest"), false);
 
     String writtenString = mockIoProvider.writer.toString();
-    assertThat(writtenString).contains("absolute_path: \"/java/com/google/Bla.java\"");
+    assertThat(writtenString).contains(String.format(
+        "root_path: \"%s\"",
+        DUMMY_SOURCE_ARTIFACT.getRootPath()));
+    assertThat(writtenString).contains(String.format(
+        "relative_path: \"%s\"",
+        DUMMY_SOURCE_ARTIFACT.getRelativePath()));
     assertThat(writtenString).contains("package_string: \"com.google\"");
-    assertThat(writtenString).contains("absolute_path: \"/java/com/other/Foo.java\"");
+
+    assertThat(writtenString).contains(String.format(
+        "root_path: \"%s\"",
+        DUMMY_DERIVED_ARTIFACT.getRootPath()));
+    assertThat(writtenString).contains(String.format(
+        "root_execution_path_fragment: \"%s\"",
+        DUMMY_DERIVED_ARTIFACT.getRootExecutionPathFragment()));
+    assertThat(writtenString).contains(String.format(
+        "relative_path: \"%s\"",
+        DUMMY_DERIVED_ARTIFACT.getRelativePath()));
     assertThat(writtenString).contains("package_string: \"com.other\"");
   }
 
+  @Test
+  public void testHandlesOldFormat() throws Exception {
+    String[] args = new String[] {
+        "--output_manifest",
+        "/tmp/out.manifest",
+        "--sources_absolute_paths",
+        "/usr/local/code/java/com/google/Foo.java:/usr/local/code/java/com/google/Bla.java",
+        "--sources_execution_paths",
+        "java/com/google/Foo.java:java/com/google/Bla.java"
+    };
+    PackageParser.PackageParserOptions options = PackageParser.parseArgs(args);
+    assertThat(options.outputManifest.toString()).isEqualTo("/tmp/out.manifest");
+    assertThat(options.sourcesAbsolutePaths.get(0).toString())
+        .isEqualTo("/usr/local/code/java/com/google/Foo.java");
+    assertThat(options.sourcesAbsolutePaths.get(1).toString())
+        .isEqualTo("/usr/local/code/java/com/google/Bla.java");
+
+    PackageParser.convertFromOldFormat(options);
+    assertThat(options.sources).hasSize(2);
+    assertThat(options.sources.get(0)).isEqualTo(
+        ArtifactLocation.newBuilder()
+            .setRootPath("/usr/local/code/")
+            .setRelativePath("java/com/google/Foo.java")
+            .build());
+
+    mockIoProvider
+        .addSource(options.sources.get(0),
+            "package com.test;\n public class Bla {}\"")
+        .addSource(options.sources.get(1),
+            "package com.other;\n public class Foo {}\"");
+
+    Map<ArtifactLocation, String> map = parser.parsePackageStrings(options.sources);
+    assertThat(map).hasSize(2);
+    assertThat(map).containsEntry(options.sources.get(0), "com.test");
+    assertThat(map).containsEntry(options.sources.get(1), "com.other");
+
+    parser.writeManifest(map, options.outputManifest, true);
+
+    String writtenString = mockIoProvider.writer.toString();
+    assertThat(writtenString).doesNotContain("location");
+    assertThat(writtenString).contains(
+        "absolute_path: \"/usr/local/code/java/com/google/Foo.java\"");
+    assertThat(writtenString).contains(
+        "absolute_path: \"/usr/local/code/java/com/google/Bla.java\"");
+  }
+
+  @Test
+  public void testHandlesFutureFormat() throws Exception {
+    String[] args = new String[] {
+        "--output_manifest",
+        "/tmp/out.manifest",
+        "--sources",
+        ",java/com/google/Foo.java:"
+            + "bin/out,java/com/google/Bla.java",
+    };
+    PackageParser.PackageParserOptions options = PackageParser.parseArgs(args);
+    assertThat(options.outputManifest.toString()).isEqualTo("/tmp/out.manifest");
+    assertThat(options.sources).hasSize(2);
+    assertThat(options.sources.get(0)).isEqualTo(
+        ArtifactLocation.newBuilder()
+            .setRelativePath("java/com/google/Foo.java")
+            .setIsSource(true)
+            .build());
+    assertThat(options.sources.get(1)).isEqualTo(
+        ArtifactLocation.newBuilder()
+            .setRootExecutionPathFragment("bin/out")
+            .setRelativePath("java/com/google/Bla.java")
+            .setIsSource(false)
+            .build());
+  }
+
 }
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 6810d5d50d..22416fc896 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
@@ -43,6 +43,7 @@ import java.io.BufferedReader;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -69,6 +70,10 @@ public class AndroidStudioInfoAspectTest extends AndroidStudioInfoAspectTestBase
         "//com/google/example:simple", ruleIdeInfos);
     if (isNativeTest()) {
       assertThat(ruleIdeInfo.getBuildFile()).isEqualTo(buildFilePath.toString());
+      ArtifactLocation location = ruleIdeInfo.getBuildFileArtifactLocation();
+      assertThat(Paths.get(location.getRootPath(), location.getRelativePath()).toString())
+          .isEqualTo(buildFilePath.toString());
+      assertThat(location.getRelativePath()).isEqualTo("com/google/example/BUILD");
     }
     assertThat(ruleIdeInfo.getKind()).isEqualTo(Kind.JAVA_LIBRARY);
     assertThat(ruleIdeInfo.getDependenciesCount()).isEqualTo(0);
@@ -907,26 +912,21 @@ public class AndroidStudioInfoAspectTest extends AndroidStudioInfoAspectTestBase
         ")");
     Map<String, RuleIdeInfo> ruleIdeInfos = buildRuleIdeInfo("//com/google/example:lib");
     RuleIdeInfo ruleIdeInfo = getRuleInfoAndVerifyLabel("//com/google/example:lib", ruleIdeInfos);
-    assertThat(ruleIdeInfo.getJavaRuleIdeInfo().getSourcesList())
-        .containsExactly(
-            expectedArtifactLocationWithRootPath(
-                    targetConfig.getGenfilesDirectory().getPath().getPathString())
-                .setRelativePath("com/google/example/gen.java")
-                .setIsSource(false)
-                .build(),
-            expectedArtifactLocationWithRootPath(directories.getWorkspace().getPathString())
-                .setRelativePath("com/google/example/Test.java")
-                .setIsSource(true)
-                .build());
-  }
-
-  private ArtifactLocation.Builder expectedArtifactLocationWithRootPath(String pathString) {
-    if (isNativeTest()) {
-      return ArtifactLocation.newBuilder().setRootPath(pathString);
-    } else {
-      // todo(dslomov): Skylark aspect implementation does not yet return a correct root path.
-      return ArtifactLocation.newBuilder();
-    }
+    // todo(dslomov): Skylark aspect implementation does not yet return a correct root path.
+    assertThat(ruleIdeInfo.getJavaRuleIdeInfo().getSourcesList()).containsExactly(
+        ArtifactLocation.newBuilder()
+            .setRootPath(
+                isNativeTest() ? targetConfig.getGenfilesDirectory().getPath().getPathString() : "")
+            .setRootExecutionPathFragment(
+                isNativeTest() ? targetConfig.getGenfilesDirectory().getExecPathString() : "")
+            .setRelativePath("com/google/example/gen.java")
+            .setIsSource(false)
+            .build(),
+        ArtifactLocation.newBuilder()
+            .setRootPath(isNativeTest() ? directories.getWorkspace().getPathString() : "")
+            .setRelativePath("com/google/example/Test.java")
+            .setIsSource(true)
+            .build());
   }
 
   @Test