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

cmd/go: lazy modules: separate section for indirect imports #45965

Closed
rogpeppe opened this issue May 5, 2021 · 22 comments
Closed

cmd/go: lazy modules: separate section for indirect imports #45965

rogpeppe opened this issue May 5, 2021 · 22 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented May 5, 2021

go version devel go1.17-9e0facd26e Wed May 5 04:48:30 2021 +0000 linux/amd64

With the new lazy module loading introduced recently, a go.mod file will contain all indirect dependencies, not just the ones which lack go.mod files. This can make it hard to see the direct dependencies because they're obscured by all the indirect dependencies. As an example, in the first repository I tried, the number of indirect dependencies listed rose from 52 to 157, against 123 direct dependencies.

Although indirect dependencies are important, we should mostly be considering the direct dependencies when adjusting version numbering. It also makes it more likely that people resolving conflicts might get things wrong by choosing the wrong indirect versions.

As a way of avoiding this issue, it might be a good idea to format indirect dependencies in their own require section.

For example:

module m

go 1.17

require (
    github.com/foo/bar v1.2.3
)

require (
    github.com/baz v1.0.0    // indirect
)

Perhaps even consider supporting a whole-directive indirect comment rather than line-by-line:

// indirect
require (
    github.com/baz v1.0.0
)
@bcmills bcmills added modules NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. release-blocker labels May 5, 2021
@bcmills bcmills added this to the Go1.17 milestone May 5, 2021
@bcmills
Copy link
Contributor

bcmills commented May 5, 2021

Interesting suggestion. I think I would rather keep per-line annotations, both because they're easier to spot in a very long file (which block is this screenful in again‽) and because they make erroneous edits more obvious.

@mvdan
Copy link
Member

mvdan commented May 5, 2021

This would also help with code review. When one is developing a module it's easy to use go list -m to get the information as needed, but when reviewing changes, one is often looking at a plain go.mod file.

@bcmills
Copy link
Contributor

bcmills commented May 5, 2021

That's true, although in the code-review setting it's not obvious to me that adding an indirect dependency is substantially different from adding a direct one — it's essential to audit the dependency for security either way.

And if an indirect dependency is promoted to a direct one, the diff is probably easier to review if it appears as the removal of an // indirect comment rather than the removal of an indirect line and the addition of a direct one with the same version.

@rsc
Copy link
Contributor

rsc commented May 12, 2021

I feel like vgo or xgo did this, or at least I considered it, but I didn't keep it (or do it) for exactly the reason @bcmills gives in the comment just above this one: something changing from // indirect to not-indirect doesn't feel like it should be zapped to an entirely different part of the file.

@mvdan
Copy link
Member

mvdan commented May 12, 2021

I guess it's a tradeoff between the plain file being easier to read and understand by humans, and diffs in some cases being more concise. I personally run into the former more often, as it's not particularly often that indirect dependencies get promoted to direct dependencies. Updating dependencies happens significantly more often than adding or removing direct dependencies, at least in my experience.

@rogpeppe
Copy link
Contributor Author

For me, the view of direct dependencies that you currently see in a go.mod file is very useful, as there you easily see the version knobs that you might wish to be tweaking directly, and the dependencies that you might be able to do something about easily.

When you've imported three modules and those are swamped in a sea of indirect dependencies, the go.mod file becomes less useful to view directly. I think that would be a shame.

FWIW I think that the transition between indirect and direct dependency is an important one and worth a bigger diff.

@peebs
Copy link

peebs commented May 18, 2021

I like this idea. I personally don't think looking at the diff would be confusing. The concerns about losing your place in large go.mod files could be avoided by keeping a // indirect comment per line.

@bcmills
Copy link
Contributor

bcmills commented May 19, 2021

There are conceptually three different categories of dependencies that a user might want to think about:

  1. Dependencies that are directly imported by something in the main module.
  2. Indirect-only dependencies that are upgraded above the maximum version otherwise required by transitive dependencies, including “upgraded above none”.
  3. Indirect-only dependencies that are redundant with the maximum version required by transitive dependencies.

go mod tidy in a go 1.16 module explicitly lists the dependencies in (1) and (2) in one block, and omits the dependencies in (3).

go mod tidy in a go 1.17 module adds in the dependencies in (3).

So, one question is: should we continue to mix (1) and (2) together (as “express” and “implied”), or keep (1) separate and mix together (2) and (3) (as “direct” and “indirect”), or split them into three separate sections (“direct”, “upgraded indirect”, “implied indirect”)?

@jayconrod pointed out that the “direct” category may also include upgrade to modules that would otherwise be constrained by implied transitive dependencies, so at the moment we're leaning toward the “direct” / “indirect” split.

@rogpeppe
Copy link
Contributor Author

Given that we seem to be leaning towards including an "indirect" qualifier on every line, how about including category 2 dependencies in the first section but still including the indirect qualifier comment?

That way the first section includes all the dependencies that the module directly interacts with in any way, and the manually upgraded deps are still clear.

@bcmills
Copy link
Contributor

bcmills commented May 24, 2021

Given that we seem to be leaning towards including an "indirect" qualifier on every line, how about including category 2 dependencies in the first section but still including the indirect qualifier comment?

Hmm. Going back to #45965 (comment), if we put upgraded-indirect dependencies in the first section, then every time they go between “upgraded” and “at implied version” they'll pop back and forth between the two sections, showing up in the diff as an addition and removal rather than a change.

I think that would be pretty awkward to read in a code review, so I think it's a compelling reason to split by direct / indirect rather than upgraded / at-implied-version.

@bcmills bcmills added okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker labels May 24, 2021
@peebs
Copy link

