-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
cmd/go: clarify best practice for tool dependencies #25922
Comments
Let's take a step back: what do you mean by “tool dependency”? The |
Roughly:
Correct, this would only be a dependency for the authors of the module (as you point out, the users of our module probably don't care whether or not we've used As I understand it, these tool dependencies do not introduce any additional requirements for users of our module: they (the tool dependencies) can only ever be referenced by running I'm using the term "context" here to mean "the |
I haven't played around with vgo much yet or looked under the hood. I know a lot of package managers handle this by sorting dependencies into buckets: regular dependencies, test dependencies, dev dependencies. That seems like overkill and there can also be dependencies by build tag, like, say, importing a module for internal use but only on windows, and dependencies may only be for a certain package in a module that you may not be using. Could vgo record all the dependencies for a project—for all build tags, for all packages, for tests, for development—but lazily fetch them as needed? (This may require more invasive integration but it would be an optimization as the semantics are the same for lazy-fetch and fetch-everything-upfront) |
My understanding is that this is exactly what And, to your earlier point, I agree it means the delineation between the buckets is unnecessary (for anyone wanting to know why a dependency exists, Just to clarify one point above. My proposal for |
A couple of observations:
To me, those suggest a fairly straightforward solution:
In contrast, if you are building a custom version of the tool, then you should be able to customize it using blank-imports as you describe. |
@bcmills that sounds good but it would be awkward to write that each time and easy for things to go out of sync and you have some code using 1.11.0 and some using 1.12.3 but probably want . go generate let's you define aliases with Perhaps a more vgo-aware variant would be helpful, like
which is module scoped. It falls somewhere between an ephemeral There are a some details to iron out in such an approach, of course. |
Completely. As you say, this can either be solved entirely orthogonally, or with Go wrappers (wrappers that could even take care of installing the right version).
I'm not entirely sure I follow your point here. The point I was trying to make is that where (i.e. in what directory) you run
As @jimmyfrasche pointed out this has the major downside of littering the required version across all the places you require Just one point to note about the "proposal":
Despite the explanation in #25416, I think |
Running a tool at a specific version depending on the module would have to go through vgo. Running stringer lets $SHELL pick which stringer to run, so it would need to be something like Something like that could be stuffed into
An IDE/editor could use tool dependencies to figure out if this is a tool dependency or a regular program and run it through vgo if need be. From the command line, though, it would be up to the user to know whether to run A tool module could have multiple commands. Look up would be slow if these weren't included somewhere in the metadata. A tool module could have multiple dependencies, some shared with the module under development. Unlike with other dependencies, you probably don't want those to be solved together: using foobar shouldn't affect your version baz or the version of baz used by other tools you depend on. They should all be siloed. This is all starting to sound mostly unrelated to vgo but perhaps it's something that can be built on top of vgo. Going back on what I said about buckets, let's say there was as separate tool
Is there anything that would need to change in vgo to allow something like this? A downside of not having it integrated in vgo itself would be that |
What is the decision to be made here? |
Indeed, we don't need to "solve" this now.
For Go 1.11
For later:
|
Adding another comment to this thread to give the equivalent, and current, This is a cd $(mktemp -d)
mkdir hello
cd hello
go mod init example.com/hello
# Either rely on GOBIN=GOPATH/bin or set it explicitly
export GOBIN=$PWD/bin
# add a dependency on golang.org/x/tools/cmd/stringer
# the build constraint ensures this file is ignored
cat <<EOD > tools.go
// +build tools
package tools
import (
_ "golang.org/x/tools/cmd/stringer"
)
EOD
go install golang.org/x/tools/cmd/stringer
cat go.mod results in:
|
Sorry, this is actually working. My |
Best practice remains creating the tools.go file. |
Probably it's worth to highlight why |
A related comment: as far as I am aware, creating a In addition, there is also this separate comment in #26244 (comment) :
...which on the one hand seems to say ...but that comment also seems to suggest that if there is a piece of Go code that will be vendored, the operation seems to function at the granularity of directories (e.g., it "copies only the directories needed to satisfy imports in builds of the top-level module"). In practice, this currently seems to mean that non-Go tool dependencies (such as a shell script) are copied into the vendor directory by However, as far as I can see, the EDIT: note that a fair amount of care is required if you are pulling in additional directories beyond what |
I've tried the solution proposed by @myitcv, it more or less works, seems like
Not sure if it is a bug in stringer or somewhere else, happy to provide more info if needed though. Not sure if useful, but:
|
re: @rsc in #25922 (comment)
The best practice described currently triggers a From llir/llvm/ir/enum/tools.go: //go:build tools
package enum
import (
_ "golang.org/x/tools/cmd/stringer"
) Running
Is using a Cheers, |
@mewmew In this case, don't run |
I've moved to installing
I've not looked into this, but I feel |
This will actually install to the users home directory somewhere,
polluting the machine outside the repo and is generally a bad
practice regardless of the build system being used (and especially
with Make where "install" is a phony target and the outputs can't be
kept track of).
The benefit of using a tools.go and then running the deps with go run is
that you always wind up using the correct version and don't have to do
the install every single time (once the temporary files are downloaded
and properly versioned/hashed Go is happy to skip fetching them). This
should work fine with a properly written Makefile because tools.go will
be a dependency of whatever your final build target is, eg. if you do
something like this:
GOFILES!=find . -name '*.go'
tool_generated_file: tools.go
go run
sometool mybinary: tool_generated_file $(GOFILES)
go build -o $@
Then when the tools.go file changes the tool will be re-run and your
files regenerated.
—Sam
…On Thu, Oct 14, 2021, at 08:40, andig wrote:
I've moved to installing `tools.go` dependencies using `make` with the
power of `go list`:
install: go install $$(go list -f '{{join .Imports " "}}'
tools.go)
I've not looked into this, but I feel `//go:build tools` should become
obsolete doing so.
--
Sam Whited
|
Another data point: I'm urgently porting an older large project from git submodules to go modules and tools.go approach breaks
I can get rid of the error by using |
I think I'll just summarize my findings down here; it didn't all seem obvious and took some googling and tinkering, as some sources are a bit ambiguous on some of the points. All native:
|
I think golang needs to:
Edit: It appears |
@antichris, I still dont understand how I get the binary from that tools file and can use it in go generate? I have this in the main package. //go:build tools
package main
import (
_ "github.com/golang-migrate/migrate/v4"
_ "github.com/golang-migrate/migrate/v4/database/postgres"
_ "github.com/golang-migrate/migrate/v4/source/github"
) I ran go mod tidy but how can I use the binary now? package main
//go:generate migrate -source=db
func main() {
}
|
Unlike many projects, the Go project does not use GitHub Issues for general discussion or asking questions. GitHub Issues are used for tracking bugs and proposals only. For questions please refer to https://github.com/golang/go/wiki/Questions |
@bluebrown In general, the common practice would be running the tool using full package path in a //go:generate go run golang.org/x/tools/cmd/stringer -type Foo This would work if you have import _ "golang.org/x/tools/cmd/stringer" In your specific case, you'd have to import import _ "github.com/golang-migrate/migrate/v4/cmd/migrate" And, according to their documentation, add the appropriate build tags when running the //go:generate go run -tags postgres github.com/golang-migrate/migrate/v4/cmd/migrate -source=db COMMAND |
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes; and latest
vgo
commit (per above)What operating system and processor architecture are you using (
go env
)?What did you do?
Off the back of #25624 (comment), I'd like to confirm that the following represents the "best practice" advice when adding and installing tool dependencies:
The
go.mod
and.Target
forstringer
look fine:The issue however is that running
vgo mod -sync
then removes our module requirement ongolang.org/x/tools
- I suspect this is a bug:If we assume this is a bug and ignore it for now, I also wonder whether we can improve this workflow for adding tool dependencies somehow. The following steps feel a bit "boilerplate" and unnecessary:
tools.go
filemain
package, so I'm not sure we can ever safely verifytools.go
is "good"?)GOBIN
to an appropriate locationvgo install tool
PATH
includesGOBIN
I wonder whether we could in fact obviate all of this by having something like:
Thoughts?
vgo run tool
is possible as a result of #22726, but because of #25416 it effectively requires a link step each time.What did you expect to see?
With respect to what I think is a bug with
vgo mod -sync
go.mod
unchanged by thevgo mod -sync
What did you see instead?
The
golang.org/x/tools
requirement removed./cc @rsc @bcmills
The text was updated successfully, but these errors were encountered: