Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dl: return the same exit status of the wrapped command #37266

Open
perillo opened this issue Feb 17, 2020 · 7 comments
Open

dl: return the same exit status of the wrapped command #37266

perillo opened this issue Feb 17, 2020 · 7 comments

Comments

@perillo
Copy link
Contributor

@perillo perillo commented Feb 17, 2020

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 toothrot added this to the Unreleased milestone Feb 18, 2020
@toothrot
Copy link
Contributor

@toothrot toothrot commented Feb 18, 2020

Sounds reasonable to me!

Loading

@toothrot toothrot changed the title dl: return the same exit status of the wrapped command x/dl: return the same exit status of the wrapped command Feb 18, 2020
@dmitshur dmitshur changed the title x/dl: return the same exit status of the wrapped command dl: return the same exit status of the wrapped command Feb 18, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 5, 2020

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

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Mar 5, 2020

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.

Loading

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Mar 5, 2020

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.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Mar 5, 2020

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.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 14, 2021

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

Thanks.

Loading

@perillo
Copy link
Contributor Author

@perillo perillo commented May 6, 2021

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

Thanks.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants