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

Give a helpful error if an unsupported Go version is used #121

Closed
mvdan opened this issue Sep 6, 2020 · 6 comments · Fixed by #136 or #140
Closed

Give a helpful error if an unsupported Go version is used #121

mvdan opened this issue Sep 6, 2020 · 6 comments · Fixed by #136 or #140

Comments

@mvdan
Copy link
Member

mvdan commented Sep 6, 2020

As suggested by @pagran in https://github.com/mvdan/garble/pull/116#issuecomment-687664313. We could call go version and check that it's newer.

Extra care should be taken with master builds, since they won't follow a semver-like version number. For example, my current build from yesterday, which is signifiantly newer than 1.15:

$ go version
go version devel +bf833ead62 Sat Sep 5 14:11:21 2020 +0000 linux/amd64

For these dev builds, it should be enough to check that their timestamp is newer than the time of the Go 1.15 release.

@lu4p
Copy link
Member

lu4p commented Sep 6, 2020

I don't know if we would overstep some boundaries with this (prompt the user before installing), but we could also just use go to install a currently supported version of go, and then use that.

https://golang.org/doc/manage-install

@mvdan
Copy link
Member Author

mvdan commented Sep 6, 2020

I think that's too far. If someone is using garble, they need to know how to install a recent Go version. If they can't manage that, they should probably not be obfuscating their Go builds.

@mvdan
Copy link
Member Author

mvdan commented Sep 6, 2020

Also, I think we could print a warning if the version is too new and we aren't testing against it. Because right now, using garble with 1.16 (master) also errors with confusing output, but we could prefix the output with a helpful warning.

@pagran
Copy link
Member

pagran commented Sep 6, 2020

I suggest adding 3 messages.

If the output of go version has an unexpected format - "You are using an unsupported version of go (devel) "

If go version is too old - "Your goXXX version is outdated, upgrade to goNNN"

If go version is too new - "Your goXXX version is not supported yet, please update garble"

pagran added a commit that referenced this issue Sep 13, 2020
@mvdan
Copy link
Member Author

mvdan commented Sep 21, 2020

I'm surprised that this was approved and closed without any tests. It's easy to place a mock go in $PATH, like a tiny shell script that just echoes a specific version.

I'm reopening this to track adding some tests. Otherwise this feature will likely break the next time we touch the code, which is going to be every six months.

@pagran
Copy link
Member

pagran commented Sep 21, 2020

Got it, my mistake. I will add tests soon

lu4p pushed a commit that referenced this issue Sep 21, 2020
See #121 (comment).

In some rare cases, it's nearly impossible to write a test for a change,
but they are truly so rare that we shouldn't give any ideas here.

By default, all contributors should try to write a test for every
change that changes what the code is meant to do.
pagran added a commit that referenced this issue Sep 22, 2020
Add tests for Go version checking

Fix panic if go version has invalid format

Fixes: #121
Co-authored-by: Daniel Martí <mvdan@mvdan.cc>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants