From 39780843f241da1b57fed2396f0b908d1a2cd5f4 Mon Sep 17 00:00:00 2001
From: David Ostrovsky <david@ostrovsky.org>
Date: Fri, 8 Jun 2018 14:58:34 -0700
Subject: maven_jar: Actually add support for sources classifier

Closes: #308, #4798.

7a7c41d7d342cd427e74f091b55690eed13e280d was incomplete addition of
sources classifier. API wasn't extended to support source SHA1. This
has two implications:

1. Sources sha1 artifact cannot be checked once downloaded.
2. Repository cache integration: when enabled, the source artifact
cannot be retrieved from the cache, because its sha1 is not known.

Rectify the problem by adding src_sha1 attribute to native maven_jar
rule.

Test Plan:

  $ bazel test src/test/shell/bazel:bazel_repository_cache_test

PiperOrigin-RevId: 198561462

Change-Id: I9c620cdc3876673195483f9e75bb58108acc87be
PiperOrigin-RevId: 199855818
---
 .../lib/bazel/repository/MavenDownloader.java      | 57 ++++++++++++++++------
 .../lib/bazel/rules/workspace/MavenJarRule.java    | 18 ++++---
 2 files changed, 53 insertions(+), 22 deletions(-)

(limited to 'src/main/java/com/google/devtools')

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 5732f8e1fb..06eccccb0e 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
@@ -63,27 +63,31 @@ public class MavenDownloader extends HttpDownloader {
     Server server = serverValue.getServer();
 
     // Initialize maven artifacts
-    Artifact artifact;
     String artifactCoords = 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 + "'");
-    }
+    String sha1 = retrieveSha1(name, "sha1", mapper);
+    String sha1Src = retrieveSha1(name, "sha1_src", mapper);
 
+    Artifact artifact;
     try {
       artifact = new DefaultArtifact(artifactCoords);
     } catch (IllegalArgumentException e) {
       throw new IOException(e.getMessage());
     }
 
+    Artifact artifactWithSrcs = srcjarCoords(artifact);
+
     boolean isCaching = repositoryCache.isEnabled() && KeyType.SHA1.isValid(sha1);
 
     if (isCaching) {
       Path downloadPath = getDownloadDestination(outputDirectory, artifact);
       Path cachedDestination = repositoryCache.get(sha1, downloadPath, KeyType.SHA1);
       if (cachedDestination != null) {
-        return new JarPaths(cachedDestination, Optional.absent());
+        Path cachedDestinationSrc = null;
+        if (sha1Src != null) {
+          Path downloadPathSrc = getDownloadDestination(outputDirectory, artifactWithSrcs);
+          cachedDestinationSrc = repositoryCache.get(sha1Src, downloadPathSrc, KeyType.SHA1);
+        }
+        return new JarPaths(cachedDestination, Optional.fromNullable(cachedDestinationSrc));
       }
     }
 
@@ -105,7 +109,6 @@ public class MavenDownloader extends HttpDownloader {
     }
 
     // Try also fetching srcjar.
-    Artifact artifactWithSrcs = srcjarCoords(artifact);
     try {
       artifactWithSrcs = downloadArtifact(artifactWithSrcs, repository, session, system);
     } catch (ArtifactResolutionException e) {
@@ -118,24 +121,47 @@ public class MavenDownloader extends HttpDownloader {
       RepositoryCache.assertFileChecksum(sha1, jarDownload, KeyType.SHA1);
     }
 
+    Path srcjarDownload = null;
+    if (artifactWithSrcs.getFile() != null) {
+      srcjarDownload = outputDirectory.getRelative(artifactWithSrcs.getFile().getAbsolutePath());
+      if (!Strings.isNullOrEmpty(sha1Src)) {
+        RepositoryCache.assertFileChecksum(sha1Src, srcjarDownload, KeyType.SHA1);
+      }
+    }
+
     if (isCaching) {
       repositoryCache.put(sha1, jarDownload, KeyType.SHA1);
+      if (srcjarDownload != null && !Strings.isNullOrEmpty(sha1Src)) {
+        repositoryCache.put(sha1Src, srcjarDownload, KeyType.SHA1);
+      }
     }
 
-    if (artifactWithSrcs.getFile() != null) {
-      Path srcjarDownload =
-          outputDirectory.getRelative(artifactWithSrcs.getFile().getAbsolutePath());
-      return new JarPaths(jarDownload, Optional.fromNullable(srcjarDownload));
-    } else {
-      return new JarPaths(jarDownload, Optional.absent());
+    return new JarPaths(jarDownload, Optional.fromNullable(srcjarDownload));
+  }
+
+  private String retrieveSha1(String name, String attribute, WorkspaceAttributeMapper mapper)
+      throws EvalException, IOException {
+    String sha1 =
+        mapper.isAttributeValueExplicitlySpecified(attribute)
+            ? mapper.get(attribute, Type.STRING)
+            : null;
+    if (sha1 != null && !KeyType.SHA1.isValid(sha1)) {
+      throw new IOException("Invalid SHA-1 for maven_jar " + name + ": '" + sha1 + "'");
     }
+    return sha1;
   }
 
   private Path getDownloadDestination(Path outputDirectory, Artifact artifact) {
     String groupIdPath = artifact.getGroupId().replace('.', '/');
     String artifactId = artifact.getArtifactId();
+    String classifier = artifact.getClassifier();
     String version = artifact.getVersion();
-    String filename = artifactId + '-' + version + '.' + artifact.getExtension();
+    String filename = artifactId + '-' + version;
+
+    if (classifier.equals("sources")) {
+      filename += "-sources";
+    }
+    filename += '.' + artifact.getExtension();
 
     StringJoiner joiner = new StringJoiner("/");
     joiner.add(groupIdPath).add(artifactId).add(version).add(filename);
@@ -209,5 +235,4 @@ public class MavenDownloader extends HttpDownloader {
       // No-op.
     }
   }
-
 }
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 5afb93429f..8cdc1ecf94 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
@@ -56,14 +56,19 @@ public class MavenJarRule implements RuleDefinition {
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("server", Type.STRING))
         /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(sha1) -->
-         A SHA-1 hash of the desired jar.
+        A SHA-1 hash of the desired 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 --> */
+        <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("sha1", Type.STRING))
+        /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(sha1_src) -->
+        A SHA-1 hash of the desired jar source file.
+
+        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("sha1_src", Type.STRING))
         .setWorkspaceOnly()
         .build();
   }
@@ -112,6 +117,7 @@ maven_jar(
     name = "com_google_guava_guava",
     artifact = "com.google.guava:guava:18.0",
     sha1 = "cce0823396aa693798f8882e64213b1772032b09",
+    sha1_src = "ad97fe8faaf01a3d3faacecd58e8fa6e78a973ca",
 )
 </pre>
 
-- 
cgit v1.2.3