peebs commented May 25, 2021

My intuition was to group 1 and 2 together as well. Mostly because I see group 3 as the largest and potentially nosiest while group 2 might be relevant to me and probably not overwhelmingly large.

That said, I can see how moving indirect deps between the two require blocks to be confusing. Its much simpler and self-evident to understand: this block is direct, this is indirect. Ultimately I think either grouping (1 and 2,3 or 1,2 and 3) is big improvement over the current single grouping and I'd be glad to see this move forward either way.

@rogpeppe
Copy link
Contributor Author

rogpeppe commented May 25, 2021

How about having an extra semantic comment when an indirect dependency is at an upgraded version:

   m.com/pkg v1.2.4 // indirect upgrade

It would automatically be applied by the go tool to indirect go.mod entries as appropriate.

@mibk
Copy link
Contributor

mibk commented May 26, 2021

I agree with @peebs that grouping 1 and 2 together, as we do now, and putting 3 to a separate group does make sense.

Nonetheless if we end up going with this proposal, I would prefer separating the groups with just a blank line instead of putting it in another require statement. E.g.:

module m

go 1.17

require (
    github.com/foo/bar v1.2.3 // group 1
    github.com/baz v1.0.0 // group 2

    github.com/bam v0.3.14 // group 3
)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/323170 mentions this issue: modfile: remove end-of-line comments in removeLine

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/323171 mentions this issue: modfile: document (*File).*Require methods

gopherbot pushed a commit to golang/mod that referenced this issue May 27, 2021
I added this test case while updating documentation for
golang/go#45965, and it failed. This CL fixes the behavior,
and the next CL in the stack documents it.

For golang/go#45965

Change-Id: Ia68dbd33530eec138745c6e291b096a9fa1e1d58
Reviewed-on: https://go-review.googlesource.com/c/mod/+/323170
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
gopherbot pushed a commit to golang/mod that referenced this issue May 27, 2021
For golang/go#45965

Change-Id: If3c7255f44adc81b69e8109a5d9d62f116579bbd
Reviewed-on: https://go-review.googlesource.com/c/mod/+/323171
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/325230 mentions this issue: modfile: clean up SetRequire

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/325970 mentions this issue: modfile: factor out methods for changing Require properties

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/325971 mentions this issue: modfile: add SetRequireSeparateIndirect

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/325969 mentions this issue: modfile: make marking for removal a method on Line instead of FileSyntax

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/325922 mentions this issue: cmd/go: in Go 1.17+ modules, add indirect go.mod dependencies separately from direct ones

gopherbot pushed a commit to golang/mod that referenced this issue Jun 8, 2021
I started this change by expanding the documentation and tests for
SetRequire. Unfortunately, the tests failed when the existing
contents included duplicates of a module path:

    --- FAIL: TestSetRequire/existing_duplicate (0.00s)
        rule_test.go:1011: after Cleanup, len(Require) = 3; want 1
    --- FAIL: TestSetRequire/existing_duplicate_multi (0.00s)
        rule_test.go:1011: after Cleanup, len(Require) = 3; want 1

So then I fixed the detected bug, by updating the Line entries
(possibly marking them for removal) in the same loop that updates the
Require entries. (We don't need to loop over f.Syntax.Stmt separately
to remove deleted entries because f.Syntax.Cleanup already does that.)

For golang/go#45965

Change-Id: I1b665c0832112de2c4273628f266dc3d966fefdd
Reviewed-on: https://go-review.googlesource.com/c/mod/+/325230
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit to golang/mod that referenced this issue Jun 8, 2021
The act of marking a line for removal intentionally does not depend on
the rest of the syntax tree, in order to avoid quadratic behavior.
Make that property more explicit by defining it as a method on Line
rather than FileSyntax.

For golang/go#45965

Change-Id: I475625eddf57396411a3fb73eaedd624dd7af3d6
Reviewed-on: https://go-review.googlesource.com/c/mod/+/325969
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit to golang/mod that referenced this issue Jun 8, 2021
For golang/go#45965

Change-Id: I331d068d115b145239933da0d8341a1627124935
Reviewed-on: https://go-review.googlesource.com/c/mod/+/325970
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit to golang/mod that referenced this issue Jun 8, 2021
The new method is a variant of SetRequire, but adds new indirect
dependencies only in indirect-only blocks, and does not add new direct
dependencies to existing indirect-only blocks.

For golang/go#45965

Change-Id: I6730b586396658e710e4bf2afcf64fb2c827203f
Reviewed-on: https://go-review.googlesource.com/c/mod/+/325971
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2021

The CL implementing this is just waiting on TryBots, so I'm going to unmark it okay-after-beta1 so that it doesn't miss the boat. It's a big enough change that I'd like it to be in the beta given that it won't hold things up.

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. and removed okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 labels Jun 8, 2021
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 8, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/335050 mentions this issue: _content/ref/mod: mention other Go 1.17 behaviors that depend on the go version

gopherbot pushed a commit to golang/website that referenced this issue Jul 16, 2021
…go version

Updates golang/go#36876
Updates golang/go#42970
Updates golang/go#45965

Change-Id: I542e9ece986806f9b4a062f242387b1ca47f5814
Reviewed-on: https://go-review.googlesource.com/c/website/+/335050
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@rsc rsc unassigned bcmills Jun 23, 2022
passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
…go version

Updates golang/go#36876
Updates golang/go#42970
Updates golang/go#45965

Change-Id: I542e9ece986806f9b4a062f242387b1ca47f5814
Reviewed-on: https://go-review.googlesource.com/c/website/+/335050
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants