vendor: use go mod by errordeveloper · Pull Request #43075 · moby/moby

errordeveloper · 2021-12-10T13:19:31Z

No description provided.

errordeveloper · 2021-12-10T14:06:13Z

vendor.mod

+	google.golang.org/genproto => google.golang.org/genproto v0.0.0-20200227132054-3f1135a288c9
+	google.golang.org/grpc => google.golang.org/grpc v1.27.1
+	gopkg.in/fsnotify.v1 => gopkg.in/fsnotify.v1 v1.4.7
+        archive/tar => ./internal/archive/tar


So this actually isn't supported (see golang/go#35283). We need to find a better way to do this, perhaps for now there could be cp -a internal/archive/tar inside of go mod vendor wrapper.

If we keep the files inside vendor/, does go mod vendor delete them, or does it leave them alone?

also wondering if a symlink would work (although Windows may not be happy with that, I recall there were some issues with Windows and symlinks, but don't recall the details)

If we keep the files inside vendor/, does go mod vendor delete them, or does it leave them alone?

It deletes them.

also wondering if a symlink would work (although Windows may not be happy with that, I recall there were some issues with Windows and symlinks, but don't recall the details)

The issue with symlinks on Windows is pretty major IMO, a Windows user basically cannot clone a repo with symlinks.

Maybe I asked before, but why can't we move vendor/archive/tar to an external module such as github.com/moby/sys/archive/tar and just apply s|archive/tar|github.com/moby/sys/archive/tar|g to the moby/moby source?

I think it might be worse trying as a follow-up, but here the goal is to make the least number of changes.

@AkihiroSuda In that case, we could as well put it in internal/ in this repository. But I think the reason we didn't "fork" it (but override) is that it may be used indirectly(?). Are we sure that the archive/tar code is never called indirectly (and we would reintroduce the vulnerability?)

thaJeztah · 2021-12-13T11:32:07Z

Also opened docker/docker-ce-packaging#611 to do a quick check if the packaging scripts don't blow up 👍

crazy-max · 2021-12-13T12:44:03Z

hack/vendor.sh

+cat > hack/make/.resources-windows/go.mod << EOF
+module github.com/docker/docker/autogen/winresources/dockerd


For this I can open a PR to implement goversioninfo like it has been done in docker/cli#3310 so you would not need to add this hack. WDYT @thaJeztah?

Yes, would be great if we could used the same tool.

I'm ok with a temporary hack like this (if it works in the meantime)

I think it'd be good as a follow-up, but doesn't seem like a blocker...

errordeveloper · 2021-12-13T19:56:04Z

This should be good as far as I'm concerned, but let's see what CI might bring...

hack/vendor.sh

thaJeztah

Thanks! Did some quick testing, and looks like there may be some issues (see comments)

.gitignore

hack/vendor.sh

hack/validate/vendor

thaJeztah · 2021-12-20T15:40:06Z

I noticed that the temporary go.mod files that we generate are not cleaned up after vendor.sh completes. They are added to .gitignore, so not "critical", but perhaps (as a follow up?) we should clean those up, so that running vendor.sh leaves a clean state afterwards.

errordeveloper · 2021-12-20T15:47:13Z

I noticed that the temporary go.mod files that we generate are not cleaned up after vendor.sh completes. They are added to .gitignore, so not "critical", but perhaps (as a follow up?) we should clean those up, so that running vendor.sh leaves a clean state afterwards.

Maybe, but I am not quite sure why this matters. With these files written one can actually use go mod commands directly, without having to always use vendor.sh, which I find quite convenient.

thaJeztah · 2021-12-20T15:48:44Z

With these files written one can actually use go mod commands directly, without having to always use vendor.sh, which I find quite convenient.

Ah, good point as well, didn't think of that

thaJeztah · 2021-12-20T15:51:13Z

One caveat with that is that it still requires the -modfile flag;

go mod graph | grep ' github.com/BurntSushi/toml'
# nothing

go mod graph -modfile=vendor.mod | grep ' github.com/BurntSushi/toml'
github.com/docker/docker github.com/BurntSushi/toml@v0.3.1
github.com/moby/buildkit@v0.8.2-0.20210615162540-9f254e18360a github.com/BurntSushi/toml@v0.3.1
github.com/urfave/cli@v1.22.2 github.com/BurntSushi/toml@v0.3.1
github.com/urfave/cli@v1.22.1 github.com/BurntSushi/toml@v0.3.1
github.com/urfave/cli@v1.22.4 github.com/BurntSushi/toml@v0.3.1

thaJeztah

LGTM

errordeveloper · 2021-12-20T16:45:05Z

There's also some dependencies that we previously were able to avoid (by only consuming certain packages), but are now pulled in because go.mod mentions them and/or because some repositories are using a tools.go (or equivalent) to have an easy way to download test dependencies.

I guess the way I think about this is that new sub-decencies are not a major concern, as long as these aren't actually imported, which I checked for this PR.

errordeveloper · 2021-12-20T16:46:19Z

@thaJeztah thanks for sharing go mod graph results for new deps, I think you need to tweak formatting a little to work with <details/>.

thaJeztah · 2021-12-20T16:59:46Z

I think you need to tweak formatting a little to work with <details/>.

🤦 I always forget the empty line; should be fixed now

tonistiigi

When I run hack/validate/vendor inside the container the vendor.sum file changes significantly. But the validation script itself returns with 0 exit code.

tonistiigi · 2021-12-20T22:15:23Z

vendor.mod

+	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
+	google.golang.org/api v0.46.0 // indirect
+	google.golang.org/appengine v1.6.7 // indirect
+	honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc // indirect


I see we are now vendoring some new packages that actually never get called. Nothing we can do about it?

No, don't think so unfortunately. Some of these may be test dependencies or a dependency using a "tools.go" file to force it being included. (also see my comment above)

errordeveloper · 2021-12-21T10:20:52Z

hack/validate/vendor

 	IFS=$'\n'
 	# shellcheck disable=SC2207
-	files=( $(validate_diff --diff-filter=ACMR --name-only -- 'vendor.conf' 'vendor/' || true) )
+	files=( $(validate_diff --diff-filter=ACMR --name-only -- 'vendor.sum' 'vendor.mod' 'vendor/' || true) )


@tonistiigi thanks for checking this, well spotted! I have looked into this and it doesn't seem to detect uncommitted changes to any of these files, it seems to be just getting a list of files that changed between origin/HEAD and HEAD.

When I ran bash -x hack/validate/vendor, the command it called was this:

+ git diff 088afc99e4bf8adb78e29733396182417d67ada2...0089b1ffb718e9c413a0687dd714dfc5c0829cb4 --diff-filter=ACMR --name-only -- vendor.sum vendor.mod vendor/

Where 088afc9 is what's in master right now, and 0089b1f is the head of my branch.

I think that changing the way this script functions would be out of scope for this PR, so I'd rather leave it as is. I know that overall diff in this script is quite big already, but I was aiming to avoid changing the logic of the checks.

Doesn't at least line 21 need vendor.mod and vendor.sum as well?

Doesn't at least line 21 need vendor.mod and vendor.sum as well?

You are right, that looks like where detection of uncommitted changes happens.

I can also see what validate_diff is all about, it checks what files changed in HEAD vs origin/HEAD.
I just tend to use git diff to check for uncommitted changes, so I too quickly assumed that's what validate_diff might be for, the function name isn't very specific...

I've updated this script just now.

errordeveloper · 2021-12-21T10:33:04Z

When I run hack/validate/vendor inside the container the vendor.sum file changes significantly. But the validation script itself returns with 0 exit code.

I've committed an update to vendor.sum just now, see #43075 (comment) regarding the behaviour of the validation script...

- use `vendor.mod` instead of `go.mod` to avoid issues to do with use of CalVer, not SemVer - ensure most of the dependency versions do not change - only zookeeper client has to change (via docker/libkv#218) as previously used version is no longer maintained and has missing dependencies Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

- all changes here are attributed to difference in behaviour between, namely: - resolution of secondary test dependencies - prunning of non-Go files Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

thaJeztah · 2021-12-22T17:09:58Z

@errordeveloper I was a bit in doubt in which order we should merge the PR's;

#42247 was green now (with the test-fixes merged), and has some commits that remove a couple of dependencies. I gave it a quick try to rebase this (your) PR on top of that one, to see how easy we can do a rebase (and update the vendor.mod etc accordingly; #43101

I can also do a try to see what the "reverse" will look like (have not tried yet)

errordeveloper · 2021-12-22T17:54:42Z

@thaJeztah ok, do give it a go! I can pick this back up on Friday, if needed.

errordeveloper · 2022-01-10T11:24:28Z

closing in favour of #43101

errordeveloper commented Dec 10, 2021

View reviewed changes

errordeveloper force-pushed the go-mod branch 2 times, most recently from 48be7a1 to d98bff4 Compare December 13, 2021 10:32

thaJeztah mentioned this pull request Dec 13, 2021

[do not merge] testing go mod vendoring PRs docker/docker-ce-packaging#611

Closed

crazy-max reviewed Dec 13, 2021

View reviewed changes

errordeveloper force-pushed the go-mod branch 2 times, most recently from 454c66a to 9e83d6c Compare December 13, 2021 19:39

errordeveloper marked this pull request as ready for review December 13, 2021 19:55

errordeveloper requested a review from tianon as a code owner December 13, 2021 19:55

errordeveloper commented Dec 13, 2021

View reviewed changes

hack/vendor.sh Show resolved Hide resolved

thaJeztah requested changes Dec 14, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

hack/vendor.sh Outdated Show resolved Hide resolved

hack/vendor.sh Outdated Show resolved Hide resolved

errordeveloper force-pushed the go-mod branch 7 times, most recently from 85051f9 to 2ab6dcc Compare December 14, 2021 14:15

errordeveloper requested a review from thaJeztah December 14, 2021 14:42

errordeveloper force-pushed the go-mod branch from 2ab6dcc to 3274205 Compare December 14, 2021 14:51

errordeveloper mentioned this pull request Dec 14, 2021

vendor with go mod docker/cli#3387

Merged

errordeveloper force-pushed the go-mod branch 3 times, most recently from 18c3503 to dc7a938 Compare December 14, 2021 16:07

errordeveloper commented Dec 14, 2021

View reviewed changes

hack/validate/vendor Show resolved Hide resolved

errordeveloper commented Dec 14, 2021

View reviewed changes

hack/validate/vendor Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

errordeveloper force-pushed the go-mod branch from edee5ed to 47f86be Compare December 14, 2021 19:59

thaJeztah approved these changes Dec 20, 2021

View reviewed changes

tonistiigi reviewed Dec 20, 2021

View reviewed changes

errordeveloper commented Dec 21, 2021

View reviewed changes

errordeveloper force-pushed the go-mod branch from 0089b1f to 2e64c14 Compare December 21, 2021 10:32

errordeveloper force-pushed the go-mod branch 2 times, most recently from ba0a704 to 38fe32e Compare December 22, 2021 12:17

vendor: regenerate

2a6c38e

- all changes here are attributed to difference in behaviour between, namely: - resolution of secondary test dependencies - prunning of non-Go files Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>

errordeveloper force-pushed the go-mod branch from 38fe32e to 2a6c38e Compare December 22, 2021 12:25

thaJeztah added area/project kind/refactor PR's that refactor, or clean-up code status/2-code-review labels Dec 22, 2021

thaJeztah added this to the 21.xx milestone Dec 22, 2021

tonistiigi approved these changes Dec 22, 2021

View reviewed changes

thaJeztah mentioned this pull request Dec 22, 2021

vendor: use go mod (rebase after removal of deprecated host-discovery) #43101

Merged

thaJeztah mentioned this pull request Jan 6, 2022

Convert "vendor.conf" to "go.mod" with minimal changes [DNM] #42489

Closed

errordeveloper closed this Jan 10, 2022

thaJeztah mentioned this pull request Feb 5, 2022

hack/vendor.sh: run "go mod tidy" before vendoring #43212

Merged

thaJeztah mentioned this pull request Feb 15, 2022

vendor.mod: reduce number of replace rules: update some dependencies #43236

Merged

		cat > hack/make/.resources-windows/go.mod << EOF
		module github.com/docker/docker/autogen/winresources/dockerd

Conversation

errordeveloper commented Dec 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 13, 2021

Uh oh!

crazy-max Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errordeveloper commented Dec 13, 2021

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

thaJeztah commented Dec 20, 2021

Uh oh!

errordeveloper commented Dec 20, 2021

Uh oh!

thaJeztah commented Dec 20, 2021

Uh oh!

thaJeztah commented Dec 20, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

errordeveloper commented Dec 20, 2021

Uh oh!

errordeveloper commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Dec 20, 2021

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errordeveloper Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errordeveloper commented Dec 21, 2021

Uh oh!

thaJeztah commented Dec 22, 2021

Uh oh!

errordeveloper commented Dec 22, 2021

Uh oh!

errordeveloper commented Jan 10, 2022

Uh oh!

crazy-max Dec 13, 2021 •

edited

Loading

errordeveloper commented Dec 20, 2021 •

edited

Loading

errordeveloper Dec 21, 2021 •

edited

Loading