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

require a compatible Go version to have built the garble binary #269

Closed
mvdan opened this issue Mar 7, 2021 · 1 comment · Fixed by #508
Closed

require a compatible Go version to have built the garble binary #269

mvdan opened this issue Mar 7, 2021 · 1 comment · Fixed by #508
Labels
enhancement New feature or request

Comments

@mvdan
Copy link
Member

mvdan commented Mar 7, 2021

Right now, on the master branch, we only test Go 1.16 builds with a garble built on Go 1.16.

Would Go 1.16 builds work on a garble built with Go 1.15?

What about Go 1.16 builds on a garble built with Go tip/master (1.17)?

You might think "it should be fine", but it's not always the case. I've encountered some rare failures because I did go install on 1.17 and then built some packages with garble on 1.16, and those failures go away if I consistently use 1.16. I think the underlying cause is a change of behavior in go/types, affecting our decision to obfuscate certain package paths.

I think we should require the major version to be the same. That is, if the user built garble on 1.15, they can't use it for builds on 1.16, and vice versa.

I think it should be fine to use a different minor version, such as building garble on Go 1.16 and then using it for an obfuscated build on Go 1.16.1. Major versions change lots of features and internals, but minor versions usually only fix security and other critical bugs, which are far less likely to affect garble.

Option B: rejecting this idea

Another option is to say "no" and explicitly support and test version mismatches. For example, if garble v0.2.0 supports Go 1.16.x and Go 1.17.x, we must test 2x2 combinations on CI:

  • install garble on 1.16.x, test on 1.16.x
  • install garble on 1.16.x, test on 1.17.x
  • install garble on 1.17.x, test on 1.16.x
  • install garble on 1.17.x, test on 1.17.x

However, note that this can multiply our CI cost, and we'd also have to worry about other cases such as "install garble on 1.15.x" even if we don't support that Go version at all - just because it happens to successfully build.

Personal preference

I feel fairly strongly about only supporting the same major Go version. It's the only scenario we've been testing, and I think doing the opposite would be added work and complexity just for the benefit of user convenience. It is more convenient to not have to rebuild when switching Go versions, or to download a single prebuilt binary that will work for multiple Go versions, but I reckon the most common use case is using a version of garble with a single major version of Go.

Plus, I think this is how most Go tools work nowadays. I doubt that a gopls built on Go 1.15.x and used for developing code on Go 1.16.x is an edge case that is supported or well tested. This is a tool for developing Go code, so it is expected that the end user has Go installed already; they should use it to build garble too.

Supporting major version mismatches is also a bad idea for other reasons. For instance, a garble built on Go 1.16.x can't support builds on Go 1.17.x if said build uses a language feature introduced in Go 1.17.x, as garble would use packages like go/parser and go/types from Go 1.16.x, which would not know about the new feature yet.

That last nasty edge case could be solved by only allowing a newer garble build to use older versions of Go for builds, but that still leaves us with the other possible failures like the one I encountered with go/types.

Implementation

Option A is to make the build fail if the major version of Go does not match the currently supported version. We could accomplish this with build tags, like // +build go1.16 and // +build !go1.17. GopherJS does this with build-tagged constants, I think. The main downside is that build errors are pretty confusing.

Option B is to use https://golang.org/pkg/runtime/#Version when garble runs and match it against the supported Go version, just like we do for go version. This won't fail until the first time the user calls garble build, but we can provide a helpful error message and the implementation will be simpler.

I lean towards option B.

@mvdan mvdan added enhancement New feature or request help wanted Extra attention is needed labels Mar 7, 2021
@mvdan mvdan added this to the v0.2.0 milestone Mar 15, 2021
@mvdan
Copy link
Member Author

mvdan commented Apr 1, 2021

I think the underlying cause is a change of behavior in go/types, affecting our decision to obfuscate certain package paths.

That particular code went away with 748c6a0; see the removed implementedOutsideGo.

