-
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: track tool dependencies in go.mod #48429
Comments
I like this, I find it annoying to use the If this proposal moves forward, where does the dependency go in the |
Good question! I'm not so familiar with the reasoning behind the multiple blocks... something to do with lazy loading? I'd defer to those with more experience in this area |
Personally, I think #42088 is already a pretty good solution. With it, one can write
Similarly, Another big advantage is that you can pick specific versions of tools, and they won't interfere with your main |
The only downside to #42088 is that, if you repeat the same |
Or |
Oh interesting, thanks @mvdan I wasn't aware of that solution. 🤔 A few concerns immediately come to mind...
|
Also this |
In module mode, |
It certainly warrants a mention. I'm not sure we should bless it as the only best practice, though, because there can be legitimate reasons for versioning, downloading, and running tools some other way. Perhaps some of your tools aren't written in Go, such as protoc, so you use a "tool bundler" that's entirely separate to Go. Or perhaps you do need your tools to share the same MVS graph with your main module for proper compatibility, so you want them to share a |
Gotta say though...
|
So even with the |
Also worth noting: codegen tools like gqlgen and protobuf are often comprised of a generator command and a runtime, both of which typically need to be versioned in lock-step. This proposal solves that case rather neatly, allowing go.mod to manage both generator and runtime versions. |
We used to do that. Then people would have that replicated across different files and the version wouldn't always match, and we wanted to automate tool updating, so we figured that migrating to Again, |
@jayconrod has previously suggested something similar, using a new directive (perhaps Personally, I prefer the approach of adding a new directive — today we do treat requirements with A new |
@bcmills would such tool requirements be part of the same MVS module graph? |
The In particular:
|
Or |
I like this proposal. I've had something similar in my drafts folder for a while. @bcmills touched on the main difference.
I don't think |
Yeah I like the A |
Or have I got that wrong? Is sharing indirect dependencies between tools and other dependencies a desirable feature? |
Right. The go command reports errors for unknown directives in the main module's go.mod file, but it ignores unknown directives in dependencies' go.mod files. So everyone working on a module that used this would need to upgrade to a version of Go that supports it (same as most other new features), but their users would be unaffected.
My suggestion is to have If you don't want to mix tool and library dependencies in |
Yep that makes a lot of sense @jayconrod |
This proposal has been added to the active column of the proposals project |
@jayconrod Did you want to write up the |
@ConradIrwin, unfortunately I won't be able to review these changes after all; I'm leaving Google (and the Go team) on this Friday, March 15. |
Hi all. I've been quietly watching this work for a few months now and I'm quite excited for this new functionality! That said, the last comments on this issue are a little discouraging. If possible, I would appreciate an update. Has anyone from the Go team taken over the code review? Is this progressing? Thanks everyone for working toward this contribution!! |
Unfortunately, at this point I'm not sure if we're going to have the time to review this before the freeze. That said, I'll try to start reviewing the CLs at the bottom of the stack and we can submit them together if we are able to get the stack done in time. |
@matloob I would love to have it reviewed before the next freeze if that's possible :D. Should I connect with you when the window opens again? |
Yes that sounds good. I'll try to see if I can start during the freeze so we can maybe make progress before the window opens. |
Great, thank you!
…On Thu, Apr 18 2024 at 11:59, Michael Matloob ***@***.***> wrote:
Yes that sounds good. I'll try to see if I can start during the freeze so
we can maybe make progress before the window opens.
—
Reply to this email directly, view it on GitHub
<#48429 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAXAQGM3MK3HNJZT2V6BCTY6ACY7AVCNFSM5EGC4OSKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBWGQ3TMNBYHAYQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Just pinging to check on the progress of this one; since there is an implementation that mainly requires review it seems like a relatively low-hanging fruit to get this done before the window opens ;-) |
I've started the reviews of the CLs. |
With this proposal, does the |
Add new tool directive to go.mod parser and functions to add and drop them. For golang/go#48429 Change-Id: I37667a69ded9d59ea248ec48ad35c87592103218 Reviewed-on: https://go-review.googlesource.com/c/mod/+/508355 Reviewed-by: Michael Matloob <[email protected]> Reviewed-by: Sam Thanawalla <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
@pierrre Yes. Tools will be treated similar to as if they were a package of the main module, so a tidy go.mod will contain all the dependencies needed to build it. |
For golang/go#48429 Change-Id: Ie3056f11b72b868d131f0a1ec09120b64b4dec24 Reviewed-on: https://go-review.googlesource.com/c/proposal/+/495555 Reviewed-by: Michael Matloob <[email protected]>
For #48429 Change-Id: I1a7bd8ffddbc65e3b687dc1d40f3853702e1b5dc Reviewed-on: https://go-review.googlesource.com/c/go/+/521958 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Sam Thanawalla <[email protected]> Reviewed-by: Michael Matloob <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Nice to see this being worked on. |
Change https://go.dev/cl/614555 mentions this issue: |
Allowing relative paths in `go.mod` introduced an inconsistency as we do not allow relative package paths anywhere else. For #48429 Change-Id: I5ef88aec4fe35f7e94a0cf6288e94099f3ca7a0e Reviewed-on: https://go-review.googlesource.com/c/go/+/614555 Reviewed-by: Sam Thanawalla <[email protected]> Reviewed-by: Michael Matloob <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
Packages referenced by tool lines in go.mod files will now be included in the module graph for the new "tool" package pattern and the "all" package pattern. For #48429 Change-Id: I128f6a50880814bd5395674426c9a7ee2ddc19bf Reviewed-on: https://go-review.googlesource.com/c/go/+/521959 Reviewed-by: Carlos Amedee <[email protected]> Reviewed-by: Michael Matloob <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
Running `go get -tool example.com/m1` will add a tool line to your mod file and add any missing dependencies. Running `go get -tool example.com/m1@none` will drop the tool line from your mod file. For #48429 Change-Id: I07b4776f1f55eff588d08cb6649d94cc42a729d2 Reviewed-on: https://go-review.googlesource.com/c/go/+/563175 Reviewed-by: Michael Matloob <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Sam Thanawalla <[email protected]>
Reminder that buildtime dependencies installed via out of band methods are likely missing from SBOM's sent to SCA tools. govulncheck, Snyk, Dependabot, etc. look primarily in A few container SCA tools like Docker Scout, Red Hat Quay Container Security Operator, Snyk Container may happen to match CVE's for vulnerable buildtime Go dependency tools. But only for Go tools installed via OS package managers, and likely only for Go tools that are not removed from the final image using modern, multi stage images. |
UPDATE: 2024-07-29: the latest proposal can be found here.
Background
The current best-practice to track tool dependencies for a module is to add a
tools.go
file to your module that includes import statements for the tools of interest. This has been extensively discussed in #25922 and is the recommended approach in the Modules FAQThis approach works, but managing the tool dependencies still feels like a missing piece in the
go mod
toolchain. For example, the instructions for getting a user set up with a new project using gqlgen (a codegen tool) looks like thisThe
printf
line above really stands out as an arbitrary command to "add a tool" and reflects a poor developer experience when managing tools. For example, an immediate problem is that theprintf
line will only work on unix systems and not windows. And what happens iftools.go
already exists?So while we have some excellent tools for managing dependencies within the
go.mod
file usinggo get
andgo mod edit
, there is no such equivalent for managing tools in thetools.go
file.Proposed Solution
The
go.mod
file uses the// indirect
comment to track some dependencies. An// indirect
comment indicates that no package from the required module is directly imported by any package in the main module (source).I propose that this same mechanism be used to add tool dependencies, using a
// tool
comment.Users could add a tool with something like
or
A
go.mod
would then look something likeAnd would allow users to subsequently run the tool with
go run github.com/99designs/gqlgen
This would mean a separate
tools.go
file would no longer be required as the tool dependency is tracked in thego.mod
file.Go modules would be "tool" aware. For example:
go mod tidy
would not remove the// tool
dependency, even though it is not referenced directly in the module// tool
dependency is imported by another module, Go modules understands that the// tool
dependency is not required as an indirect dependency. Currently when usingtools.go
, go modules does not have that context and the tool is treated like any other indirect dependencygo get -tool [packages]
would only add a dependency with amain
packageThe text was updated successfully, but these errors were encountered: