dl: return the same exit status of the wrapped command

[perillo]

Currently, the Run function in the internal/version and gotip packages returns the exit status 1 if the wrapped command fails:

if err := cmd.Run(); err != nil {
        // TODO: return the same exit status maybe.
        os.Exit(1)
    }
    os.Exit(0)

I'm developing a package that wraps various go commands, and I'm adding support for testing different versions of the go tool. Unfortunately one test that checks for the exit status 2 now fails.

I'm using this patched version locally:

diff --git a/gotip/main.go b/gotip/main.go
index 6338234..0a8f0db 100644
--- a/gotip/main.go
+++ b/gotip/main.go
@@ -57,9 +57,8 @@ func main() {
    }
    cmd.Env = dedupEnv(caseInsensitiveEnv, append(os.Environ(), "GOROOT="+root, "PATH="+newPath))
    if err := cmd.Run(); err != nil {
-       if _, ok := err.(*exec.ExitError); ok {
-           // TODO: return the same exit status maybe.
-           os.Exit(1)
+       if err, ok := err.(*exec.ExitError); ok {
+           os.Exit(err.ExitCode())
        }
        log.Fatalf("gotip: failed to execute %v: %v", gobin, err)
    }
diff --git a/internal/version/version.go b/internal/version/version.go
index a63c649..349bd6e 100644
--- a/internal/version/version.go
+++ b/internal/version/version.go
@@ -61,7 +61,9 @@ func Run(version string) {
    }
    cmd.Env = dedupEnv(caseInsensitiveEnv, append(os.Environ(), "GOROOT="+root, "PATH="+newPath))
    if err := cmd.Run(); err != nil {
-       // TODO: return the same exit status maybe.
+       if err, ok := err.(*exec.ExitError); ok {
+           os.Exit(err.ExitCode())
+       }
        os.Exit(1)
    }
    os.Exit(0)

[toothrot]

Sounds reasonable to me!

[gopherbot]

Change https://golang.org/cl/221978 mentions this issue: dl: exit with the exit code returned by cmd.Run

[perillo]

In https://golang.org/cl/221978 I have added the internal/compat package.

If the CL is approved, there are two functions, duplicated in gotip and internal/version packages, that can be moved there: homedir and dedup. The dedup function also have duplicate tests.

[zikaeroh]

I opened #37037 and have a CL chain for dl that merges gotip with version. I've been waiting a month for review, but feasibly everything could live in internal/version and avoid the extra package and simplify your CL.

[perillo]

In the end, excluding homedir and dedup, the remaining duplicate code is const caseInsensitiveEnv, func exe, func goroot and the main function.

I'm not against your CL, but I think having a compat package for functions that have compatibility problems makes the code more readable.

[perillo]

I have updated https://golang.org/cl/221978. In addition to rebasing, I have also added the new go:build directive.

Thanks.

[perillo]

@dmitshur, when you have time can you review https://golang.org/cl/221978? I think that it is ready to be merged.

Thanks.

[perillo]

@dmitshur, now that the dl package requires go 1.18, using `exec.ExitError should be ok. I have updated my CL.

Thanks.