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

Replace dep with Go modules #1584

Merged
merged 5 commits into from
Aug 14, 2020
Merged

Replace dep with Go modules #1584

merged 5 commits into from
Aug 14, 2020

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Aug 11, 2020

Closes #1070

This was prioritized because dep has an issue importing https://github.com/semihalev/sdns, which we're evaluating for the DNS fixes.

There are some minor differences between go.mod and the previous Gopkg.lock. The missing packages are indirect and seem fine, and the version differences shouldn't cause problems, but let me know if they should be explicitly overriden.

  • In go.mod but not in Gopkg.lock:
github.com/Shopify/toxiproxy v2.1.4+incompatible // indirect  (from github.com/Shopify/sarama)
github.com/google/go-cmp v0.5.1 // indirect                   (from github.com/andybalholm/brotli)
github.com/klauspost/cpuid v1.3.1 // indirect                 (from github.com/klauspost/compress/zstd)
github.com/onsi/ginkgo v1.14.0 // indirect                    (from github.com/manyminds/api2go)
  • Different versions in go.mod compared to (=>) Gopkg.lock:
github.com/stretchr/testify v1.2.2                      => v1.2.1
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2  => c7dcf104e3a7a1417abc0230cb0d5240d764159d
golang.org/x/net v0.0.0-20200520004742-59133d7f0dd7     => 351d144fa1fc0bd934e2408202be0c29f25e35a0
golang.org/x/text v0.3.2                                => v0.3.0
gopkg.in/yaml.v2 v2.3.0                                 => v2.1.1

@imiric imiric requested review from mstoykov and na-- August 11, 2020 11:24
@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2020

Codecov Report

Merging #1584 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1584      +/-   ##
==========================================
- Coverage   76.11%   76.09%   -0.03%     
==========================================
  Files         163      163              
  Lines       13607    13607              
==========================================
- Hits        10357    10354       -3     
- Misses       2723     2726       +3     
  Partials      527      527              
Impacted Files Coverage Δ
lib/testutils/minirunner/minirunner.go 78.57% <0.00%> (-3.58%) ⬇️
lib/executor/vu_handle.go 95.49% <0.00%> (-2.71%) ⬇️
stats/cloud/collector.go 79.23% <0.00%> (+0.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 691ef2d...1e23d03. Read the comment docs.

@na--
Copy link
Member

na-- commented Aug 11, 2020

Something fishy is going on 😕 +180,579 −110,284 Files changed 818 Are you sure you haven't updated versions or something like that, there shouldn't be so many changes when you're just switching dependency managers and not upgrading the actual dependencies 😕

@imiric
Copy link
Contributor Author

imiric commented Aug 11, 2020

@na-- These were produced by go mod vendor/go mod tidy. I was also surprised by the amount, but it seems go mod is pulling in lots more files than dep ensure did.

@na--
Copy link
Member

na-- commented Aug 11, 2020

I'd be fine if it was just pulling more files, maybe because it wasn't pruning or tree-shaking as much as dep, but look at the diff... There are differences in old files, which suggests that different versions were pulled.

I want to update our dependencies soon, but that should be in a separate PR - this one should just encapsulate the switch of dependency manager. Please investigate what's happening, something is fishy.

@mstoykov
Copy link
Collaborator

From my memories of when I tried to this a year ago ... one of the reason for changing versions is that now go mod actually looks at go.mod which tells it that we need version X of given dependancy ... which is not how dep works ;)

@imiric
Copy link
Contributor Author

imiric commented Aug 11, 2020

So it's a combination of several things:

  • go mod vendor pulls in more (unrelated) files than dep ensure. See the goja changes even though the version remained the same:
