all: Use Go module machinery to book-keep external tools #2430
Conversation
2opremio
force-pushed
the
dependencies-go-mod
branch
3 times, most recently
from
9f18c2c to
17c7729
Compare
2opremio
force-pushed
the
dependencies-go-mod
branch
from
17c7729 to
d94b05f
Compare
There was a problem hiding this comment.
I think in regards to this becoming a common practice, I'd agree, but I don't think it's yet a general best practice. It comes with some tradeoffs, just like the approach we're currently using does too, so it just depends which tradeoffs are more valuable or less problematic for us.
An upside of this approach not mentioned above is it guarantees we're running the same versions of these tools locally. Our existing code doesn't guarantee this even though it installs specific versions of most of the tools.
The main tradeoff is stellar/go's module will expand it's dependency graph to include all the dependencies of these tools and will set minimum versions for those dependencies, which is unfortunate given none of our code is dependent on them. Anyone importing a stellar/go package will be constrained by those new minimum versions in their own dependency graph.
If this was a monorepo containing only applications I think the approach in this PR would have the least tradeoffs because it would impact only the applications inside the repo.
Because this monorepo also contains our Go SDK we should weigh if the benefits of this change are worth the continued expansion of the graph.
The dependencies we're adding are:
github.com/google/renameio + v0.1.0 https://github.com/google/renameio
github.com/kevinburke/go-bindata + v3.18.0 https://github.com/kevinburke/go-bindata
github.com/kr/pty + v1.1.1 https://github.com/kr/pty
github.com/rogpeppe/go-internal + v1.3.0 https://github.com/rogpeppe/go-internal
golang.org/x/mod + 4bf6d317e70e https://golang.org/x/mod
golang.org/x/xerrors + a985d3407aa7 https://golang.org/x/xerrors
gopkg.in/errgo.v2 + v2.1.0 https://gopkg.in/errgo.v2
The dependencies being changed are:
github.com/kr/text e373e137fafd > v0.1.0 https://github.com/kr/text/compare/e373e137fafd...v0.1.0
golang.org/x/tools 70d37148ca0c > 6e064ea0cf2d https://github.com/golang/tools/compare/70d37148ca0c...6e064ea0cf2d
github.com/kr/pretty e6ac2fc51e89 > v0.1.0 https://github.com/kr/pretty/compare/e6ac2fc51e89...v0.1.0
honnef.co/go/tools c2f93a96b099 > v0.0.1-2020.1.3
Another thing to consider is that when/if we move to multiple modules how we run the tools will need to change and it's not completely clear to me how we'd structure it since the simple go run ... won't work anymore. Would the plan be to have multiple tools.go, one for each project?
I don't think this adds enough benefits for the complications it adds to multiple modules, but if there's consensus that this approach is preferred I'm not opposed to it, we should just be aware of the trade-offs:
- Dependencies in the graph that are never used by the code.
- Something additional to unwind/figure out with multiple modules.
### What When encountering a version that doesn't conform to the standard semver version format (vX.Y.Z) or the format that Go uses for tagless versions (v0.0.0-date-hash), fallback to just using the raw version name as the version. ### Why While most developers use either semver tags for their versions, or no tags, some developers use non-standard formats for their tags and when we encounter these we should just use this as the version and not try to parse commit hashes out of them. The code is currently trying to parse a commit hash (the third component) out of `v0.0.1-2020.1.3` and raising an error because there is no third component. This bug was discovered when using `golistcmp` on the `go list -m -json all` output for the dependencies in stellar/go#2430.
|
@leighmcculloch are we planning to break out the code base into different modules? If so, do you know when and where? Also, regardless of this PR, the Go SDK should be its own module, so that it doesn't drag all the other dependencies. Once that is done we can evaluate whether it makes sense to push this trick to the new modules or roll it back. Until then (based on the approvals in this PR) I think there is consensus on moving forward with this approach. |
|
There are plans to do it yup, see go modules part 1 and part 2 documents. We've done part 1, haven't done part 2 yet. I think it is worth considering this after part 2 if the tools.go can live in its own module so that it doesn't pollute any other module. In fact you could probably introduce just that one module now independent of part 2. |
|
@2opremio Yeah this is a plan we've had for a while but haven't had bandwidth to execute. I'd love it if you took some time to read through the links @leighmcculloch shared and then we can discuss what we want to do about it, and when. For now I think we should pause this PR to keep the Go SDK deps frozen. Happy to discuss early next week. |
|
I still need to look at this, but I think we can close it |
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Track and maintain external tooling through Go's module machinery.
For that, I am using a
tools.gofile, which seems to be an increasingly-adopted practice.Why
go run, making things work transparently (e.g. it took me a while to find out that the specific version ofgo-bindatato use was buried ingogenerate.sh)Known limitations
go runis slower than running a binary (at least until golang/go#33468 is resolved), so we need to be careful on where to use it (e.g. theshadowbinary is installed becausego runis too slow).