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

Jump to bottom

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

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

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

perillo opened this issue Feb 17, 2020 · 8 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unreleased

Comments

@perillo
Copy link
Contributor

perillo commented Feb 17, 2020 •
edited

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 the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 18, 2020
@toothrot toothrot added this to the Unreleased milestone Feb 18, 2020
@toothrot
Copy link
Contributor

Sounds reasonable to me!

@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

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

@perillo
Copy link
Contributor Author

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.

@zikaeroh
Copy link
Contributor

zikaeroh commented Mar 5, 2020 •
edited

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
Copy link
Contributor Author

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.

@perillo
Copy link
Contributor Author

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.

@perillo
Copy link
Contributor Author

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.

@perillo
Copy link
Contributor Author

perillo commented Jan 2, 2024

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

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@toothrot @perillo @gopherbot @zikaeroh