etcd-io / etcd

Distributed reliable key-value store for the most critical data of a distributed system
https://etcd.io
Apache License 2.0
46.77k stars 9.64k forks source link

make: fix `verify-dep` target #18205

Closed ivanvc closed 1 week ago

ivanvc commented 1 week ago

Part of #18180.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

k8s-ci-robot commented 1 week ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

ivanvc commented 1 week ago

/test all

ivanvc commented 1 week ago

This is an example of a failure (with the current mismatch in dependency versions from the main branch): https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18205/pull-etcd-verify/1803142325526859776

@ahrtr, could you PTAL at this? Thanks :)

henrybear327 commented 1 week ago

This is an example of a failure (with the current mismatch in dependency versions from the main branch): https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18205/pull-etcd-verify/1803142325526859776

@ahrtr, could you PTAL at this? Thanks :)

Just curious @ivanvc, is it possible to show the file paths where the inconsistent versions are extracted from? :)

ivanvc commented 1 week ago

Just curious @ivanvc, is it possible to show the file paths where the inconsistent versions are extracted from? :)

Without adding a lot of complexity to the code, I can achieve something like:

Found inconsistent dependency versions:
{
  "google.golang.org/genproto/googleapis/rpc": [
    "v0.0.0-20240318140521-94a12d6c2237",
    "v0.0.0-20240513163218-0867130af1f8",
    "v0.0.0-20240520151616-dc85e6b867a5"
  ],
  "golang.org/x/sys": [
    "v0.19.0",
    "v0.21.0"
  ]
}

google.golang.org/genproto/googleapis/rpc:
./client/v3/go.mod:     google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./etcdctl/go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./etcdutl/go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
./pkg/go.mod:   google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 // indirect
./tests/go.mod: google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
./tools/mod/go.mod:     google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./tools/testgrid-analysis/go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 // indirect
./server/go.mod:        google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
./api/go.mod:   google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect

golang.org/x/sys:
./client/pkg/go.mod:    golang.org/x/sys v0.19.0
./client/v3/go.mod:     golang.org/x/sys v0.21.0 // indirect
./etcdctl/go.mod:       golang.org/x/sys v0.21.0 // indirect
./etcdutl/go.mod:       golang.org/x/sys v0.21.0 // indirect
./pkg/go.mod:   golang.org/x/sys v0.21.0 // indirect
./tests/go.mod: golang.org/x/sys v0.21.0 // indirect
./tools/mod/go.mod:     golang.org/x/sys v0.21.0 // indirect
./tools/testgrid-analysis/go.mod:       golang.org/x/sys v0.21.0 // indirect
./server/go.mod:        golang.org/x/sys v0.21.0 // indirect
./api/go.mod:   golang.org/x/sys v0.21.0 // indirect
./go.mod:       golang.org/x/sys v0.21.0 // indirect
henrybear327 commented 1 week ago

Just curious @ivanvc, is it possible to show the file paths where the inconsistent versions are extracted from? :)

Without adding a lot of complexity to the code, I can achieve something like:

Found inconsistent dependency versions:
{
  "google.golang.org/genproto/googleapis/rpc": [
    "v0.0.0-20240318140521-94a12d6c2237",
    "v0.0.0-20240513163218-0867130af1f8",
    "v0.0.0-20240520151616-dc85e6b867a5"
  ],
  "golang.org/x/sys": [
    "v0.19.0",
    "v0.21.0"
  ]
}

google.golang.org/genproto/googleapis/rpc:
./client/v3/go.mod:     google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./etcdctl/go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./etcdutl/go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
./pkg/go.mod:   google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 // indirect
./tests/go.mod: google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
./tools/mod/go.mod:     google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./tools/testgrid-analysis/go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 // indirect
./server/go.mod:        google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect
./api/go.mod:   google.golang.org/genproto/googleapis/rpc v0.0.0-20240513163218-0867130af1f8 // indirect
./go.mod:       google.golang.org/genproto/googleapis/rpc v0.0.0-20240520151616-dc85e6b867a5 // indirect

