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

Run go mod tidy after go mod edit -replace #39

Merged
merged 4 commits into from
Feb 17, 2022
Merged

Conversation

mstoykov
Copy link
Contributor

This lets you not specify a correctly formatted version for a replace
but let you use branch names or latest or nothing (which is latest)

This lets you *not* specify a correctly formatted version for a replace
but let you use branch names or `latest` or nothing (which is `latest`)
@mstoykov mstoykov requested a review from imiric February 16, 2022 10:16
@mstoykov
Copy link
Contributor Author

I did test with a forked extension and with local repo for the main k6 project and both worked fine after the update so hopefully nothing is broken 🤞

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Hhmm this is breaking xk6 run from within the xk6-browser repo:

Log
$ XK6_BROWSER_LOG=info ~/Projects/grafana/xk6/xk6 run -q examples/226-waitfornavigation-timeout.js
2022/02/17 10:51:26 [INFO] Temporary folder: /tmp/buildenv_2022-02-17-1051.1260111165
2022/02/17 10:51:26 [INFO] Writing main module: /tmp/buildenv_2022-02-17-1051.1260111165/main.go
2022/02/17 10:51:26 [INFO] Initializing Go module
2022/02/17 10:51:26 [INFO] exec (timeout=10s): /home/ivan/.local/go/bin/go mod init k6
go: creating new go.mod: module k6
go: to add module requirements and sums:
        go mod tidy
2022/02/17 10:51:26 [INFO] Replace github.com/grafana/xk6-browser => /home/ivan/Projects/grafana/xk6-browser
2022/02/17 10:51:26 [INFO] exec (timeout=10s): /home/ivan/.local/go/bin/go mod edit -replace github.com/grafana/xk6-browser=/home/ivan/Projects/grafana/xk6-browser
2022/02/17 10:51:26 [INFO] exec (timeout=0s): /home/ivan/.local/go/bin/go mod tidy
go: finding module for package go.k6.io/k6/cmd
go: found github.com/grafana/xk6-browser in github.com/grafana/xk6-browser v0.0.0-00010101000000-000000000000
go: found go.k6.io/k6/cmd in go.k6.io/k6 v0.36.0
go: finding module for package gopkg.in/tomb.v1
go: found gopkg.in/tomb.v1 in gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7
k6 imports
        github.com/grafana/xk6-browser imports
        go.k6.io/k6/js/common imports
        github.com/serenize/snaker tested by
        github.com/serenize/snaker.test imports
        github.com/onsi/ginkgo imports
        github.com/onsi/ginkgo/internal/remote imports
        github.com/nxadm/tail imports
        github.com/nxadm/tail/watch imports
        github.com/fsnotify/fsnotify loaded from github.com/fsnotify/fsnotify@v1.4.7,
        but go 1.16 would select v1.4.9

To upgrade to the versions selected by go 1.16:
        go mod tidy -go=1.16 && go mod tidy -go=1.17
If reproducibility with go 1.16 is not needed:
        go mod tidy -compat=1.17
For other options, see:
        https://golang.org/doc/modules/pruning
2022/02/17 10:51:27 [INFO] Cleaning up temporary folder: /tmp/buildenv_2022-02-17-1051.1260111165
2022/02/17 10:51:27 [ERROR] exit status 1

Whereas it works on the current master. 😕 I'm on go 1.17.3.

And unrelated to these changes, but I'm actually noticing the same error from here and here, except when running xk6 build v0.36.0 --output xk6-browser --with github.com/grafana/xk6-browser@main (this happens both on the current xk6 master and on this branch). I wiped my cache just in case... 😕 EDIT: Hhmm for some reason v0.1.3 is always selected with @main or @917b83ef14b3e2075f92c092163c47e359883a6a, which is not compatible with k6 v0.36.0. Is this a regression or did it never work like this?

And a related comment: could you add some documentation for --replace in the README, with some usage examples? Currently it's not clear when it should be used for or what it accomplishes.

@mstoykov
Copy link
Contributor Author

