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: "mod tidy" erroneously labels imports from within normally-ignored directories as direct #37376

Open
agnivade opened this issue Feb 22, 2020 · 12 comments
Labels
Milestone

Comments

@agnivade
Copy link
Contributor

@agnivade agnivade commented Feb 22, 2020

What version of Go are you using (go version)?

$ go version
1.13.5 and 1.14beta1

Does this issue reproduce with the latest release?

yes

What did you do?

Please fetch this branch of mine: https://github.com/mattermost/mattermost-server/tree/tidygomod

  • Run "go mod vendor". You will see some changes in go.mod.
  • Run "go mod tidy". Those changes go away.
    ???

Things I tried:

  • Tried doing a "go clean -modcache -i -r", but didn't help.
  • Tried re-generating the vendor directory again, but didn't help.

What did you expect to see?

I expect no changes to go.mod in whatever order I run those commands.

What did you see instead?

go.mod changed.

/cc @bcmills @jayconrod

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Feb 24, 2020

I cannot reproduce the observed instability using go1.14rc1 or go1.13.8.

I see only one diff, but that diff is produced identically for both subcommands:

mattermost-server$ git status
On branch tidygomod
Your branch is up to date with 'origin/tidygomod'.

nothing to commit, working tree clean

mattermost-server$ go1.14rc1 mod tidy

mattermost-server$ git diff
diff --git i/go.mod w/go.mod
index 410361d74..2ba38df44 100644
--- i/go.mod
+++ w/go.mod
@@ -67,7 +67,7 @@ require (
        github.com/pkg/errors v0.9.1
        github.com/prometheus/client_golang v1.4.0
        github.com/rs/cors v1.7.0
-       github.com/russellhaering/goxmldsig v0.0.0-20180430223755-7acd5e4a6ef7
+       github.com/russellhaering/goxmldsig v0.0.0-20180430223755-7acd5e4a6ef7 // indirect
        github.com/rwcarlsen/goexif v0.0.0-20190401172101-9e8deecbddbd
        github.com/segmentio/analytics-go v3.1.0+incompatible
        github.com/segmentio/backo-go v0.0.0-20160424052352-204274ad699c // indirect

mattermost-server$ git reset --hard
HEAD is now at eb3f7be36 Merge branch 'master' into tidygomod

mattermost-server$ go1.14rc1 mod vendor
gi
mattermost-server$ git diff
diff --git i/go.mod w/go.mod
index 410361d74..2ba38df44 100644
--- i/go.mod
+++ w/go.mod
@@ -67,7 +67,7 @@ require (
        github.com/pkg/errors v0.9.1
        github.com/prometheus/client_golang v1.4.0
        github.com/rs/cors v1.7.0
-       github.com/russellhaering/goxmldsig v0.0.0-20180430223755-7acd5e4a6ef7
+       github.com/russellhaering/goxmldsig v0.0.0-20180430223755-7acd5e4a6ef7 // indirect
        github.com/rwcarlsen/goexif v0.0.0-20190401172101-9e8deecbddbd
        github.com/segmentio/analytics-go v3.1.0+incompatible
        github.com/segmentio/backo-go v0.0.0-20160424052352-204274ad699c // indirect

I wonder if this is a platform-specific bug — what is the output of your go env?

@agnivade

This comment has been minimized.

Copy link
Contributor Author

@agnivade agnivade commented Feb 26, 2020

Just updated to 1.14 and didn't see any difference. But I think I know where's the difference here. We use an enterprise layer which is a bundle of packages linked with a symlink. Both of them share this go.mod. You don't have that, I do.

The Go toolchain does say "ignoring symlink", but that's how I could see your "goxmldsig" change, if I delete the enterprise repo.

Now that I can't share the code with you, any pointers on how do I resolve this ?

go env

GO111MODULE=""
GOARCH="amd64"
GOBIN="/home/agniva/go/bin"
GOCACHE="/home/agniva/.cache/go-build"
GOENV="/home/agniva/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/agniva/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/agniva/mattermost/mattermost-server/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build526246433=/tmp/go-build -gno-record-gcc-switches"

This is exactly what I am seeing. (Ignore the untracked files)

22:47:35-~/mattermost/mattermost-server$git st
On branch tidygomod
Untracked files:
  (use "git add <file>..." to include in what will be committed)

	config/config.json.bak
	loadtestbulkload.json
	logrus
	testfiles/

nothing added to commit but untracked files present (use "git add" to track)
22:47:36-~/mattermost/mattermost-server$go version
go version go1.14 linux/amd64
22:47:38-~/mattermost/mattermost-server$go mod tidy
warning: ignoring symlink /home/agniva/mattermost/mattermost-server/client
warning: ignoring symlink /home/agniva/mattermost/mattermost-server/enterprise
22:47:45-~/mattermost/mattermost-server$git diff
diff --git a/go.mod b/go.mod
index 410361d74..c50747e82 100644
--- a/go.mod
+++ b/go.mod
@@ -41,7 +41,7 @@ require (
        github.com/icrowley/fake v0.0.0-20180203215853-4178557ae428
        github.com/jaytaylor/html2text v0.0.0-20190408195923-01ec452cbe43
        github.com/jmoiron/sqlx v1.2.0
-       github.com/jonboulle/clockwork v0.1.0 // indirect
+       github.com/jonboulle/clockwork v0.1.0
        github.com/konsorten/go-windows-terminal-sequences v1.0.2 // indirect
        github.com/lib/pq v1.3.0
        github.com/magiconair/properties v1.8.1 // indirect
@@ -66,6 +66,7 @@ require (
        github.com/pelletier/go-toml v1.6.0 // indirect
        github.com/pkg/errors v0.9.1
        github.com/prometheus/client_golang v1.4.0
+       github.com/prometheus/client_model v0.2.0
        github.com/rs/cors v1.7.0
        github.com/russellhaering/goxmldsig v0.0.0-20180430223755-7acd5e4a6ef7
        github.com/rwcarlsen/goexif v0.0.0-20190401172101-9e8deecbddbd
22:47:46-~/mattermost/mattermost-server$go mod vendor
warning: ignoring symlink /home/agniva/mattermost/mattermost-server/client
warning: ignoring symlink /home/agniva/mattermost/mattermost-server/enterprise
22:47:49-~/mattermost/mattermost-server$git diff
diff --git a/go.mod b/go.mod
index 410361d74..35f3652e7 100644
--- a/go.mod
+++ b/go.mod
@@ -66,6 +66,7 @@ require (
        github.com/pelletier/go-toml v1.6.0 // indirect
        github.com/pkg/errors v0.9.1
        github.com/prometheus/client_golang v1.4.0
+       github.com/prometheus/client_model v0.2.0 // indirect
        github.com/rs/cors v1.7.0
        github.com/russellhaering/goxmldsig v0.0.0-20180430223755-7acd5e4a6ef7
        github.com/rwcarlsen/goexif v0.0.0-20190401172101-9e8deecbddbd
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Feb 26, 2020

I would probably start with go mod why -m github.com/jonboulle/clockwork and go mod why -m github.com/prometheus/client_model.

The differences between go mod tidy and go mod vendor arise through dependencies of tests of dependencies of the main module, so a minimal repro probably involves one of those.

@agnivade agnivade self-assigned this Feb 27, 2020
@agnivade

This comment has been minimized.

Copy link
Contributor Author

@agnivade agnivade commented Feb 27, 2020

Update:

github.com/prometheus/client_model is actually a test dependency of the enterprise repo. It is used nowhere in the main repo, not even as a test dependency.

So ideally, it should be a direct dependency. And it seems like go mod tidy does the right thing, by somehow going inside the symlink, but go mod vendor misses the whole thing and makes it an indirect dependency (test dependencies are considered as direct dependencies right ?). I think there is a difference in how both of these commands handle symlinks. Making them consistent should fix the issue.

Same for github.com/jonboulle/clockwork too.

The issue goes away if I replace the symlink by the actual directory. Needless to say, I am well-aware of the fact that symlinks aren't recommended to be used. But unfortunately, we are stuck with it for now.

I will create a reproducer and update the ticket shortly. Should be fairly straightforward.

agnivade added a commit to mattermost/mattermost-server that referenced this issue Feb 27, 2020
@agnivade

This comment has been minimized.

Copy link
Contributor Author

@agnivade agnivade commented Feb 27, 2020

Ok, here's the branch - https://github.com/mattermost/mattermost-server/tree/syncgomod. I made some custom changes to add some files.

After cloning the branch, create a directory called enterprise in the parent directory. And a file in it containing:

package enterprise

import (
	"testing"

	_ "github.com/jonboulle/clockwork"
	_ "github.com/prometheus/client_model/go"
)

func TestDummy(t *testing.T) {

}

Then come to the root directory and create a symlink to that directory:
ln -s ../enterprise/ enterprise

The structure should be

path/to/dir
|- enterprise
|- mattermost-server
  |- api4
  |- app
  |- cmd
  |- ... 
  |- symlink to ../enterprise.

That's all. You should see the behavior. Let me know if you have any issues.

The github.com/russellhaering/goxmldsig change is still there. But so is the actual issue.

@agnivade agnivade removed their assignment Feb 27, 2020
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 4, 2020

The key to this instability is having a package within the main module that is by default not included in the pattern ./....

Symlinks are normally skipped, as are directories named testdata and those that begin with _ or .. However, if one of those packages is imported by a package within the main module, the dirInModule helper function sees that package and the dirInModule check claims that it is provided by the main module (because, at least according to the filesystem path, it is).

Then when we go to compute “directness”, the extraneous package contributes to the “direct” markings, even though that package is technically an indirect dependency of ./...:

// Compute directly referenced dependency modules.
ld.direct = make(map[string]bool)
for _, pkg := range ld.pkgs {
if pkg.mod == Target {
for _, dep := range pkg.imports {
if dep.mod.Path != "" {
ld.direct[dep.mod.Path] = true
}
}
}
}

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 4, 2020

Here is a much smaller reproducer that does not involve a symlink at all:

cp go.mod go.mod.orig
go mod tidy
cmp go.mod.orig go.mod
go mod vendor
cmp go.mod.orig go.mod

-- go.mod --
module example.com

go 1.14

require example.com/oblique v0.0.0

replace example.com/oblique => ./oblique
-- main.go --
package main

import _ "example.com/testdata"

func main() {}
-- testdata/testdata.go --
package testdata
-- testdata/testdata_test.go --
package testdata

import _ "example.com/oblique"
-- oblique/go.mod --
module example.com/oblique

go 1.14
-- oblique/oblique.go --
package oblique

--- FAIL: TestScript (0.01s)
    --- FAIL: TestScript/issue37376 (0.03s)
        script_test.go:193:
            > cp go.mod go.mod.orig
            > go mod tidy
            > cmp go.mod.orig go.mod
            > go mod vendor
            > cmp go.mod.orig go.mod
            [diff -go.mod.orig +go.mod]
             module example.com

             go 1.14

            -require example.com/oblique v0.0.0
            +require example.com/oblique v0.0.0 // indirect

             replace example.com/oblique => ./oblique

            FAIL: testdata/script/issue37376.txt:5: go.mod.orig and go.mod differ
@bcmills bcmills changed the title cmd/go: mod tidy and mod vendor fighting with each other cmd/go: "mod tidy" erroneously labels imports from within normally-ignored directories as direct Mar 4, 2020
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 4, 2020

I think the fix is to widen this check to cover only non-ignored packages in the main module:

if pkg.mod == Target {

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 4, 2020

(The reason go mod vendor labels the dependency here indirect is because it loads all of the packages it knows about within the module and doesn't see the import. That's because the import comes from a test of an indirectly-imported package that happens to also be within the filesystem of the main module.)

@agnivade

This comment has been minimized.

Copy link
Contributor Author

@agnivade agnivade commented Mar 4, 2020

@bcmills - Thanks for looking into this ! I guess this is not a regression right ? Would have been great to have this backported. Nothing too major, but it would have helped us enforce a "go mod tidy" check in our CI.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Mar 4, 2020

Does not appear to be a regression, no.

I'm doing some refactoring in the package and module loader for #36460 anyway, though, so I can probably knock it out incidentally in 1.15.

I'm curious about the use-case, though: why not treat github.com/mattermost/mattermost-server/enterprise as a separate module? You could publish a module containing a dummy package (with just a bunch of blank-imports) for go mod tidy stability for open-source users, and use a replace directive (instead of a symlink) to slot in a full-featured version for commercially-licensed users.

@agnivade

This comment has been minimized.

Copy link
Contributor Author

@agnivade agnivade commented Mar 5, 2020

why not treat github.com/mattermost/mattermost-server/enterprise as a separate module?

Heh .. well you are not the first person to raise this. AFAIU, it's mostly due to legal+legacy reasons than technical. But the replace directive is an interesting solution. Let me look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.