x/mod/modfile: AddNewRequire doesn't put direct dependencies in the first block #69050

stevenh · 2024-08-24T00:58:52Z

Go version

go version go1.23.0 linux/amd64

Output of `go env` in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/steveh/.cache/go-build'
GOENV='/home/steveh/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/steveh/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/steveh/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/steveh/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/steveh/code/github.com/rocketsciencegg/congestion-control/tools/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build155249492=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Use AddNewRequire to add new requires.

package main

import (
	"testing"

	"github.com/stretchr/testify/require"
	"golang.org/x/mod/modfile"
	"golang.org/x/mod/module"
)

var testFile = `module test

go 1.23.0

require (
	github.com/test/test1 v0.1.0
	github.com/test/test2 v0.1.0
)

require (
	github.com/test/test3 v0.1.0 // indirect
)
`

var expected = `module test

go 1.23.0

require (
	github.com/foo/direct1 v0.1.1
	github.com/foo/direct2 v0.1.2
	github.com/test/test1 v0.1.0
	github.com/test/test2 v0.1.0
)

require (
	github.com/foo/indirect1 v0.2.1 // indirect
	github.com/foo/indirect2 v0.2.2 // indirect
	github.com/test/test3 v0.1.0 // indirect
)
`

func TestAddRequire(t *testing.T) {
	file, err := modfile.Parse("go.mod", []byte(testFile), nil)
	require.NoError(t, err)

	file.AddNewRequire("github.com/foo/indirect2", "v0.2.2", true)
	file.AddNewRequire("github.com/foo/direct2", "v0.1.2", false)
	file.AddNewRequire("github.com/foo/direct1", "v0.1.1", false)
	file.AddNewRequire("github.com/foo/indirect1", "v0.2.1", true)

	file.Cleanup()
	file.SortBlocks()

	data, err := file.Format()
	require.NoError(t, err)
	require.Equal(t, expected, string(data))
}

What did you see happen?

Direct requires should be added to first require block, indirect requires should be added to second block.

What did you expect to see?

When using AddNewRequire, requires are added to the last block. This is the documented behaviour, but other go tools such as go mod tidy maintain two blocks, so this use the same approach.

It seems possible to use SetRequireSeparateIndirect to replicate the desired behaviour but that was far from obvious.

If nothing else reference in AddNewRequire and Addequire to SetRequireSeparateIndirect would help users fine the right functionality, but ideally AddNewRequire and AddRequire should function as expected.

If a user isn't aware of the two block rule then using this will result in a file that go mod tidy handles badly, in some cases resulting in three blocks instead of two.

The text was updated successfully, but these errors were encountered:

gabyhelp · 2024-08-24T01:00:21Z

Related Issues and Documentation

cmd/go: `mod tidy -go=1.17` should move indirect dependencies to the second require part #47733 (closed)
cmd/go: 'mod tidy' sometimes adds new, unnecessary require sections #67948
cmd/go: separate direct and indirect dependencies in go.mod #27887 (closed)
cmd/go: 'go mod tidy' may delete comments on require blocks #47563 (closed)
cmd/go: mod tidy should join "require" sections if there are more than two #56471
import/path: issue title more than require blocks in go.mod #68303 (closed)
cmd/go: mod edit doesn't respect direct/indirect blocks in go.mod #51983 (closed)
cmd/go: go directive is not only added during go mod init, but also under other conditions that are hard to deduce #30790 (closed)
cmd/go: 'mod tidy' chooses points in requirement cycles that differ from other subcommands #34086 (closed)
x/tools/gopls: missing direct requirements also listed in go.mod diagnostics #40470 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

seankhliao · 2024-08-24T01:59:35Z

See #68593, just parsing the modfile doesn't provide enough information for if a module should be direct or indirect, and the default addition is to mark it as indirect.

stevenh · 2024-08-24T02:07:31Z

See #68593, just parsing the modfile doesn't provide enough information for if a module should be direct or indirect, and the default addition is to mark it as indirect.

I'm not sure you correctly understood what I reported, AddNewRequire takes the indirect parameter to specify if the caller wants a direct or indirect, which it does correctly.

What it's not doing is adding it to the correct block.

seankhliao · 2024-08-24T02:10:40Z

Ah sorry.

stevenh · 2024-08-24T02:15:24Z

FYI I have some code which I believe fixes the behaviour, just need to get a PR created from it.

gopherbot · 2024-08-24T12:30:52Z

Change https://go.dev/cl/608056 mentions this issue: modfile: fix AddNewRequire block ordering

matloob · 2024-08-26T21:50:17Z

AddNewRequire is documented to add the require to the last block and I think we should be careful when making changes to behavior like this.

The go command only creates the two blocks for go modules declaring go 1.17 or later. We should make sure that we don't change that behavior.

Take a look at the code here: src/cmd/go/internal/modload/init.go#1846 The go command decides to call SetRequire or SetRequireSeparateIndirect based on the module's goVersion.

stevenh · 2024-08-27T21:28:11Z

Thanks for the link, to clarify are users of modfile expected to use SetRequireSeparateIndirect to add requires instead of AddRequire / AddNewRequire @matloob?

matloob · 2024-08-28T16:41:09Z

SetRequire and SetRequireSeparateIndirect are meant to be used by the go command.

I think users that need to manually edit a go.mod file should use AddRequire but then run go mod tidy to fix the go.mod file. The direct and indirect values aren't always updated until a tidy is done.

We should fix the problems with tidy: Tidy shouldn't split a two-group go.mod file into three.

stevenh · 2024-08-28T17:25:09Z