And a related comment: could you add some documentation for --replace in the README, with some usage examples? Currently it's not clear when it should be used for or what it accomplishes.

This was done in #33 ?

@imiric
Copy link
Contributor

imiric commented Feb 17, 2022

I know 😄 But coming back to it I'm lapsing on when it's useful, and have to go back and read #32, which probably means the README could explain when it's useful, along with some examples. But you're right, it's unrelated to these changes, we can do that in another PR.

@mstoykov
Copy link
Contributor Author

It is ... literally what go mod edit -replace does ;) So the usefulness and the idea are the same ;)

@mstoykov
Copy link
Contributor Author

I think this is because of https://go.dev/doc/go1.17#go-command . This does not happen if you run with go1.18 and is unrelated to this change AFAIK as well.

I guess we can add -compat=1.17 and 🤞 that nobody using 1.16 will run it ?

@imiric
Copy link
Contributor

imiric commented Feb 17, 2022

It is ... literally what go mod edit -replace does ;) So the usefulness and the idea are the same ;)

Sure, but within the context of xk6, which is a wrapper around the go toolchain, we should explain when this is useful (e.g. when you want to replace a dependency of the extension without importing it), instead of "go see what go mod edit -replace does".

I guess we can add -compat=1.17 and 🤞 that nobody using 1.16 will run it ?

I wouldn't want to make 1.17 the minimum required version, but if that's the only way of getting this to work, then sure. In that case, let's update the README and go.mod to indicate that.

@mstoykov
Copy link
Contributor Author

This will now error out with

2022/02/17 13:10:37 [INFO] Temporary folder: /tmp/buildenv_2022-02-17-1310.2750852615
2022/02/17 13:10:37 [INFO] Writing main module: /tmp/buildenv_2022-02-17-1310.2750852615/main.go
2022/02/17 13:10:37 [INFO] Initializing Go module
2022/02/17 13:10:37 [INFO] exec (timeout=10s): /home/mstoykov/go/bin/go mod init k6
go: creating new go.mod: module k6
go: to add module requirements and sums:
        go mod tidy
2022/02/17 13:10:37 [INFO] Pinning versions
2022/02/17 13:10:37 [INFO] exec (timeout=0s): /home/mstoykov/go/bin/go mod edit -require go.k6.io/k6@v0.36.0
2022/02/17 13:10:37 [INFO] exec (timeout=0s): /home/mstoykov/go/bin/go mod tidy -compat=1.17
flag provided but not defined: -compat
usage: go mod tidy [-e] [-v]
Run 'go help mod tidy' for details.
2022/02/17 13:10:37 [INFO] Cleaning up temporary folder: /tmp/buildenv_2022-02-17-1310.2750852615
2022/02/17 13:10:37 [FATAL] exit status 2

which is not great but I guess it's somewhat better than erroring out for everybody using the latest version (which I would argue are the majority of the people - especially new users).

Unfortunately capturing the output and checking it will be ... a lot of work which arguably will be outdated in few months when 1.18 is more ubiqutious and we in general no longer support go1.16. Or even depend on 1.18 in k6 or an extension 🤷 .

@mstoykov mstoykov requested a review from imiric February 17, 2022 11:16
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Thanks, @<commit-ish> works properly now 🎉

To avoid that obscure flag provided but not defined: -compat error, should we just check go version up front and error out with a friendlier message if it's <1.17?

@mstoykov
Copy link
Contributor Author

mstoykov commented Feb 17, 2022

Getting the output of a command seems to involve a lot of work which IMO will not really payoff given that 1.17 is now 6 months old and 1.18 is getting out soon ™️ . I would expect by the time people update to this version xk6 1.16 will not be supported by the go team either way. Which doesn't mean that people will not use it, but hopefully it will be in smaller numbers 🤞
I prefer to see how it goes and try to mitigate it if it's a problem

@mstoykov mstoykov merged commit 65e187a into master Feb 17, 2022
@mstoykov mstoykov deleted the goModAfterReplace branch February 17, 2022 14:31
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

2 participants