golang.org/x/sys:
./client/pkg/go.mod:    golang.org/x/sys v0.19.0
./client/v3/go.mod:     golang.org/x/sys v0.21.0 // indirect
./etcdctl/go.mod:       golang.org/x/sys v0.21.0 // indirect
./etcdutl/go.mod:       golang.org/x/sys v0.21.0 // indirect
./pkg/go.mod:   golang.org/x/sys v0.21.0 // indirect
./tests/go.mod: golang.org/x/sys v0.21.0 // indirect
./tools/mod/go.mod:     golang.org/x/sys v0.21.0 // indirect
./tools/testgrid-analysis/go.mod:       golang.org/x/sys v0.21.0 // indirect
./server/go.mod:        golang.org/x/sys v0.21.0 // indirect
./api/go.mod:   golang.org/x/sys v0.21.0 // indirect
./go.mod:       golang.org/x/sys v0.21.0 // indirect

Looks good! Thanks for the quick work!

I think with this information it’s faster for reviewers to understand what’s doing on, as the source and type (direct, indirect, etc.) 😀 is very clear!

ivanvc commented 1 week ago

/test pull-etcd-verify

ivanvc commented 1 week ago

As I was working on this, I realized that there's a similar check already in place:

https://github.com/etcd-io/etcd/blob/043096067f2368e173c928581257e1ada305b593/scripts/test.sh#L533-L539

However, the approach is slightly different. This check runs against go list instead of go mod. The result from the list (removing the indirect check) opens a can of worms, as it lists the whole dependency tree (from go.sum). We can't update deep-level dependencies.

I wonder what approach we should follow, as the current didn't catch the inconsistencies we have right now.

Find attached the output log when removing the {{ if not .Indirect }} condition: verify-dep-output.log.

Update: I found a potential issue on how the current code lists duplicates. After fixing it, the list is shorter. However, AFAIK we don't have the ability to update those deep-level dependencies, as they don't show on the go.mod. Unless we force them in the go.mod, but a go mod tidy would remove them. Updated log output: verify-dep-output-fixed.log.

ivanvc commented 1 week ago

/test pull-etcd-verify

ivanvc commented 1 week ago

I updated this PR. I have two possible implementations:

  1. New script/Makefile target: 432274001fc6c574b85dca1ef0616edbf869784c. Intended prow failure: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18205/pull-etcd-verify/1803175651885191168
  2. Update verify-dep to use go mod edit rather than go list, scan for indirect dependencies in the go module, and fix an issue with matching the dependency name: 0bec4c59a6e07a4dfd4a6b2d78d15f442015345a. Intended prow failure: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18205/pull-etcd-verify/1803218126519668736

I think I prefer the latter option. @jmhbnz, it would be great to have your input too.

ahrtr commented 1 week ago

Great work, thanks @ivanvc

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18205/pull-etcd-verify/1803142325526859776

Checking dependencies for ./go.mod Found inconsistent dependency versions: { "google.golang.org/genproto/googleapis/rpc": [ "v0.0.0-20240318140521-94a12d6c2237", "v0.0.0-20240513163218-0867130af1f8", "v0.0.0-20240520151616-dc85e6b867a5" ], "golang.org/x/sys": [ "v0.19.0", "v0.21.0" ] } make: *** [Makefile:161: verify-dependency-versions] Error 1

Thanks for the finding.

I updated this PR. I have two possible implementations:

  1. New script/Makefile target: 4322740. Intended prow failure: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18205/pull-etcd-verify/1803175651885191168
  2. Update verify-dep to use go mod edit rather than go list, scan for indirect dependencies in the go module, and fix an issue with matching the dependency name: 0bec4c5. Intended prow failure: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18205/pull-etcd-verify/1803218126519668736

I think I prefer the latter option. @jmhbnz, it would be great to have your input too.

Yes, let's go for simpler solution. No need to add a separate script file

ivanvc commented 1 week ago

Updated the description and finalized commits + addressed dependency version mismatches.

/cc @ahrtr

ivanvc commented 1 week ago

@henrybear327, do you want to give it a try at backporting this to 3.5?

henrybear327 commented 1 week ago

@henrybear327, do you want to give it a try at backporting this to 3.5?

Yes! Thank you @ivanvc!

/assign @henrybear327