Repository: bazelbuild/bazel
Issue: maven_jar should also pull source jars
Labels: type: feature request

most maven jars have a parallel -src.jar available.  This is very useful in an IDE as eclipse lets you tie a src-jar to a precompiled-jar.
In mvn that is usually: 
mvn eclipse:eclipse -DdownloadSources=true

I would suggest that this should be the default option in bazel, or at least configurable in the maven_jar(..., downloadSrcs = 1)


Issue comments:
As mentioned on https://groups.google.com/d/msg/bazel-discuss/25X8NKHv4H8/pS2so6AjJAAJ, downloading the javadoc jar would also be nice.

+1

+1

+1

+1

+1

Why is this P4 priority issue? Quoting Gerrit Code Review priority descriptions: [1], that apparently common for all Google projects:

```
Priority-4

Ponies and icebox.
Unfortunate: it's a legitimate issue, but the team never plans to fix this.
```

And even better question: what is the appropriate workaround for this missing feature? What we are surely not going to do is to spam `WORKSPACE` file with two rule invocations for each and every artifact:

```
maven_jar(
  name = 'javax_validation',
  artifact = 'javax.validation:validation-api:1.0.0.GA',
  sha1 = 'b6bd7f9d78f6fdaa3c37dae18a4bd298915f328e',
)

http_jar(
  name = "javax_validation_src",
  url = "http://repo1.maven.org/maven2/javax/validation/validation-api/1.0.0.GA/validation-api-1.0.0.GA-sources.jar",
)
```

Instead, Skylark `maven_jar2` rule that is doing something similar to this `maven_jar` Bucklet rule would be nice: [2].
- [1] https://www.gerritcodereview.com/issues.md#Priority_4
- [2] https://github.com/davido/bucklets/blob/master/maven_jar.bucklet#L60-L73

P4 is almost that yes. For us it mostly means "It makes sense, we have no time to work on it but we would welcome any contribution to make it happens"

Which is weird because it is P4 and assigned. Anyway, defering to 
@kchodorow to reasssess its priority and ping @jin for the reimplementation of maven_jar in skylark.

Probably much easier to add once #1410 is in.

7a7c41d adds this capability! I'm closing this for now but definitely reopen if there are any lingering qs (:

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorDescriptor.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorDescriptor.java
index 40c7b7c297c506..b1d02132cd8f1c 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/DecompressorDescriptor.java
@@ -31,17 +31,21 @@ public class DecompressorDescriptor {
   private final String targetKind;
   private final String targetName;
   private final Path archivePath;
+  private final Optional<Path> archiveSourcesPath;
   private final Path repositoryPath;
   private final Optional<String> prefix;
   private final boolean executable;
   private final Decompressor decompressor;
 
   private DecompressorDescriptor(
-      String targetKind, String targetName, Path archivePath, Path repositoryPath,
+      String targetKind, String targetName, Path archivePath,
+      @Nullable Path archiveSourcesPath,
+      Path repositoryPath,
       @Nullable String prefix, boolean executable, Decompressor decompressor) {
     this.targetKind = targetKind;
     this.targetName = targetName;
     this.archivePath = archivePath;
+    this.archiveSourcesPath = Optional.fromNullable(archiveSourcesPath);
     this.repositoryPath = repositoryPath;
     this.prefix = Optional.fromNullable(prefix);
     this.executable = executable;
@@ -60,6 +64,10 @@ public Path archivePath() {
     return archivePath;
   }
 
+  public Optional<Path> archiveSourcesPath() {
+    return archiveSourcesPath;
+  }
+
   public Path repositoryPath() {
     return repositoryPath;
   }
@@ -113,6 +121,7 @@ public static class Builder {
     private String targetKind;
     private String targetName;
     private Path archivePath;
+    private Path archiveSourcesPath;
     private Path repositoryPath;
     private String prefix;
     private boolean executable;
@@ -126,7 +135,7 @@ public DecompressorDescriptor build() throws RepositoryFunctionException {
         decompressor = DecompressorValue.getDecompressor(archivePath);
       }
       return new DecompressorDescriptor(
-          targetKind, targetName, archivePath, repositoryPath, prefix, executable, decompressor);
+          targetKind, targetName, archivePath, archiveSourcesPath, repositoryPath, prefix, executable, decompressor);
     }
 
     public Builder setTargetKind(String targetKind) {
@@ -144,6 +153,11 @@ public Builder setArchivePath(Path archivePath) {
       return this;
     }
 
+    public Builder setArchiveSourcesPath(Path archiveSourcesPath) {
+      this.archiveSourcesPath = archiveSourcesPath;
+      return this;
+    }
+
     public Builder setRepositoryPath(Path repositoryPath) {
       this.repositoryPath = repositoryPath;
       return this;
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/FileDecompressor.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/FileDecompressor.java
index 9031d4ba54b30c..c2cb2355cd9f4b 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/FileDecompressor.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/FileDecompressor.java
@@ -15,7 +15,9 @@
 package com.google.devtools.build.lib.bazel.repository;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
 import com.google.devtools.build.lib.bazel.repository.DecompressorValue.Decompressor;
+import com.google.devtools.build.lib.vfs.Path;
 
 /**
  * Creates a repository for a random file.
@@ -32,7 +34,7 @@ protected String getPackageName() {
   }
 
   @Override
-  protected String createBuildFile(String baseName) {
+  protected String createBuildFile(String baseName, Optional<Path> sourcesJar) {
     return Joiner.on("\n")
         .join(
             "filegroup(",
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/JarDecompressor.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/JarDecompressor.java
index 703c4a32dd72ae..6825d8083d6aa3 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/JarDecompressor.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/JarDecompressor.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.bazel.repository;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
 import com.google.devtools.build.lib.bazel.repository.DecompressorValue.Decompressor;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
@@ -60,6 +61,12 @@ public Path decompress(DecompressorDescriptor descriptor) throws RepositoryFunct
       if (!jarSymlink.exists()) {
         jarSymlink.createSymbolicLink(descriptor.archivePath());
       }
+      if (descriptor.archiveSourcesPath().isPresent()) {
+        Path sourcesSymlink = jarDirectory.getRelative(descriptor.archiveSourcesPath().get().getBaseName());
+        if (!sourcesSymlink.exists()) {
+          sourcesSymlink.createSymbolicLink(descriptor.archiveSourcesPath().get());
+        }
+      }
       // external/some-name/repository/jar/BUILD.bazel defines the //jar target.
       Path buildFile = jarDirectory.getRelative("BUILD.bazel");
       FileSystemUtils.writeLinesAs(
@@ -69,7 +76,7 @@ public Path decompress(DecompressorDescriptor descriptor) throws RepositoryFunct
               + descriptor.targetKind()
               + " rule "
               + descriptor.targetName(),
-          createBuildFile(baseName));
+          createBuildFile(baseName, descriptor.archiveSourcesPath()));
       if (descriptor.executable()) {
         descriptor.archivePath().chmod(0755);
       }
@@ -84,13 +91,21 @@ protected String getPackageName() {
     return "jar";
   }
 
-  protected String createBuildFile(String baseName) {
-    return Joiner.on("\n")
+  protected String createBuildFile(String baseName, Optional<Path> srcJar) {
+    String buildFileText = Joiner.on("\n")
         .join(
             "java_import(",
             "    name = 'jar',",
-            "    jars = ['" + baseName + "'],",
-            "    visibility = ['//visibility:public']",
+            "    jars = ['" + baseName + "'],");
+
+    if (srcJar.isPresent()) {
+      buildFileText += "\n    srcjar = ':sources',";
+    }
+
+    buildFileText += Joiner.on("\n")
+        .join(
+            "",
+            "    visibility = ['//visibility:public'],",
             ")",
             "",
             "filegroup(",
@@ -98,5 +113,17 @@ protected String createBuildFile(String baseName) {
             "    srcs = ['" + baseName + "'],",
             "    visibility = ['//visibility:public']",
             ")");
+
+    if (srcJar.isPresent()) {
+      return buildFileText + Joiner.on("\n").join("",
+          "",
+          "filegroup(",
+          "    name = 'sources',",
+          "    srcs = ['" + srcJar.get().getBaseName() + "'],",
+          "    visibility = ['//visibility:public']",
+          ")");
+    } else {
+      return buildFileText;
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenDownloader.java
index 264173ef8f59a4..855b328bc5d6c4 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenDownloader.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenDownloader.java
@@ -43,8 +43,8 @@
 import org.eclipse.aether.resolution.ArtifactResult;
 
 /**
- * Downloader for JAR files from Maven repositories.
- * TODO(jingwen): standardize interface between this and HttpDownloader
+ * Downloader for JAR files from Maven repositories. TODO(jingwen): standardize interface between
+ * this and HttpDownloader
  */
 public class MavenDownloader extends HttpDownloader {
 
@@ -79,22 +79,36 @@ public Path download(String name, WorkspaceAttributeMapper mapper, Path outputDi
     this.name = name;
     this.outputDirectory = outputDirectory;
 
+    Artifact artifact = createArtifact(mapper);
+    String sha1 = getSha1(mapper, "sha1");
+    return downloadArtifact(sha1, outputDirectory, artifact, serverValue);
+  }
+
+  /**
+   * Download the sources for the Maven artifact to the output directory. Returns the path to the
+   * jar.
+   */
+  public Path downloadSources(String name, WorkspaceAttributeMapper mapper, Path outputDirectory,
+      MavenServerValue serverValue)
+      throws IOException, EvalException {
+    this.name = name;
+    this.outputDirectory = outputDirectory;
+
+    Artifact artifact = createArtifact(mapper);
+    Artifact sourcesArtifact =
+        new DefaultArtifact(artifact.getGroupId(), artifact.getArtifactId(), "sources",
+            artifact.getExtension(), artifact.getVersion());
+    String sha1 = getSha1(mapper, "sources_sha1");
+
+    return downloadArtifact(sha1, outputDirectory, sourcesArtifact, serverValue);
+  }
+
+  private Path downloadArtifact(String sha1, Path outputDirectory, Artifact artifact,
+      MavenServerValue serverValue) throws EvalException, IOException {
+
     String url = serverValue.getUrl();
     Server server = serverValue.getServer();
 
-    Artifact artifact;
-    String artifactId = mapper.get("artifact", Type.STRING);
-    String sha1 = mapper.isAttributeValueExplicitlySpecified("sha1")
-        ? mapper.get("sha1", Type.STRING) : null;
-        if (sha1 != null && !KeyType.SHA1.isValid(sha1)) {
-          throw new IOException("Invalid SHA-1 for maven_jar " + name + ": '" + sha1 + "'");
-        }
-    try {
-      artifact = new DefaultArtifact(artifactId);
-    } catch (IllegalArgumentException e) {
-      throw new IOException(e.getMessage());
-    }
- 
     boolean isCaching = repositoryCache.isEnabled() && KeyType.SHA1.isValid(sha1);
 
     if (isCaching) {
@@ -137,6 +151,29 @@ public Path download(String name, WorkspaceAttributeMapper mapper, Path outputDi
     return downloadPath;
   }
 
+  private String getSha1(WorkspaceAttributeMapper mapper, String attrName)
+      throws EvalException, IOException {
+    String sha1 = mapper.isAttributeValueExplicitlySpecified(attrName)
+        ? mapper.get(attrName, Type.STRING) : null;
+    if (sha1 != null && !KeyType.SHA1.isValid(sha1)) {
+      throw new IOException(
+          "Invalid SHA-1 attr " + attrName + " for maven_jar " + name + ": '" + sha1 + "'");
+    }
+    return sha1;
+  }
+
+  private Artifact createArtifact(WorkspaceAttributeMapper mapper)
+      throws EvalException, IOException {
+    Artifact artifact;
+    String artifactId = mapper.get("artifact", Type.STRING);
+    try {
+      artifact = new DefaultArtifact(artifactId);
+    } catch (IllegalArgumentException e) {
+      throw new IOException(e.getMessage());
+    }
+    return artifact;
+  }
+
   private Path getDownloadDestination(Artifact artifact) {
     String groupIdPath = artifact.getGroupId().replace('.', '/');
     String artifactId = artifact.getArtifactId();
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunction.java
index 907227e959aa4a..87cdbf7069e5e1 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunction.java
@@ -120,16 +120,40 @@ private RepositoryDirectoryValue.Builder createOutputTree(Rule rule, Path output
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     }
 
+    Path repositorySourcesJar = null;
+    if (shouldAttachSources(rule)) {
+      try {
+        repositorySourcesJar =
+            mavenDownloader.downloadSources(name, WorkspaceAttributeMapper.of(rule),
+                outputDirectory, serverValue);
+      } catch (IOException e) {
+        throw new RepositoryFunctionException(e, Transience.TRANSIENT);
+      } catch (EvalException e) {
+        throw new RepositoryFunctionException(e, Transience.TRANSIENT);
+      }
+    }
+
     // Add a WORKSPACE file & BUILD file to the Maven jar.
     Path result = DecompressorValue.decompress(DecompressorDescriptor.builder()
         .setDecompressor(JarDecompressor.INSTANCE)
         .setTargetKind(MavenJarRule.NAME)
         .setTargetName(name)
         .setArchivePath(repositoryJar)
+        .setArchiveSourcesPath(repositorySourcesJar)
         .setRepositoryPath(outputDirectory).build());
     return RepositoryDirectoryValue.builder().setPath(result);
   }
 
+  private boolean shouldAttachSources(Rule rule) throws RepositoryFunctionException {
+    WorkspaceAttributeMapper mapper = WorkspaceAttributeMapper.of(rule);
+    try {
+      return mapper.isAttributeValueExplicitlySpecified("attach_sources")
+          && mapper.get("attach_sources", Type.BOOLEAN) == Boolean.TRUE;
+    } catch (EvalException e) {
+      throw new RepositoryFunctionException(e, Transience.PERSISTENT);
+    }
+  }
+
   /**
    * @see RepositoryFunction#getRule(RepositoryName, String, Environment)
    */
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/MavenJarRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/MavenJarRule.java
index 63c0e95b0d91a8..2880982b9d2a38 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/MavenJarRule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/MavenJarRule.java
@@ -65,6 +65,22 @@ public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
          should be set before shipping.</p>
          <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("sha1", Type.STRING))
+        /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(attach_sources) -->
+         Set this to make Bazel also pull down the sources jar from the Maven repository.
+
+         <p>The sources jar will be located in the same package as the main jar with the label
+         :sources</p>
+         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("attach_sources", Type.BOOLEAN))
+        /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(sources_sha1) -->
+         A SHA-1 hash of the desired sources jar.
+
+         <p>If the downloaded jar does not match this hash, Bazel will error out. <em>It is a
+         security risk to omit the SHA-1 as remote files can change.</em> At best omitting this
+         field will make your build non-hermetic. It is optional to make development easier but
+         should be set before shipping.</p>
+         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("sources_sha1", Type.STRING))
         .setWorkspaceOnly()
         .build();
   }


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/JarDecompressorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/JarDecompressorTest.java
index 37cfa8daa33f41..f3bafaeb8e79ef 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/repository/JarDecompressorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/repository/JarDecompressorTest.java
@@ -32,13 +32,17 @@
  */
 @RunWith(JUnit4.class)
 public class JarDecompressorTest {
+  private static final String FOO_JAR = "/foo.jar";
+  private static final String BUILD_FILE = "BUILD.bazel";
+  private static final String FOO_SOURCES_JAR = "/foo-sources.jar";
   private DecompressorDescriptor.Builder descriptorBuilder;
+  private Scratch fs;
 
   @Before
   public void setUpFs() throws Exception {
-    Scratch fs = new Scratch();
+    fs = new Scratch();
     Path dir = fs.dir("/whatever/external/tester");
-    Path jar = fs.file("/foo.jar", "I'm a jar");
+    Path jar = fs.file(FOO_JAR, "I'm a jar");
     FileSystemUtils.createDirectoryAndParents(dir);
     descriptorBuilder = DecompressorDescriptor.builder()
         .setDecompressor(JarDecompressor.INSTANCE)
@@ -53,7 +57,9 @@ public void testTargets() throws Exception {
     Path outputDir = DecompressorValue.decompress(descriptorBuilder.build());
     assertThat(outputDir.exists()).isTrue();
     String buildContent =
-        new String(FileSystemUtils.readContentAsLatin1(outputDir.getRelative("jar/BUILD.bazel")));
+        new String(FileSystemUtils.readContentAsLatin1(jarDirectoryFile(BUILD_FILE, outputDir)));
+    assertThat(jarDirectoryFile(FOO_JAR, outputDir).exists()).isTrue();
+    assertThat(jarDirectoryFile(FOO_SOURCES_JAR, outputDir).exists()).isFalse();
     assertThat(buildContent).contains("java_import");
     assertThat(buildContent).contains("filegroup");
   }
@@ -66,4 +72,25 @@ public void testWorkspaceGen() throws Exception {
         FileSystemUtils.readContentAsLatin1(outputDir.getRelative("WORKSPACE")));
     assertThat(workspaceContent).contains("workspace(name = \"tester\")");
   }
+
+  @Test
+  public void testTargetsWithSources() throws Exception {
+    Path sourceJar = fs.file(FOO_SOURCES_JAR, "I'm a sources jar");
+    descriptorBuilder.setArchiveSourcesPath(sourceJar);
+
+    Path outputDir = DecompressorValue.decompress(descriptorBuilder.build());
+    assertThat(outputDir.exists()).isTrue();
+    assertThat(jarDirectoryFile(sourceJar.getBaseName(), outputDir).exists()).isTrue();
+    assertThat(jarDirectoryFile(FOO_JAR, outputDir).exists()).isTrue();
+
+    String buildContent =
+        new String(FileSystemUtils.readContentAsLatin1(jarDirectoryFile(BUILD_FILE, outputDir)));
+    assertThat(buildContent).contains("java_import");
+    assertThat(buildContent).contains("srcjar");
+    assertThat(buildContent).contains("filegroup");
+  }
+
+  private Path jarDirectoryFile(String baseName, Path outputPath) {
+    return outputPath.getRelative("jar/" + baseName);
+  }
 }