> git show b72ffed1 --stat -- vendor/github.com/dop251/goja | grep '^ vendor'
 vendor/github.com/dop251/goja/.gitignore           |    3 +
 vendor/github.com/dop251/goja/.travis.yml          |   17 +
 vendor/github.com/dop251/goja/README.md            |  277 +++++
 vendor/github.com/dop251/goja/ast/README.markdown  | 1068 ++++++++++++++++++++
 vendor/github.com/dop251/goja/file/README.markdown |  110 ++
 vendor/github.com/dop251/goja/token/Makefile       |    2 +
 vendor/github.com/dop251/goja/token/tokenfmt       |  222 ++++
  • Half of the changes are because many of the golang.org/x/* packages were updated.
> git show b72ffed1 --stat -- vendor/golang.org/x | grep 'files changed'
 420 files changed, 79116 insertions(+), 105655 deletions(-)

For example, golang.org/x/crypto was updated from golang/crypto@c7dcf10 to golang/crypto@c2843e01d9a2 .

I'm not sure why these were updated, but since we were using very old versions, and these are semi-stdlib packages, we should consider updating them anyway, though we could pin them for now and do that later. Let me know.

  • github.com/golang/protobuf/google.golang.org/protobuf are transitive dependencies that also got updated, and ended up pulling in a bunch of files:
> git show b72ffed1 --stat | grep protobuf | wc -l
136

@na--
Copy link
Member

na-- commented Aug 11, 2020

As I said, I'm fine with the READMEs and new files that were previously pruned. I'd strongly prefer if we can leave the changes to actual dependencies in a separate PR, even golang.org/x and protobuf... Btw, why do we even have protobuf? 😅

@imiric
Copy link
Contributor Author

imiric commented Aug 11, 2020

OK, I'll see if I can pin the golang.org/x packages, but there's a few of them 😅

As for protobuf:

> go mod why -m google.golang.org/protobuf
# google.golang.org/protobuf
github.com/loadimpact/k6/api/v1
github.com/manyminds/api2go
github.com/manyminds/api2go/routing
github.com/gin-gonic/gin
github.com/gin-gonic/gin/binding
github.com/golang/protobuf/proto
google.golang.org/protobuf/encoding/prototext

😞

@imiric
Copy link
Contributor Author

imiric commented Aug 11, 2020

@na-- I version pinned some of the dependencies and the diff is much cleaner now.

All additions now are just extra unrelated files that go mod vendor pulls in, and all changes to .go files are deletions, likely because of better tree shaking.

@imiric
Copy link
Contributor Author

imiric commented Aug 11, 2020

BTW, should this be fixed?

On this branch:

> go run main.go version
k6 v0.27.1 ((devel), go1.14.3, linux/amd64)

On master:

> go run main.go version
k6 v0.27.1 (dev build, go1.14.3, linux/amd64)

mstoykov
mstoykov previously approved these changes Aug 12, 2020
Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM!

Went through the files, the missing ones are because they are //build +ignore which in most cases is because those are files to generate other files so ... yeah :D.

About the version thing .. this is because of https://golang.org/pkg/runtime/debug/#ReadBuildInfo now returns something ... which is what we want, I think the slight difference is not a problem ;)

imiric pushed a commit that referenced this pull request Aug 12, 2020
If this works in all cases, it would elegantly solve
#1584 (comment) .
imiric pushed a commit that referenced this pull request Aug 12, 2020
If this works in all cases, it would solve
#1584 (comment) .
@imiric imiric force-pushed the build/1070-go-modules branch 2 times, most recently from 844578c to ec5f107 Compare August 12, 2020 09:09
@imiric
Copy link
Contributor Author

imiric commented Aug 12, 2020

Unfortunately, this doesn't fix flaky tests. 😆

Went through the files, the missing ones are because they are //build +ignore which in most cases is because those are files to generate other files so ... yeah :D.

Ah, that makes sense, thanks. I just wish there could be a way of configuring go mod to ignore certain files from the user's side. :-/

Serious question: what do you think about ignoring some of these at least at the repo level? Something like /vendor/**/Makefile or whatever the .gitignore syntax is. And even specific files like this 108k test data file: vendor/github.com/dlclark/regexp2/testoutput1. I guess we shouldn't blindly ignore all /vendor/**/*.yml as those could be functionally significant, but Makefiles and test data seem safe to ignore.

@mstoykov
Copy link
Collaborator

Serious question: what do you think about ignoring some of these at least at the repo level?

This will break the go mod check ... as long as they don't change I don't think it's that big of an issue ;)

@na--
Copy link
Member

na-- commented Aug 12, 2020

Serious question: what do you think about ignoring some of these at least at the repo level?

This will break the go mod check ... as long as they don't change I don't think it's that big of an issue ;)

I'm not sure it will break the check. Considering that you can't check the integrity of the vendor folder with go mod commands yet, the way we'll do it is with running go mod vendor and seeing if anything in git has changed. But if the files are in .gitignore, then nothing will appear changed, even if you go mod vendor; git add . again 🎉

So, @imiric, my vote is 👍 for ignoring Makefiles, READMEs, *_test.go files, and yes, especially single 100k+ test fixture files 😅

imiric pushed a commit that referenced this pull request Aug 12, 2020
If this works in all cases, it would solve
#1584 (comment) .
imiric pushed a commit that referenced this pull request Aug 12, 2020
@imiric
Copy link
Contributor Author

imiric commented Aug 12, 2020

golangci-lint seems to be failing more often that not these days 😞

I added some .gitignore entries and it removed about 50 files. I don't want to remove READMEs and .md files in general, since these are usually helpful when you're working with a dependency and want to quickly check the documentation. rg/grep picks up examples in the docs, so it's handy to leave them in. *_test.go files are always excluded by go mod so no need to specify those.

@imiric imiric requested review from na-- and mstoykov August 12, 2020 15:57
@mstoykov
Copy link
Collaborator

I am not really certain the *.md files are worth it .. but can we also drop .png and .txt ?

@imiric
Copy link
Contributor Author

imiric commented Aug 13, 2020

can we also drop .png and .txt ?

Not sure, those could be functionally significant 😄

imiric pushed a commit that referenced this pull request Aug 13, 2020
If this works in all cases, it would solve
#1584 (comment) .
imiric pushed a commit that referenced this pull request Aug 13, 2020
imiric pushed a commit that referenced this pull request Aug 14, 2020
If this works in all cases, it would solve
#1584 (comment) .
imiric pushed a commit that referenced this pull request Aug 14, 2020
@imiric
Copy link
Contributor Author

imiric commented Aug 14, 2020

The latest .gitignore changes removed about 20 more files, and the additions now are mostly *.md, .gitignore and go.mod. There are some .pl, .sh and .py files, but let's leave those in.

@na--
Copy link
Member

na-- commented Aug 14, 2020

@imiric, going through all the files in just in case and I noticed there are some surprising differences with vendor/github.com/spf13/cobra 😕 Any idea why so many files were deleted?

@na--
Copy link
Member

na-- commented Aug 14, 2020

Ah, nevermind, I think I answered my own question 😅 They seem like the non-library parts, an app you can use to generate your cobra boilerplate and docs or something like that...

@imiric
Copy link
Contributor Author

imiric commented Aug 14, 2020

Yeah, I think this is just better pruning by go mod. We are using the same version: a114f312e075.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM, there are only a few more things we might want to ignore, like the perl scripts in vendor/golang.org/x/sys/unix/, but they're not a big deal...

@imiric imiric merged commit bc31944 into master Aug 14, 2020
@imiric imiric deleted the build/1070-go-modules branch August 14, 2020 11:02
@imiric imiric mentioned this pull request Aug 14, 2020
@na-- na-- added this to the v0.28.0 milestone Aug 14, 2020
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.

Adopt Go modules
4 participants