fix doc comments and error messages

Author: amkartashov

cmd/rbe_configs_gen/rbe_configs_gen.go

@@ -36,7 +36,7 @@ var (
 	// Optional input arguments.
 	runner = flag.String("runner", "", "Runner (host|docker) to use to generate configs. Defaults to docker for linux|windows, host for osx.")
 	// toolchainContainer is required option for runner=docker
-	toolchainContainer = flag.String("toolchain_container", "", "Repository path to toolchain image to generate configs for. E.g., l.gcr.io/google/rbe-ubuntu16-04:latest. Required if runner=docker, ignored if runner=host.")
+	toolchainContainer = flag.String("toolchain_container", "", "Repository path to toolchain image to generate configs for. E.g., l.gcr.io/google/rbe-ubuntu16-04:latest. Required if runner=docker.")
 	bazelVersion = flag.String("bazel_version", "", "(Optional) Bazel release version to generate configs for. E.g., 4.0.0. If unspecified, the latest available Bazel release is picked.")
 
 	// Arguments affecting output generation not specific to either C++ or Java Configs.

pkg/options/options.go

@@ -12,8 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 //
-// Package rbeconfigsgen contains utilities to generate C++ & Java Toolchain configs for Bazel to be
-// used to run RBE builds
+// Package options describes cli options for rbe_configs_gen
 package options
 
 import (
@@ -211,6 +210,7 @@ var (
 				"BAZEL_COMPILER":      "clang",
 				"BAZEL_TARGET_CPU":    "darwin_x86_64",
 				"CC":                  "clang",
+				"CC_TOOLCHAIN_NAME":   "darwin_x86_64",
 			},
 			CPPToolchainTargetName: "cc-compiler-darwin_x86_64",
 		},

pkg/rbeconfigsgen/rbeconfigsgen.go

@@ -157,7 +157,7 @@ type outputConfigs struct {
 	// licence will contain the OSS license applicable for the generated configs.
 	license generatedFile
 	// cppConfigsTarball is the path to the tarball file containing the C++ configs generated by
-	// Bazel inside the toolchain container.
+	// Bazel inside the runner.
 	cppConfigsTarball string
 	// configBuild represents the BUILD file containing the C++ crosstool top toolchain target
 	// and the default platform definition.
@@ -181,8 +181,8 @@ func BazeliskDownloadInfo(os string) (string, string, error) {
 }
 
 // installBazelisk downloads bazelisk locally to the specified directory for the given os and copies
-// it into the running toolchain container.
-// Returns the path Bazelisk was installed to inside the running toolchain container.
+// it into the runner.
+// Returns the path Bazelisk was installed to inside the runner.
 func installBazelisk(r runner.Runner, downloadDir, execOS string) (string, error) {
 	url, filename, err := BazeliskDownloadInfo(execOS)
 	if err != nil {
@@ -210,7 +210,7 @@ func installBazelisk(r runner.Runner, downloadDir, execOS string) (string, error
 	}
 
 	if _, err := r.ExecCmd("chmod", "+x", bazeliskRunnerPath); err != nil {
-		return "", fmt.Errorf("failed to mark the Bazelisk binary as executable inside the container: %w", err)
+		return "", fmt.Errorf("failed to mark the Bazelisk binary as executable inside the runner: %w", err)
 	}
 	return bazeliskRunnerPath, nil
 }
@@ -243,10 +243,10 @@ func appendCppEnv(env map[string]string, o *options.Options) (map[string]string,
 	return env, nil
 }
 
-// genCppConfigs generates C++ configs inside the running toolchain container represented by the
-// given docker runner according to the given options. bazeliskPath is the path to the bazelisk
-// binary inside the running toolchain container.
-// The return value is the path to the C++ configs tarball copied out of the toolchain container.
+// genCppConfigs generates C++ configs inside the runner represented by the
+// given runner according to the given options. bazeliskPath is the path to the bazelisk
+// binary inside the runner.
+// The return value is the path to the C++ configs tarball copied out of the runner.
 func genCppConfigs(r runner.Runner, o *options.Options, bazeliskPath string) (string, error) {
 	if !o.GenCPPConfigs {
 		return "", nil
@@ -256,7 +256,7 @@ func genCppConfigs(r runner.Runner, o *options.Options, bazeliskPath string) (st
 	// command we run in this function.
 	cppProjDir := path.Join(r.GetWorkdir(), "cpp_configs_project")
 	if _, err := r.ExecCmd("mkdir", cppProjDir); err != nil {
-		return "", fmt.Errorf("failed to create empty directory %q inside the toolchain container: %w", cppProjDir, err)
+		return "", fmt.Errorf("failed to create empty directory %q inside the runner: %w", cppProjDir, err)
 	}
 	oldWorkDir := r.GetWorkdir()
 	if err := r.SetWorkdir(cppProjDir); err != nil {
@@ -265,7 +265,7 @@ func genCppConfigs(r runner.Runner, o *options.Options, bazeliskPath string) (st
 	defer r.SetWorkdir(oldWorkDir)
 
 	if _, err := r.ExecCmd("touch", "WORKSPACE", "BUILD.bazel"); err != nil {
-		return "", fmt.Errorf("failed to create empty build & workspace files in the container to initialize a blank Bazel repository: %w", err)
+		return "", fmt.Errorf("failed to create empty build & workspace files in the runner to initialize a blank Bazel repository: %w", err)
 	}
 
 	// Backup the current environment & restore it before returning.
@@ -280,24 +280,24 @@ func genCppConfigs(r runner.Runner, o *options.Options, bazeliskPath string) (st
 	// logs.
 	generationEnv, err := appendCppEnv(bazeliskEnv, o)
 	if err != nil {
-		return "", fmt.Errorf("failed to add additional environment variables to the C++ config generation docker command: %w", err)
+		return "", fmt.Errorf("failed to add additional environment variables to the C++ config generation command: %w", err)
 	}
 	r.SetAdditionalEnv(generationEnv)
 
 	args := []string{o.CppBazelCmd}
 	args = append(args, o.CPPConfigTargets...)
 	if _, err := r.ExecCmd(bazeliskPath, args...); err != nil {
-		return "", fmt.Errorf("Bazel was unable to build the C++ config generation targets in the toolchain container: %w", err)
+		return "", fmt.Errorf("Bazel was unable to build the C++ config generation targets in the runner: %w", err)
 	}
 
 	// Restore the env needed for Bazelisk.
 	r.SetAdditionalEnv(bazeliskEnv)
 	bazelOutputRoot, err := r.ExecCmd(bazeliskPath, "info", "output_base")
 	if err != nil {
-		return "", fmt.Errorf("unable to determine the build output directory where Bazel produced C++ configs in the toolchain container: %w", err)
+		return "", fmt.Errorf("unable to determine the build output directory where Bazel produced C++ configs in the runner: %w", err)
 	}
 	cppConfigDir := path.Join(bazelOutputRoot, "external", o.CPPConfigRepo)
-	log.Printf("Extracting C++ config files generated by Bazel at %q from the toolchain container.", cppConfigDir)
+	log.Printf("Extracting C++ config files generated by Bazel at %q from the runner.", cppConfigDir)
 
 	// Restore the old env now that we're done with Bazelisk commands. This is purely to reduce
 	// noise in the logs.
@@ -306,7 +306,7 @@ func genCppConfigs(r runner.Runner, o *options.Options, bazeliskPath string) (st
 	// 1. Get a list of symlinks in the config output directory.
 	// 2. Harden each link.
 	// 3. Archive the contents of the config output directory into a tarball.
-	// 4. Copy the tarball from the container to the local temp directory.
+	// 4. Copy the tarball from the runner to the local temp directory.
 	var out string
 	if o.ExecOS == "windows" {
 		out, err = r.ExecCmd("cmd", "/r", "dir", filepath.Clean(cppConfigDir), "/a:l", "/b")
@@ -332,7 +332,7 @@ func genCppConfigs(r runner.Runner, o *options.Options, bazeliskPath string) (st
 		}
 		resolvedPath, err := r.ExecCmd("readlink", s)
 		if err != nil {
-			return "", fmt.Errorf("unable to determine what the symlink %q in %q in the toolchain container points to: %w", s, cppConfigDir, err)
+			return "", fmt.Errorf("unable to determine what the symlink %q in %q in the runner points to: %w", s, cppConfigDir, err)
 		}
 		if _, err := r.ExecCmd("ln", "-f", resolvedPath, s); err != nil {
 			return "", fmt.Errorf("failed to harden symlink %q in %q pointing to %q: %w", s, cppConfigDir, resolvedPath, err)
@@ -342,11 +342,11 @@ func genCppConfigs(r runner.Runner, o *options.Options, bazeliskPath string) (st
 	outputTarball := "cpp_configs.tar"
 	// Explicitly use absolute paths to avoid confusion on what's the working directory.
 	outputTarballPath := path.Join(o.TempWorkDir, outputTarball)
-	outputTarballContainerPath := path.Join(cppProjDir, outputTarball)
-	if _, err := r.ExecCmd("tar", "-cf", outputTarballContainerPath, "-C", cppConfigDir, "."); err != nil {
+	outputTarballRunnerPath := path.Join(cppProjDir, outputTarball)
+	if _, err := r.ExecCmd("tar", "-cf", outputTarballRunnerPath, "-C", cppConfigDir, "."); err != nil {
 		return "", fmt.Errorf("failed to archive the C++ configs into a tarball inside the toolchain container: %w", err)
 	}
-	if err := r.CopyFrom(outputTarballContainerPath, outputTarballPath); err != nil {
+	if err := r.CopyFrom(outputTarballRunnerPath, outputTarballPath); err != nil {
 		return "", fmt.Errorf("failed to copy the C++ config tarball out of the toolchain container: %w", err)
 	}
 	log.Printf("Generated C++ configs at %s.", outputTarballPath)
@@ -368,20 +368,20 @@ func UsesLocalJavaRuntime(bazelVersion string) (bool, error) {
 
 // genJavaConfigs returns a BUILD file containing a Java toolchain rule definition that contains
 // the following attributes determined by probing details about the JDK version installed in the
-// running toolchain container.
-// 1. Value of the JAVA_HOME environment variable set in the toolchain image.
+// runner.
+// 1. Value of the JAVA_HOME environment variable set in the runner or as reported by java binary.
 // 2. Value of the Java version as reported by the java binary installed in JAVA_HOME inside the
-//    running toolchain container.
+//    runner.
 func genJavaConfigs(r runner.Runner, o *options.Options) (generatedFile, error) {
 	if !o.GenJavaConfigs {
 		return generatedFile{}, nil
 	}
 	javaBin := "java"
-	imageEnv, err := r.GetEnv()
+	runnerEnv, err := r.GetEnv()
 	if err != nil {
 		return generatedFile{}, fmt.Errorf("unable to get the environment of the toolchain image to determine JAVA_HOME: %w", err)
 	}
-	javaHome, ok := imageEnv["JAVA_HOME"]
+	javaHome, ok := runnerEnv["JAVA_HOME"]
 	if ok && len(javaHome) > 0 {
 		log.Printf("JAVA_HOME was %q.", javaHome)
 		javaBin = path.Join(javaHome, "bin/java")
@@ -392,7 +392,7 @@ func genJavaConfigs(r runner.Runner, o *options.Options) (generatedFile, error)
 	// some non-deterministic prefix.
 	out, err := r.ExecCmd(javaBin, "-XshowSettings:properties", "-version")
 	if err != nil {
-		return generatedFile{}, fmt.Errorf("unable to determine the Java version installed in the toolchain container: %w", err)
+		return generatedFile{}, fmt.Errorf("unable to determine the Java version installed in the runner: %w", err)
 	}
 	javaVersion := ""
 	for _, line := range strings.Split(out, "\n") {
@@ -412,9 +412,13 @@ func genJavaConfigs(r runner.Runner, o *options.Options) (generatedFile, error)
 		}
 	}
 	if len(javaVersion) == 0 {
-		return generatedFile{}, fmt.Errorf("unable to determine the java version installed in the container by running 'java -XshowSettings:properties' in the container because it didn't return a line that looked like java.version = <version>")
+		return generatedFile{}, fmt.Errorf("unable to determine the java version installed in the runner by running 'java -XshowSettings:properties' because it didn't return a line that looked like java.version = <version>")
 	}
 	log.Printf("Java version: '%s'.", javaVersion)
+	if len(javaHome) == 0 {
+		return generatedFile{}, fmt.Errorf("unable to determine the JAVA_HOME installed in the runner: not in env, not returned as line `java.home = <path>` by 'java -XshowSettings:properties'")
+	}
+	log.Printf("Java home: '%s'.", javaHome)
 
 	usesNewJavaRule, err := UsesLocalJavaRuntime(o.BazelVersion)
 	if err != nil {
@@ -770,10 +774,11 @@ func createManifest(o *options.Options) error {
 	}
 	// Extract the sha256 digest from the image name to be included in the manifest.
 	s := imageDigestRegexp.FindStringSubmatch(o.PlatformParams.ToolchainContainer)
-	if len(s) != 2 {
+	if len(s) != 2 && o.Runner == "docker" {
 		return fmt.Errorf("failed to extract sha256 digest using regex from image name %q, got %d substrings, want 2", o.PlatformParams.ToolchainContainer, len(s))
+	} else if len(s) == 2 {
+		m.ImageDigest = s[1]
 	}
-	m.ImageDigest = s[1]
 	// Include the sha256 digest of the configs tarball if output tarball generation was enabled by
 	// actually hashing the contents of the output tarball.
 	if len(o.OutputTarball) != 0 {

pkg/runner/host_runner.go

@@ -39,8 +39,8 @@ type hostRunner struct {
 	additionalEnv map[string]string
 }
 
-// NewHostRunner creates a new Runner which executes commands directly in host environment. deleteWorkdir
-// determines if the Cleanup function on the hostRunner will remove temporary directory
+// NewHostRunner creates a new Runner which executes commands directly on the host where rbe_configs_gen runs.
+// deleteWorkdir determines if the Cleanup function of the hostRunner will remove temporary directory
 func NewHostRunner(deleteWorkdir bool) (*hostRunner, error) {
 
 	workdir, err := ioutil.TempDir("", "host_runner_")
@@ -55,7 +55,7 @@ func NewHostRunner(deleteWorkdir bool) (*hostRunner, error) {
 	}, nil
 }
 
-// execCmd runs the given command and returns the output with whitespace
+// ExecCmd runs the given command and returns the output with whitespace
 // trimmed from the edges.
 func (r *hostRunner) ExecCmd(cmd string, args ...string) (string, error) {
 	log.Printf("Running: %s", strings.Join(append([]string{cmd}, args...), " "))
@@ -71,7 +71,7 @@ func (r *hostRunner) ExecCmd(cmd string, args ...string) (string, error) {
 	return strings.TrimSpace(string(o)), err
 }
 
-// cleanup stops the running container if stopContainer was true when the hostRunner was created.
+// Cleanup deletes runner temporary files if deleteWorkdir was true when the hostRunner was created.
 func (r *hostRunner) Cleanup() {
 	if !r.deleteWorkdir {
 		log.Printf("Not deleting workdir %v because the Cleanup option was set to false.", r.globalWorkdir)
@@ -83,29 +83,28 @@ func (r *hostRunner) Cleanup() {
 	}
 }
 
-// copyTo copies the local file at 'src' to the container where 'dst' is the path inside
-// the container. d.workdir has no impact on this function.
+// CopyTo copies the local file at 'src' to 'dst' on the host
+// CopyTo works the same way as CopyFrom for the host runner
 func (r *hostRunner) CopyTo(src, dst string) error {
 	if _, err := runCmd("cp", src, dst); err != nil {
 		return err
 	}
 	return nil
 }
 
-// copyFrom extracts the file at 'src' from inside the container and copies it to the path
-// 'dst' locally. d.workdir has no impact on this function.
+// CopyFrom copies the local file at 'src' to 'dst' on the host
+// CopyFrom works the same way as CopyTo for the host runner
 func (r *hostRunner) CopyFrom(src, dst string) error {
 	if _, err := runCmd("cp", src, dst); err != nil {
 		return err
 	}
 	return nil
 }
 
-// getEnv gets the shell environment values from the toolchain container as determined by the
-// image config. Env value set or changed by running commands after starting the container aren't
+// GetEnv gets the shell environment values from the host.
+// Env values set or changed by running commands inside the runner aren't
 // captured by the return value of this function.
-// The return value of this function is a map from env keys to their values. If the image config,
-// specifies the same env key multiple times, later values supercede earlier ones.
+// The return value of this function is a map from env keys to their values.
 func (r *hostRunner) GetEnv() (map[string]string, error) {
 	result := make(map[string]string)
 	for _, s := range os.Environ() {