To confirm my understanding, AddRequire and AddNewRequire should ideally maintain the status?

If so this is what exactly golang/mod#21 targets to do. However it does always look to create two groups so if we want it to only create one per 1.17 then it would need tweaking.

matloob · 2024-08-28T17:39:08Z

I don't think AddRequire should maintain the // indirect blocks. go mod tidy should do that.

It should be okay to had AddRequire add an // indirect requirement to the third block and then have go mod tidy figure out where it belongs.

stevenh · 2024-08-28T18:14:45Z

The problem with that is it means you add a dependency on the go binary, the reason for using modfile is to avoid that, otherwise you could just exec go get, go mod edit --replace.

If modfile.Cleanup() corrected the order that would be fine though.

For reference the use case I'm working with is automating the correction of modules which create plugins, which have the requirement that they use identical module versions, specifically for krakend, which is typically used in a docker container that doesn't have go.

matloob · 2024-09-12T15:28:12Z

We expect the go command to be used to tidy a go.mod file before it's released. That's the main supported use case. We provide the x/mod library to allow programatic modification of the go.mod file, but it isn't meant to produce tidy go.mod files.

If the edited modules are being released I'd strongly recommend running go mod tidy before releasing them.

I think for what you're trying to your best bet is probably to use SetRequireSeparateIndirect to set the requires in the file and sort the requires into direct and indirect blocks. But you should be aware that there may be more work required to produce a tidy go.mod file`.

stevenh · 2024-09-13T13:36:37Z

Totally agree that go mod tidy is still needed, in our use case this is done as a separate step outside of the container that is making the initial changes. The problem is go mod tidy doesn't correct the output from AddNewRequire.

We have already made the switch to SetRequireSeparateIndirect for now but I think this should still be fixed.

From a users perspective the AddNewRequire is the most obvious way add a require, and it should behave correctly, specifically it should leave the file in a state that go mod tidy can correct, which is not currently the case. Having two ways to do things is fine, but they should both have a valid output.

If SetRequireSeparateIndirect is the new way, deprecating AddNewRequire would at least make it obvious to the user, but as AddNewRequire is an better match for the typical use case of "adding a specific require" I would suggest the ideal solution would be to make AddNewRequire and SetRequire result in the expected layout, and deprecate SetRequireSeparateIndirect as it would become unnecessary.

In short all methods should result in a file that would require minimal changes that go mod tidy will handle.

Thoughts?

matloob · 2024-09-19T19:00:56Z

I agree that the methods should result in a file that go mod tidy will handle. We should fix go mod tidy to move direct dependencies that appear in the indirect block back into the same block as the other dependencies.

But I think it's okay that AddNewRequire places direct requirements in the second "indirect" block.

SetRequireSeparateIndirect essentially exists to be used by go mod tidy, so it's not something we want to push people to. And we don't want to deprecate AddNewRequire.

We do plan to fix the go mod tidy issue where it doesn't keep the two blocks.

gopherbot added this to the Unreleased milestone Aug 24, 2024

seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2024

seankhliao reopened this Aug 24, 2024

seankhliao added the modules label Aug 24, 2024

stevenh linked a pull request Aug 24, 2024 that will close this issue

modfile: fix AddNewRequire block ordering golang/mod#21

Open

seankhliao changed the title ~~x/mod/modfile: AddNewRequire always adds to the last block~~ x/mod/modfile: AddNewRequire doesn't put direct dependencies in the first block Aug 24, 2024

prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/mod/modfile: AddNewRequire doesn't put direct dependencies in the first block #69050

x/mod/modfile: AddNewRequire doesn't put direct dependencies in the first block #69050

stevenh commented Aug 24, 2024 •

edited

Loading

gabyhelp commented Aug 24, 2024

seankhliao commented Aug 24, 2024

stevenh commented Aug 24, 2024

seankhliao commented Aug 24, 2024

stevenh commented Aug 24, 2024

gopherbot commented Aug 24, 2024

matloob commented Aug 26, 2024

stevenh commented Aug 27, 2024

matloob commented Aug 28, 2024

stevenh commented Aug 28, 2024

matloob commented Aug 28, 2024 •

edited

Loading

stevenh commented Aug 28, 2024

matloob commented Sep 12, 2024

stevenh commented Sep 13, 2024

matloob commented Sep 19, 2024 •

edited

Loading

x/mod/modfile: AddNewRequire doesn't put direct dependencies in the first block #69050

x/mod/modfile: AddNewRequire doesn't put direct dependencies in the first block #69050

Comments

stevenh commented Aug 24, 2024 • edited Loading

Go version

Output of go env in your module/workspace:

What did you do?

What did you see happen?

What did you expect to see?

gabyhelp commented Aug 24, 2024

seankhliao commented Aug 24, 2024

stevenh commented Aug 24, 2024

seankhliao commented Aug 24, 2024

stevenh commented Aug 24, 2024

gopherbot commented Aug 24, 2024

matloob commented Aug 26, 2024

stevenh commented Aug 27, 2024

matloob commented Aug 28, 2024

stevenh commented Aug 28, 2024

matloob commented Aug 28, 2024 • edited Loading

stevenh commented Aug 28, 2024

matloob commented Sep 12, 2024

stevenh commented Sep 13, 2024

matloob commented Sep 19, 2024 • edited Loading

stevenh commented Aug 24, 2024 •

edited

Loading

Output of `go env` in your module/workspace:

matloob commented Aug 28, 2024 •

edited

Loading

matloob commented Sep 19, 2024 •

edited

Loading