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

go.mod: sort and move self managed indirect dependencies to first block #2348

Merged
merged 1 commit into from Sep 20, 2021

Conversation

zchee
Copy link
Contributor

@zchee zchee commented Sep 6, 2021

As @thaJeztah suggested on #2331 (comment), Sort and moved self-managed indirect dependencies and pinned (replace ) to the new block.

@zchee
Copy link
Contributor Author

zchee commented Sep 6, 2021

If there are more self-managed packages, post comments, please. I'll move the suggested package to the new require block.

@zchee zchee force-pushed the gomod-resort branch 2 times, most recently from 18fcb44 to 5b07194 Compare September 6, 2021 17:43
@thaJeztah
Copy link
Member

If there are more self-managed packages, post comments, please. I'll move the suggested package to the new require block.

I tried to discover which of these are "manual" overrides. What I did to discover that;

  1. checkout current "master" branch
  2. remove all packages from the second require except for github.com/golang/snappy (as we know that one was manual) to keep "two" blocks
  3. ran make vendor to have go modules itself add the other indirects

After the above, the diff should show those dependencies for which the version in go.mod didn't match the version that was automatically found by go modules, so I ran:

git diff -- go.mod

Which gave me:

diff --git a/go.mod b/go.mod
index 5e1f3cd2..39d81b13 100644
--- a/go.mod
+++ b/go.mod
@@ -24,6 +24,7 @@ require (
 	github.com/gofrs/flock v0.7.3
 	github.com/gogo/googleapis v1.4.0
 	github.com/gogo/protobuf v1.3.2
+	github.com/golang/protobuf v1.5.2
 	github.com/google/go-cmp v0.5.6
 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
 	github.com/grpc-ecosystem/go-grpc-middleware v1.2.0
@@ -71,7 +72,7 @@ require (
 )
 
 require (
-	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
+	github.com/Azure/go-ansiterm v0.0.0-20170929234023-d6e3b3328b78 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/bits-and-blooms/bitset v1.2.0 // indirect
 	github.com/cenkalti/backoff/v4 v4.1.1 // indirect
@@ -89,7 +90,6 @@ require (
 	github.com/docker/go-units v0.4.0 // indirect
 	github.com/felixge/httpsnoop v1.0.2 // indirect
 	github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect
-	github.com/golang/protobuf v1.5.2
 	// snappy: updated for go1.17 support
 	github.com/golang/snappy v0.0.4-0.20210608040537-544b4180ac70 // indirect
 	github.com/google/uuid v1.2.0 // indirect
@@ -97,12 +97,12 @@ require (
 	github.com/hanwen/go-fuse/v2 v2.1.0 // indirect
 	github.com/hashicorp/errwrap v1.0.0 // indirect
 	github.com/hashicorp/go-multierror v1.1.1 // indirect
-	github.com/ishidawataru/sctp v0.0.0-20210226210310-f2269e66cdee // indirect
+	github.com/ishidawataru/sctp v0.0.0-20191218070446-00ab2ac2db07 // indirect
 	github.com/klauspost/compress v1.12.3 // indirect
 	github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
-	github.com/moby/sys/mount v0.2.0 // indirect
+	github.com/moby/sys/mount v0.1.1 // indirect
 	github.com/moby/sys/mountinfo v0.4.1 // indirect
-	github.com/moby/term v0.0.0-20210619224110-3f7ff695adc6 // indirect
+	github.com/moby/term v0.0.0-20200915141129-7f0af18e79f2 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_golang v1.11.0 // indirect
 	github.com/prometheus/client_model v0.2.0 // indirect

So those are the ones that were set manually, and the ones we should keep separate from the auto-generated ones.

@thaJeztah
Copy link
Member

(Looks like github.com/golang/protobuf also accidentally landed in the wrong block)

go.mod Outdated
Comment on lines 73 to 74
// self managed
require (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to keep these together with the first group (but open to suggestions from others)

go.mod Outdated
Comment on lines 75 to 76
// snappy: updated for go1.17 support
github.com/golang/snappy v0.0.4-0.20210608040537-544b4180ac70 // indirect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that go.mod allows additional comments to be put after // indirect by adding a semi-colon (;) followed by the comment.

Perhaps we should use that notation so that the comment explaining why we have the override is there, e.g.;

Suggested change
// snappy: updated for go1.17 support
github.com/golang/snappy v0.0.4-0.20210608040537-544b4180ac70 // indirect
github.com/golang/snappy v0.0.4-0.20210608040537-544b4180ac70 // indirect; snappy: updated for go1.17 support

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we might be able to revert the snappy change golang/snappy#61 (comment) but can be separate from here and I haven't verified myself. Seems like the breakage may have only been in rc releases.

@zchee
Copy link
Contributor Author

zchee commented Sep 14, 2021

@thaJeztah @tonistiigi Fixed. I'll describe more details later.

thaJeztah
thaJeztah previously approved these changes Sep 15, 2021
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

go.mod Outdated Show resolved Hide resolved
@thaJeztah thaJeztah dismissed their stale review September 15, 2021 17:43

removing my LGTM (see above)

@zchee
Copy link
Contributor Author

zchee commented Sep 15, 2021

@thaJeztah @tonistiigi done, PTAL.

go.mod Outdated
github.com/google/uuid v1.2.0 // indirect
github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect
github.com/hanwen/go-fuse/v2 v2.1.0 // indirect
github.com/hashicorp/errwrap v1.0.0 // indirect
github.com/hashicorp/go-multierror v1.1.1 // indirect
github.com/ishidawataru/sctp v0.0.0-20210226210310-f2269e66cdee // indirect
github.com/klauspost/compress v1.12.3
github.com/ishidawataru/sctp v0.0.0-20191218070446-00ab2ac2db07 // indirect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this one was downgraded in the last iteration (or did I miss that in the previous iteration as well?) can you keep it at the same version as what it was? (in that case, it may have to go into the first block)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noticed that one module was downgraded; can you also squash the commits?

@zchee
Copy link
Contributor Author

zchee commented Sep 19, 2021

@thaJeztah done. PTAL.

@thaJeztah
Copy link
Member

@zchee hm... could it be you forgot to make a change? Looks like github.com/ishidawataru/sctp is still at the wrong commit (and commits not squashed)

Let me know if you need help making the changes

@zchee
Copy link
Contributor Author

zchee commented Sep 20, 2021

@thaJeztah ah, you means squash all commit messages? I see, will do.
Also, fix your pointed out.

@thaJeztah
Copy link
Member

ah, you means squash all commit messages? I see, will do.

Yes, if it's only moving the modules to a different block, I think it makes sense to have the changes in a single commit

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented Sep 20, 2021

@thaJeztah Sorry for taking your time. PTAL.

@zchee zchee changed the title go.mod: sort and move self managed indirect dependencies to new block go.mod: sort and move self managed indirect dependencies to first block Sep 20, 2021
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking your time. PTAL.

No worries, thanks for contributing!

LGTM

@thaJeztah
Copy link
Member

@tonistiigi @crazy-max PTAL

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zchee
Copy link
Contributor Author

zchee commented Sep 20, 2021

JFYI: Go core also split two blocks
golang/go@3c764ba

@tonistiigi tonistiigi merged commit e878d4d into moby:master Sep 20, 2021
@zchee zchee deleted the gomod-resort branch September 20, 2021 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants