Style cleanups.

--
MOS_MIGRATED_REVID=122058951

5 files changed, 117 insertions, 61 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java
index 9677439705..b4181aa449 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkExecutionResult.java
@@ -21,12 +21,16 @@ import com.google.devtools.build.lib.shell.CommandResult;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.util.Preconditions;
 
 import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
 import java.util.List;
 
 /**
- * A Skylark structure
+ * A structure callable from Skylark that stores the result of repository_ctx.execute() method. It
+ * contains the standard output stream content, the standard error stream content and the execution
+ * return code.
  */
 @SkylarkModule(
   name = "exec_result",
@@ -35,7 +39,7 @@ import java.util.List;
           + " output stream content, the standard error stream content and the execution return"
           + " code."
 )
-public class SkylarkExecutionResult {
+final class SkylarkExecutionResult {
   private final int returnCode;
   private final String stdout;
   private final String stderr;
@@ -58,9 +62,10 @@ public class SkylarkExecutionResult {
   @SkylarkCallable(
     name = "return_code",
     structField = true,
-    doc = "The return code returned after the execution of the program."
+    doc = "The return code returned after the execution of the program. 256 if an error happened"
+        + " while executing the command."
   )
-  public int returnCode() {
+  public int getReturnCode() {
     return returnCode;
   }
 
@@ -69,7 +74,7 @@ public class SkylarkExecutionResult {
     structField = true,
     doc = "The content of the standard output returned by the execution."
   )
-  public String stdout() {
+  public String getStdout() {
     return stdout;
   }
 
@@ -78,32 +83,79 @@ public class SkylarkExecutionResult {
     structField = true,
     doc = "The content of the standard error output returned by the execution."
   )
-  public String stderr() {
+  public String getStderr() {
     return stderr;
   }
 
   /**
-   * Executes a command given by a list of arguments and returns a SkylarkExecutionResult with
-   * the output of the command.
+   * Returns a Builder that can be used to execute a command and build an execution result.
    */
-  static SkylarkExecutionResult execute(List<Object> args, long timeout) throws EvalException {
-    try {
-      String[] argsArray = new String[args.size()];
-      for (int i = 0; i < args.size(); i++) {
+  public static Builder builder() {
+    return new Builder();
+  }
+
+  /**
+   * A Builder class to build a {@link SkylarkExecutionResult} object by executing a command.
+   */
+  static final class Builder {
+
+    private final List<String> args = new ArrayList<>();
+    private long timeout = -1;
+    private boolean executed = false;
+
+    /**
+     * Adds arguments to the list of arguments to pass to the command. The first argument is
+     * expected to be the binary to execute. The subsequent arguments are the arguments passed
+     * to the binary.
+     *
+     * <p>Each argument can be either a string or a {@link SkylarkPath}, passing another argument
+     * will fail when executing the command.
+     */
+    Builder addArguments(Iterable<Object> args) throws EvalException {
+      for (Object arg : args) {
         // We might have skylark path, do conversion.
-        Object arg = args.get(i);
         if (!(arg instanceof String || arg instanceof SkylarkPath)) {
           throw new EvalException(
-              Location.BUILTIN, "Argument " + i + " of execute is neither a path nor a string.");
+              Location.BUILTIN,
+              "Argument " + this.args.size() + " of execute is neither a path nor a string.");
+        }
+        this.args.add(arg.toString());
+      }
+      return this;
+    }
+
+    /**
+     * Sets the timeout, in milliseconds, after which the executed command will be terminated.
+     */
+    Builder setTimeout(long timeout) {
+      Preconditions.checkArgument(timeout > 0, "Timeout must be a positive number.");
+      this.timeout = timeout;
+      return this;
+    }
+
+    /**
+     * Execute the command specified by {@link #addArguments(Iterable)}.
+     */
+    SkylarkExecutionResult execute() throws EvalException {
+      Preconditions.checkArgument(timeout > 0, "Timeout must be set prior to calling execute().");
+      Preconditions.checkArgument(!args.isEmpty(), "No command specified.");
+      Preconditions.checkState(!executed, "Command was already executed, cannot re-use builder.");
+      executed = true;
+
+      try {
+        String[] argsArray = new String[args.size()];
+        for (int i = 0; i < args.size(); i++) {
+          argsArray[i] = args.get(i);
         }
-        argsArray[i] = arg.toString();
+        CommandResult result = new Command(argsArray).execute(new byte[]{}, timeout, false);
+        return new SkylarkExecutionResult(result);
+      } catch (BadExitStatusException e) {
+        return new SkylarkExecutionResult(e.getResult());
+      } catch (CommandException e) {
+        // 256 is outside of the standard range for exit code on Unixes. We are not guaranteed that
+        // on all system it would be outside of the standard range.
+        return new SkylarkExecutionResult(256, "", e.getMessage());
       }
-      CommandResult result = new Command(argsArray).execute(new byte[] {}, timeout, false);
-      return new SkylarkExecutionResult(result);
-    } catch (BadExitStatusException e) {
-      return new SkylarkExecutionResult(e.getResult());
-    } catch (CommandException e) {
-      return new SkylarkExecutionResult(256, "", e.getMessage());
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkOS.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkOS.java
index fd51059137..05ba5cdc44 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkOS.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkOS.java
@@ -38,7 +38,7 @@ final class SkylarkOS {
   }
 
   @SkylarkCallable(name = "environ", structField = true, doc = "The list of environment variables.")
-  public ImmutableMap<String, String> getEnviron() {
+  public ImmutableMap<String, String> getEnvironmentVariables() {
     return environ;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkPath.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkPath.java
index 328fa74e94..3e29c7048e 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkPath.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkPath.java
@@ -33,13 +33,13 @@ import java.util.List;
 @SkylarkModule(name = "path", doc = "A structure representing a file to be used inside a repository"
 )
 final class SkylarkPath {
-  final Path path;
+  private final Path path;
 
-  public SkylarkPath(Path path) {
+  SkylarkPath(Path path) {
     this.path = path;
   }
 
-  public Path getPath() {
+  Path getPath() {
     return path;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
index 372d9baac2..e4dd0293c8 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
@@ -73,8 +73,10 @@ public class SkylarkRepositoryContext {
   private final Environment env;
 
   /**
-   * In native code, private values start with $. In Skylark, private values start with _, because
-   * of the grammar.
+   * Convert attribute name from native naming convention to Skylark naming convention.
+   *
+   * <p>In native code, private values start with $ or :. In Skylark, private values start
+   * with _, because of the grammar.
    */
   private String attributeToSkylark(String oldName) {
     if (!oldName.isEmpty() && (oldName.charAt(0) == '$' || oldName.charAt(0) == ':')) {
@@ -146,15 +148,13 @@ public class SkylarkRepositoryContext {
   private SkylarkPath getPath(String method, Object path) throws EvalException {
     if (path instanceof String) {
       PathFragment pathFragment = new PathFragment(path.toString());
-      if (pathFragment.isAbsolute()) {
-        return new SkylarkPath(outputDirectory.getFileSystem().getPath(path.toString()));
-      } else {
-        return new SkylarkPath(outputDirectory.getRelative(pathFragment));
-      }
+      return new SkylarkPath(pathFragment.isAbsolute()
+          ? outputDirectory.getFileSystem().getPath(path.toString())
+          : outputDirectory.getRelative(pathFragment));
     } else if (path instanceof Label) {
       SkylarkPath result = getPathFromLabel((Label) path);
       if (result == null) {
-        SkylarkRepositoryFunction.restart();
+        throw SkylarkRepositoryFunction.restart();
       }
       return result;
     } else if (path instanceof SkylarkPath) {
@@ -175,8 +175,8 @@ public class SkylarkRepositoryContext {
     SkylarkPath toPath = getPath("symlink()", to);
     try {
       checkInOutputDirectory(toPath);
-      makeDirectories(toPath.path);
-      toPath.path.createSymbolicLink(fromPath.path);
+      makeDirectories(toPath.getPath());
+      toPath.getPath().createSymbolicLink(fromPath.getPath());
     } catch (IOException e) {
       throw new RepositoryFunctionException(
           new IOException(
@@ -187,7 +187,7 @@ public class SkylarkRepositoryContext {
   }
 
   private void checkInOutputDirectory(SkylarkPath path) throws RepositoryFunctionException {
-    if (!path.path.getPathString().startsWith(outputDirectory.getPathString())) {
+    if (!path.getPath().getPathString().startsWith(outputDirectory.getPathString())) {
       throw new RepositoryFunctionException(
           new IOException("Cannot write outside of the output directory for path " + path),
           Transience.TRANSIENT);
@@ -198,6 +198,7 @@ public class SkylarkRepositoryContext {
   public void createFile(Object path) throws RepositoryFunctionException, EvalException {
     createFile(path, "");
   }
+
   @SkylarkCallable(
       name = "file",
       documented = false
@@ -217,12 +218,12 @@ public class SkylarkRepositoryContext {
     SkylarkPath p = getPath("file()", path);
     try {
       checkInOutputDirectory(p);
-      makeDirectories(p.path);
-      try (OutputStream stream = p.path.getOutputStream()) {
+      makeDirectories(p.getPath());
+      try (OutputStream stream = p.getPath().getOutputStream()) {
         stream.write(content.getBytes(StandardCharsets.UTF_8));
       }
       if (executable) {
-        p.path.setExecutable(true);
+        p.getPath().setExecutable(true);
       }
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
@@ -255,17 +256,17 @@ public class SkylarkRepositoryContext {
     SkylarkPath t = getPath("template()", template);
     try {
       checkInOutputDirectory(p);
-      makeDirectories(p.path);
-      String tpl = FileSystemUtils.readContent(t.path, StandardCharsets.UTF_8);
+      makeDirectories(p.getPath());
+      String tpl = FileSystemUtils.readContent(t.getPath(), StandardCharsets.UTF_8);
       for (Map.Entry<String, String> substitution : substitutions.entrySet()) {
         tpl =
             StringUtilities.replaceAllLiteral(tpl, substitution.getKey(), substitution.getValue());
       }
-      try (OutputStream stream = p.path.getOutputStream()) {
+      try (OutputStream stream = p.getPath().getOutputStream()) {
         stream.write(tpl.getBytes(StandardCharsets.UTF_8));
       }
       if (executable) {
-        p.path.setExecutable(true);
+        p.getPath().setExecutable(true);
       }
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
@@ -301,12 +302,14 @@ public class SkylarkRepositoryContext {
             + " command."
   )
   public SkylarkExecutionResult execute(List<Object> arguments, long timeout) throws EvalException {
-    return SkylarkExecutionResult.execute(arguments, timeout / 1000);
+    return SkylarkExecutionResult.builder()
+        .addArguments(arguments).setTimeout(timeout / 1000).execute();
   }
 
   @SkylarkCallable(name = "execute", documented = false)
   public SkylarkExecutionResult execute(List<Object> arguments) throws EvalException {
-    return SkylarkExecutionResult.execute(arguments, 600000);
+    return SkylarkExecutionResult.builder()
+        .addArguments(arguments).setTimeout(600000).execute();
   }
 
   @SkylarkCallable(
@@ -362,11 +365,11 @@ public class SkylarkRepositoryContext {
     SkylarkPath outputPath = getPath("download()", output);
     try {
       checkInOutputDirectory(outputPath);
-      makeDirectories(outputPath.path);
-      HttpDownloader.download(
-          url, sha256, null, outputPath.getPath(), env.getListener(), osObject.getEnviron());
+      makeDirectories(outputPath.getPath());
+      HttpDownloader.download(url, sha256, null, outputPath.getPath(), env.getListener(),
+          osObject.getEnvironmentVariables());
       if (executable) {
-        outputPath.path.setExecutable(true);
+        outputPath.getPath().setExecutable(true);
       }
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
@@ -415,14 +418,15 @@ public class SkylarkRepositoryContext {
             + " <code>build_file</code>, this field can be used to strip it extracted"
             + " files."
   )
-  public void download_and_extract(
+  public void downloadAndExtract(
       String url, Object output, String sha256, String type, String stripPrefix)
       throws RepositoryFunctionException, InterruptedException, EvalException {
     // Download to outputDirectory and delete it after extraction
     SkylarkPath outputPath = getPath("download_and_extract()", output);
     checkInOutputDirectory(outputPath);
-    Path downloadedPath = HttpDownloader.download(
-        url, sha256, type, outputPath.getPath(), env.getListener(), osObject.getEnviron());
+    Path downloadedPath = HttpDownloader
+        .download(url, sha256, type, outputPath.getPath(), env.getListener(),
+            osObject.getEnvironmentVariables());
     DecompressorValue.decompress(
         DecompressorDescriptor.builder()
             .setTargetKind(rule.getTargetKind())
@@ -444,15 +448,15 @@ public class SkylarkRepositoryContext {
   }
 
   @SkylarkCallable(name = "download_and_extract", documented = false)
-  public void download_and_extract(String url, Object output, String type)
+  public void downloadAndExtract(String url, Object output, String type)
       throws RepositoryFunctionException, InterruptedException, EvalException {
-    download_and_extract(url, output, "", "", type);
+    downloadAndExtract(url, output, "", "", type);
   }
 
   @SkylarkCallable(name = "download_and_extract", documented = false)
-  public void download_and_extract(String url, Object output)
+  public void downloadAndExtract(String url, Object output)
       throws RepositoryFunctionException, InterruptedException, EvalException {
-    download_and_extract(url, output, "", "", "");
+    downloadAndExtract(url, output, "", "", "");
   }
 
   // This is just for test to overwrite the path environment
@@ -467,7 +471,7 @@ public class SkylarkRepositoryContext {
     if (pathEnv != null) {
       return pathEnv;
     }
-    String pathEnviron = osObject.getEnviron().get("PATH");
+    String pathEnviron = osObject.getEnvironmentVariables().get("PATH");
     if (pathEnviron == null) {
       return ImmutableList.of();
     }
@@ -487,7 +491,7 @@ public class SkylarkRepositoryContext {
         label = Label.create(label.getPackageIdentifier().makeAbsolute(),
             label.getName());
       } catch (LabelSyntaxException e) {
-        throw new IllegalStateException(e);  // Can't happen because the input label is valid
+        throw new AssertionError(e);  // Can't happen because the input label is valid
       }
     }
     SkyKey pkgSkyKey = PackageLookupValue.key(label.getPackageIdentifier());
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
index 08015a220e..e8a0538472 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
@@ -57,12 +57,12 @@ public class SkylarkRepositoryFunction extends RepositoryFunction {
   }
 
   /**
-   * Skylark repository context functions can use this function to notify the
+   * Skylark repository context functions can throw the result of this function to notify the
    * SkylarkRepositoryFunction that a dependency was missing and the evaluation of the function must
    * be restarted.
    */
-  static void restart() throws EvalException {
-    throw new SkylarkRepositoryMissingDependencyException();
+  static EvalException restart() {
+    return new SkylarkRepositoryMissingDependencyException();
   }
 
   private CommandEnvironment commandEnvironment = null;