I'm inclined to sit on this for now, because maybe I'm being overly dramatic. If we don't do hacks that assume internal and undocumented details, like implementedOutsideGo, it should generally be fine to use a slightly older or newer version of Go to build garble.

For instance, a garble built on Go 1.16.x can't support builds on Go 1.17.x if said build uses a language feature introduced in Go 1.17.x, as garble would use packages like go/parser and go/types from Go 1.16.x, which would not know about the new feature yet.

I suspect that this edge case is what will be most common around users, so we might want to add a check for it in the future. It wouldn't need to be as strict as "the major version must be an exact match", but rather "the major version used to build garble must not be older than the version of Go currently being used".

@mvdan mvdan removed the help wanted Extra attention is needed label Apr 1, 2021
@mvdan mvdan removed this from the v0.2.0 milestone Apr 1, 2021
mvdan added a commit to mvdan/garble-fork that referenced this issue Mar 21, 2022
For instance, Go 1.18 added support for generics, so its compiler output
files changed format to accomodate for the new language feature.
If garble is built with Go 1.17 and then used to perform builds on Go
1.18, it will fail in a very confusing way, because garble's go/types
and go/importer packages will not know how to deal with that.

As already discussed in burrowers#269, require the version that built the garble
binary to be equal or newer. In that thread we discussed only comparing
the major version, so for example garble built on go1.18 could be used
on the toolchain go1.18.5. However, that could still fail in confusing
ways if a fix to go/types or go/importer happened in a point release.

Fixes burrowers#269.
@mvdan mvdan changed the title require the same major Go version to have built garble require a compatible Go version to have built the garble binary Mar 21, 2022
mvdan added a commit to mvdan/garble-fork that referenced this issue Mar 21, 2022
For instance, Go 1.18 added support for generics, so its compiler output
files changed format to accomodate for the new language feature.
If garble is built with Go 1.17 and then used to perform builds on Go
1.18, it will fail in a very confusing way, because garble's go/types
and go/importer packages will not know how to deal with that.

As already discussed in burrowers#269, require the version that built the garble
binary to be equal or newer. In that thread we discussed only comparing
the major version, so for example garble built on go1.18 could be used
on the toolchain go1.18.5. However, that could still fail in confusing
ways if a fix to go/types or go/importer happened in a point release.

While here, I noticed that we were still using Go 1.17 for some CI
checks. Fix that.

Fixes burrowers#269.
mvdan added a commit to mvdan/garble-fork that referenced this issue Mar 21, 2022
For instance, Go 1.18 added support for generics, so its compiler output
files changed format to accomodate for the new language feature.
If garble is built with Go 1.17 and then used to perform builds on Go
1.18, it will fail in a very confusing way, because garble's go/types
and go/importer packages will not know how to deal with that.

As already discussed in burrowers#269, require the version that built the garble
binary to be equal or newer. In that thread we discussed only comparing
the major version, so for example garble built on go1.18 could be used
on the toolchain go1.18.5. However, that could still fail in confusing
ways if a fix to go/types or go/importer happened in a point release.

While here, I noticed that we were still using Go 1.17 for some CI
checks. Fix that, except for staticcheck.

Fixes burrowers#269.
@lu4p lu4p closed this as completed in #508 Mar 22, 2022
lu4p pushed a commit that referenced this issue Mar 22, 2022
For instance, Go 1.18 added support for generics, so its compiler output
files changed format to accomodate for the new language feature.
If garble is built with Go 1.17 and then used to perform builds on Go
1.18, it will fail in a very confusing way, because garble's go/types
and go/importer packages will not know how to deal with that.

As already discussed in #269, require the version that built the garble
binary to be equal or newer. In that thread we discussed only comparing
the major version, so for example garble built on go1.18 could be used
on the toolchain go1.18.5. However, that could still fail in confusing
ways if a fix to go/types or go/importer happened in a point release.

While here, I noticed that we were still using Go 1.17 for some CI
checks. Fix that, except for staticcheck.

Fixes #269.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

1 participant