repository_ctx.download{_and_extract} return a struct.

The methods currently return a struct with an entry for "sha256". If a sha was provided in the rule and it was the correct sha, that sha will be returned. If no sha was provided, the calculated sha will be returned.

Going forward, removing entries from the returned struct will be subject to proper deprecation periods, but additions can happen at any time. Repository rule writers should not depend on the absence of entries in the struct.

RELNOTES[NEW]: repository_ctx.download and repository_ctx.download_and_extract now return a struct.

PiperOrigin-RevId: 220474155

Author: dkelmer

src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java

@@ -16,12 +16,14 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Optional;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.bazel.debug.WorkspaceRuleEvent;
 import com.google.devtools.build.lib.bazel.repository.DecompressorDescriptor;
 import com.google.devtools.build.lib.bazel.repository.DecompressorValue;
+import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache;
 import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType;
 import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
 import com.google.devtools.build.lib.bazel.repository.downloader.HttpUtils;
@@ -332,7 +334,7 @@ private SkylarkPath findCommandOnPath(String program) throws IOException {
   }
 
   @Override
-  public void download(
+  public StructImpl download(
       Object url, Object output, String sha256, Boolean executable, Location location)
       throws RepositoryFunctionException, EvalException, InterruptedException {
     validateSha256(sha256);
@@ -342,16 +344,18 @@ public void download(
         WorkspaceRuleEvent.newDownloadEvent(
             urls, output.toString(), sha256, executable, rule.getLabel().toString(), location);
     env.getListener().post(w);
+    Path downloadedPath;
     try {
       checkInOutputDirectory(outputPath);
       makeDirectories(outputPath.getPath());
-      httpDownloader.download(
-          urls,
-          sha256,
-          Optional.<String>absent(),
-          outputPath.getPath(),
-          env.getListener(),
-          osObject.getEnvironmentVariables());
+      downloadedPath =
+          httpDownloader.download(
+              urls,
+              sha256,
+              Optional.<String>absent(),
+              outputPath.getPath(),
+              env.getListener(),
+              osObject.getEnvironmentVariables());
       if (executable) {
         outputPath.getPath().setExecutable(true);
       }
@@ -361,10 +365,21 @@ public void download(
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     }
+    String finalSha256;
+    try {
+      finalSha256 = calculateSha256(sha256, downloadedPath);
+    } catch (IOException e) {
+      throw new RepositoryFunctionException(
+          new IOException(
+              "Couldn't hash downloaded file (" + downloadedPath.getPathString() + ")", e),
+          Transience.PERSISTENT);
+    }
+    SkylarkDict<String, Object> dict = SkylarkDict.of(null, "sha256", finalSha256);
+    return StructProvider.STRUCT.createStruct(dict, null);
   }
 
   @Override
-  public void downloadAndExtract(
+  public StructImpl downloadAndExtract(
       Object url, Object output, String sha256, String type, String stripPrefix, Location location)
       throws RepositoryFunctionException, InterruptedException, EvalException {
     validateSha256(sha256);
@@ -410,6 +425,15 @@ public void downloadAndExtract(
             .setRepositoryPath(outputPath.getPath())
             .setPrefix(stripPrefix)
             .build());
+    String finalSha256 = null;
+    try {
+      finalSha256 = calculateSha256(sha256, downloadedPath);
+    } catch (IOException e) {
+      throw new RepositoryFunctionException(
+          new IOException(
+              "Couldn't hash downloaded file (" + downloadedPath.getPathString() + ")", e),
+          Transience.PERSISTENT);
+    }
     try {
       if (downloadedPath.exists()) {
         downloadedPath.delete();
@@ -420,6 +444,16 @@ public void downloadAndExtract(
               "Couldn't delete temporary file (" + downloadedPath.getPathString() + ")", e),
           Transience.TRANSIENT);
     }
+    SkylarkDict<String, Object> dict = SkylarkDict.of(null, "sha256", finalSha256);
+    return StructProvider.STRUCT.createStruct(dict, null);
+  }
+
+  private String calculateSha256(String originalSha, Path path) throws IOException {
+    if (!Strings.isNullOrEmpty(originalSha)) {
+      // The sha is checked on download, so if we got here, the user provided sha is good
+      return originalSha;
+    }
+    return RepositoryCache.getChecksum(KeyType.SHA256, path);
   }
 
   private static void validateSha256(String sha256) throws RepositoryFunctionException {

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/repository/SkylarkRepositoryContextApi.java

@@ -246,7 +246,9 @@ public SkylarkExecutionResultApi execute(
 
   @SkylarkCallable(
       name = "download",
-      doc = "Download a file to the output path for the provided url.",
+      doc =
+          "Download a file to the output path for the provided url and return the hash of"
+              + " the file.",
       useLocation = true,
       parameters = {
         @Param(
@@ -272,26 +274,27 @@ public SkylarkExecutionResultApi execute(
             type = String.class,
             defaultValue = "''",
             named = true,
-            doc =
-                "the expected SHA-256 hash of the file downloaded."
-                    + " This must match the SHA-256 hash of the file downloaded. It is a security"
-                    + " risk to omit the SHA-256 as remote files can change. At best omitting this"
-                    + " field will make your build non-hermetic. It is optional to make"
-                    + " development easier but should be set before shipping."),
+            doc =  "the expected SHA-256 hash of the file downloaded."
+                + " This must match the SHA-256 hash of the file downloaded. It is a security risk"
+                + " to omit the SHA-256 as remote files can change. At best omitting this field"
+                + " will make your build non-hermetic. It is optional to make development easier"
+                + " but should be set before shipping."),
         @Param(
             name = "executable",
             type = Boolean.class,
             defaultValue = "False",
             named = true,
             doc = "set the executable flag on the created file, false by default."),
       })
-  public void download(
+  public StructApi download(
       Object url, Object output, String sha256, Boolean executable, Location location)
       throws RepositoryFunctionExceptionT, EvalException, InterruptedException;
 
   @SkylarkCallable(
       name = "download_and_extract",
-      doc = "Download a file to the output path for the provided url, and extract it.",
+      doc =
+          "Download a file to the output path for the provided url, extract it, and return the"
+              + " hash of the downloaded file.",
       useLocation = true,
       parameters = {
         @Param(
@@ -319,27 +322,23 @@ public void download(
             type = String.class,
             defaultValue = "''",
             named = true,
-            doc =
-                "the expected SHA-256 hash of the file downloaded."
-                    + " This must match the SHA-256 hash of the file downloaded. It is a security"
-                    + " risk to omit the SHA-256 as remote files can change. At best omitting this"
-                    + " field will make your build non-hermetic. It is optional to make"
-                    + " development easier but should be set before shipping."
-                    + " If provided, the repository cache will first be checked for a file with the"
-                    + " given hash; a download will only be attempted, if the file was not found"
-                    + " in the cache. After a successful download, the file will be added to the"
-                    + " cache."),
+            doc = "the expected SHA-256 hash of the file downloaded."
+                + " This must match the SHA-256 hash of the file downloaded. It is a security risk"
+                + " to omit the SHA-256 as remote files can change. At best omitting this field"
+                + " will make your build non-hermetic. It is optional to make development easier"
+                + " but should be set before shipping."
+                + " If provided, the repository cache will first be checked for a file with the"
+                + " given hash; a download will only be attempted, if the file was not found in the"
+                + " cache. After a successful download, the file will be added to the cache."),
         @Param(
             name = "type",
             type = String.class,
             defaultValue = "''",
             named = true,
-            doc =
-                "the archive type of the downloaded file."
-                    + " By default, the archive type is determined from the file extension of the"
-                    + " URL. If the file has no extension, you can explicitly specify either"
-                    + " \"zip\", \"jar\", \"war\", \"tar.gz\", \"tgz\", \"tar.bz2\", or \"tar.xz\""
-                    + " here."),
+            doc = "the archive type of the downloaded file."
+                + " By default, the archive type is determined from the file extension of the URL."
+                + " If the file has no extension, you can explicitly specify either \"zip\","
+                + " \"jar\", \"war\", \"tar.gz\", \"tgz\", \"tar.bz2\", or \"tar.xz\" here."),
         @Param(
             name = "stripPrefix",
             type = String.class,
@@ -352,7 +351,7 @@ public void download(
                     + " <code>build_file</code>, this field can be used to strip it from extracted"
                     + " files."),
       })
-  public void downloadAndExtract(
+  public StructApi downloadAndExtract(
       Object url, Object output, String sha256, String type, String stripPrefix, Location location)
       throws RepositoryFunctionExceptionT, InterruptedException, EvalException;
 }

src/test/shell/bazel/skylark_repository_test.sh

@@ -851,6 +851,71 @@ EOF
     || fail "download_executable_file.sh is not executable"
 }
 
+function test_skylark_repository_context_downloads_return_struct() {
+   # Prepare HTTP server with Python
+  local server_dir="${TEST_TMPDIR}/server_dir"
+  mkdir -p "${server_dir}"
+  local download_with_sha256="${server_dir}/download_with_sha256.txt"
+  local download_no_sha256="${server_dir}/download_no_sha256.txt"
+  local compressed_with_sha256="${server_dir}/compressed_with_sha256.txt"
+  local compressed_no_sha256="${server_dir}/compressed_no_sha256.txt"
+  echo "This is one file" > "${download_no_sha256}"
+  echo "This is another file" > "${download_with_sha256}"
+  echo "Compressed file with sha" > "${compressed_with_sha256}"
+  echo "Compressed file no sha" > "${compressed_no_sha256}"
+  zip "${compressed_with_sha256}".zip "${compressed_with_sha256}"
+  zip "${compressed_no_sha256}".zip "${compressed_no_sha256}"
+
+  provided_sha256="$(sha256sum "${download_with_sha256}" | head -c 64)"
+  not_provided_sha256="$(sha256sum "${download_no_sha256}" | head -c 64)"
+  compressed_provided_sha256="$(sha256sum "${compressed_with_sha256}.zip" | head -c 64)"
+  compressed_not_provided_sha256="$(sha256sum "${compressed_no_sha256}.zip" | head -c 64)"
+
+  # Start HTTP server with Python
+  startup_server "${server_dir}"
+
+  setup_skylark_repository
+  # Our custom repository rule
+  cat >test.bzl <<EOF
+def _impl(repository_ctx):
+  no_sha_return = repository_ctx.download(
+    url = "http://localhost:${fileserver_port}/download_no_sha256.txt",
+    output = "download_no_sha256.txt")
+  with_sha_return = repository_ctx.download(
+    url = "http://localhost:${fileserver_port}/download_with_sha256.txt",
+    output = "download_with_sha256.txt",
+    sha256 = "${provided_sha256}")
+  compressed_no_sha_return = repository_ctx.download_and_extract(
+    url = "http://localhost:${fileserver_port}/compressed_no_sha256.txt.zip",
+    output = "compressed_no_sha256.txt.zip")
+  compressed_with_sha_return = repository_ctx.download_and_extract(
+      url = "http://localhost:${fileserver_port}/compressed_with_sha256.txt.zip",
+      output = "compressed_with_sha256.txt.zip",
+      sha256 = "${compressed_provided_sha256}")
+
+  file_content = "no_sha_return " + no_sha_return.sha256 + "\n"
+  file_content += "with_sha_return " + with_sha_return.sha256 + "\n"
+  file_content += "compressed_no_sha_return " + compressed_no_sha_return.sha256 + "\n"
+  file_content += "compressed_with_sha_return " + compressed_with_sha_return.sha256
+  repository_ctx.file("returned_shas.txt", content = file_content, executable = False)
+  repository_ctx.file("BUILD")  # necessary directories should already created by download function
+repo = repository_rule(implementation = _impl, local = False)
+EOF
+
+  bazel build @foo//:all >& $TEST_log && shutdown_server \
+    || fail "Execution of @foo//:all failed"
+
+  output_base="$(bazel info output_base)"
+  grep "no_sha_return $not_provided_sha256" $output_base/external/foo/returned_shas.txt \
+      || fail "expected calculated sha256 $not_provided_sha256"
+  grep "with_sha_return $provided_sha256" $output_base/external/foo/returned_shas.txt \
+      || fail "expected provided sha256 $provided_sha256"
+  grep "compressed_with_sha_return $compressed_provided_sha256" $output_base/external/foo/returned_shas.txt \
+      || fail "expected provided sha256 $compressed_provided_sha256"
+  grep "compressed_no_sha_return $compressed_not_provided_sha256" $output_base/external/foo/returned_shas.txt \
+      || fail "expected compressed calculated sha256 $compressed_not_provided_sha256"
+}
+
 function test_skylark_repository_download_args() {
   # Prepare HTTP server with Python
   local server_dir="${TEST_TMPDIR}/server_dir"