-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
proposal: cmd/go: unused (yet) dependencies added by go get should not be indirect #68593
Comments
go get
on go.mod
should match user intent
I've seen projects on Github where the dependencies are incorrectly marked as indirect, presumably for this reason. |
I'm not sure what all the implications of doing this are. You're right that since the I would like to see if we can first try to find an alternate solution to the bzlmod the right set of direct dependencies. For example, if it's too expensive to run I'm curious to know how long |
A tool that is based on this assumption would seem very brittle: Since I nonetheless agree that we should take the potential implications on backwards compatibility seriously. If we otherwise arrive at the conclusion that this behavior would be beneficial to users in general (not just users of rules_go), could gating this behind
Could you elaborate on the
In a large monorepo with >2,000 Go module deps, the logic that translates |
I think we would use those if we wanted to experiment with the behavior change without committing to it. But I think what we'd end up doing would be to keep the old behavior for modules declaring, say, go version 1.23 or older, and only change the behavior for 1.24 or later. I think we'd ultimately get the same level of safety, but any people or tools depending on the old behavior would have to adjust their expectations for (say) 1.24 or later modules.
The Take the following directory as an example (it's in txtar format: use golang.org/x/exp/cmd/txtar --extract to write the files to disk so you can try it out)
If you run So
I see. I can understand the need for something faster. I'd like to see first if we can find a solution without changing the behavior of |
Thanks for the explanation and the example! This could be a fast replacement for
We could possibly use this for a workaround that is similar to what Downsides would be that our "transparent wrapper around |
They are indirect because there are no references to them yet. This is an accurate recording of the facts. If you want them to not be indirect, add the imports first and then use 'go get' with no arguments to scan the current directory's source code and resolve its imports. Then it will see that the imports are used and mark them direct. Or else run 'go mod tidy' after writing your code. Both of those will also end up with an accurate recording of the facts. This is not something we should change. |
I think a better description of the state of the world than the import being "indirect" is that it was "manual". It's not a normal indirect import where A imports B. It's C was added manually but isn't needed (yet). |
If we follow this line of argument to the end, wouldn't that mean that
Adding just the imports is tricky since IDEs (both VSCode with gopls and GoLand) tend to remove imports that aren't used in a file on save (or explicit format action). So this would require adding both an
Compared to that, it would be more convenient to just type out the Which leads to my most important question: Why should a user interactively run the
I like this point of view. If adding the new dep as a direct require seems inappropriate, perhaps it could be marked as |
I still don't think we should make changes here. Manually added dependencies can be considered indirect, meaning they are not directly used by the source code. If you want to specify a specific version, you can still add the import where you plan to use it and then run |
This proposal has been added to the active column of the proposals project |
Based on the discussion above, this proposal seems like a likely decline. |
No change in consensus, so declined. |
**What type of PR is this?** Feature **What does this PR do? Why is it needed?** When running `go get example.com/[email protected]`, the module `example.com/foo` will be added to `go.mod` with an `// indirect` comment, just like its transitive dependencies. This results in `go_deps` not being able to distinguish this explicitly requested new dep from other indirect deps that shouldn't be imported by the root module. Running `go mod tidy` or `go get` without arguments is required to have the comment removed after adding a reference to the new module in code. This change makes it so that `bazel run @rules_go//go get example.com/[email protected]` marks the requested module as a direct dependency. This realizes golang/go#68593 in our custom wrapper, thus making this command the only one needed to add a new module dependency and have it work with a subsequent Bazel build (assuming that also updates the BUILD files with Gazelle). This [matches the behavior of `gopls` when adding a new dependency](https://github.com/golang/tools/blob/ec1a81bfec7cc6563474b7aa160db30df9bfce6b/gopls/internal/server/command.go#L805-L809). --------- Co-authored-by: Son Luong Ngoc <[email protected]>
) **What type of PR is this?** Feature **What does this PR do? Why is it needed?** When running `go get example.com/[email protected]`, the module `example.com/foo` will be added to `go.mod` with an `// indirect` comment, just like its transitive dependencies. This results in `go_deps` not being able to distinguish this explicitly requested new dep from other indirect deps that shouldn't be imported by the root module. Running `go mod tidy` or `go get` without arguments is required to have the comment removed after adding a reference to the new module in code. This change makes it so that `bazel run @rules_go//go get example.com/[email protected]` marks the requested module as a direct dependency. This realizes golang/go#68593 in our custom wrapper, thus making this command the only one needed to add a new module dependency and have it work with a subsequent Bazel build (assuming that also updates the BUILD files with Gazelle). This [matches the behavior of `gopls` when adding a new dependency](https://github.com/golang/tools/blob/ec1a81bfec7cc6563474b7aa160db30df9bfce6b/gopls/internal/server/command.go#L805-L809). --------- Co-authored-by: Son Luong Ngoc <[email protected]>
Proposal Details
(In the following,
$package
stands for some package in a Go module$module
at version$version
.)When a developer runs
go get $package@$version
to add a new dependency to their module, the result is that the module'sgo.mod
file is updated withrequire
directives for$module@$version
as well as the transitive dependencies of the given package, with all of theserequire
s being marked with// indirect
.While this seems appropriate for the transitive dependencies, the outcome for the
require $module $version
is not what the user intended:$package
in their code, they will have introduced a direct dependency on$module@$version
without this being reflected ingo.mod
.$package
(or any other package in$module
), therequire
directives for$module@$version
and all its new transitive dependencies are unnecessary.In either case, a command such as
go mod tidy
orgo get -t ./...
is necessary to get thego.mod
file into a consistent state. In fact, thego.mod
file after runninggo get $package@$version
is, due to the// indirect
comment on the newrequire
directive for$module@$version
, usually not in a state that could ever be produced bygo mod tidy
.This is problematic for external tooling that relies on the information in
go.mod
to determine the set of dependencies for a module in a lightweight way (compared to the more heavyweight and over-approximatinggo list -m all
).gopls
explicitly works around this behavior by manually runninggo mod edit -require $module@$version
to ensure that therequire
directive for$module@$version
is not marked as// indirect
.Proposal
When a user runs
go get $package@$version
(or any other package pattern), therequire
directive for$module@$version
should not be marked as// indirect
. Since adding a usage of the package to the code and runninggo mod tidy
would have the same effect, this change should not break any existing workflows or tooling.A more conservative approach would be to gate this behavior behind a flag (say
-r
for "require").Context
While I believe that this issue is generic to all users of
go get
, especially those that aren't also usinggopls
, there is a specific use case that motivated me to write it up.The
rules_go
Bazel ruleset uses Bazel's new dependency management system Bzlmod to directly translate the information in a project'sgo.mod
andgo.sum
files into Bazel dependencies, with each Go module corresponding to a Bazel repository.Since external dependencies are subject to "strict visibility", each Bazel module needs to declare all its direct dependencies. For this reason, the
// indirect
comment ingo.mod
is used as a signal torules_go
that a dependency is not direct, avoiding build breakages caused by changes to transitive dependencies of updated direct dependencies. Due to the particular behavior ofgo get
, this makes it very difficult for users to add new dependencies to their Bazel-managed Go project without also adding a usage in code and runninggo mod tidy
, which can be slow in large monorepos.If
go get
didn't mark the explicitly requested modules as// indirect
, this setup would "just work".The text was updated successfully, but these errors were encountered: