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 modules #74

Merged
merged 9 commits into from
Jul 3, 2019
Merged

Go modules #74

merged 9 commits into from
Jul 3, 2019

Conversation

jesseduffield
Copy link
Owner

@jesseduffield jesseduffield commented Jul 2, 2019

to be honest I have no idea what I'm doing here. I just hope I'm making the right decision 😢 and this doesn't make things harder in the long run

@codecov-io
Copy link

codecov-io commented Jul 2, 2019

Codecov Report

Merging #74 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #74   +/-   ##
=======================================
  Coverage   23.61%   23.61%           
=======================================
  Files          13       13           
  Lines        1080     1080           
=======================================
  Hits          255      255           
  Misses        813      813           
  Partials       12       12

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 8abffd7...999d12a. Read the comment docs.

@jesseduffield jesseduffield mentioned this pull request Jul 2, 2019
@tommyknows
Copy link

LGTM in general.
I'd recommend tagging at least your repositories with valid SemVer tags, so you can rely on go modules when making possible (breaking) changes.
(And it looks better than a commit ID 🙂)

If you should have any questions regarding the new modules, don't hesitate to ping me. I'll be happy to help (if I can 😉)

@mcintyre94
Copy link
Contributor

Newbie question - would it be worth an update to contributing.md to show how to build locally after this if anything changes? And what would the equivalent of dep ensure be after this? :)

@tommyknows
Copy link

Nothing should actually change - if anything, things could be removed.

Building locally means running go build . if you cloned the repository.
Installing the package from source is a go get github.com/jesseduffield/lazydocker.

There's nothing like dep ensure, as modules are "included" in the go command. When running go build or anything else that needs compiling, the packages as defined in the go.mod file will be downloaded (and the checksum of the downloaded files will be compared to the checksums defined in go.sum).

Copy link
Collaborator

@dawidd6 dawidd6 left a comment

Choose a reason for hiding this comment

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

Builds and tests fine (with or without vendor/).

I think it's ready to merge!

@jesseduffield
Copy link
Owner Author

I want to ensure that I come out of this migration without any major change to my workflow. Here's my main requirements:

  1. I need to be able to continue using the vendor directory. The main reason is that I want to be able to make some changes across various packages inside the vendor directory, and then be able to stash my changes if I need to switch to another branch to do something else. I've been advised that this is possible, though tenuous because the vendor directory might be deprecated down the line
  2. I need to be able to easily bump a dependency. E.g. quite often I'm bumping my gocui fork so I often make changes to it, push to its master branch, then do dep ensure -update github.com/jesseduffield/gocui. Given how often this happens I ideally want this to be a single command
  3. I need to be able to quickly convert a non-forked package in the vendor directory to my own fork, which I often need to do once I discover I need to make a change to one of my dependencies.

Is anybody able to give me a run-down on how I could achieve all of these things using go modules?

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 3, 2019

  1. You can easily make changes to vendored dependencies and then check if everything builds fine using for example: go build -mod=vendor. Finally copying your changes to proper repo of your fork (like you said you are doing right now).
  2. go get github.com/jesseduffield/gocui@master IIRC (or even without @master) should update to latest master (after @ you can specify branch, tag or commit).
  3. Hmm, the workflow for this shouldn't change I guess, but would need to know how do you do that with details.

@tommyknows
Copy link

tommyknows commented Jul 3, 2019

Okay so @dawidd6 was faster than me :)
Regarding 2.:
just executing go get github.com/jesseduffield/gocui will fetch the latest release with a semver tag (if one exists), or master. If you specify @master, it will always get the latest commit on the master branch, ignoring SemVer tags.

regarding 3.:
you can also use replace directives to test it.

Basically, at the end of your go.mod file, add
replace github.com/x/y => github.com/jesseduffield/y v0.0.0
where v0.0.0 either a semver tag, or branch name or commit is 🙂

This means it will replace all imports of github.com/x/y with github.com/jesseduffield/y at compile time.
-> you don't need to rewrite the imports in your .go files to start with.

See here for further info.
It's also a really good read to get into more or less everything you need to know regarding modules 🙂

@jesseduffield
Copy link
Owner Author

I intend on reading that link in the near future but alas I'm having both a busy work week and a busy open source week haha.

I've just tried testing this out locally. I've pushed a commit to the master branch of my gocui fork containing a random comment in one of the files, then in lazydocker I've done go get github.com/jesseduffield/gocui@master but that didn't modify my vendor directory. I then realised I had forgotten to source ~/.zshrc which contained my newly added export GOFLAGS=-mod=vendor. So I did that, but now I'm getting:

▶ go get github.com/jesseduffield/gocui@master
go get: disabled by -mod=vendor

Have I missed a step?

@jesseduffield
Copy link
Owner Author

I ran go mod vendor afterwards and that seemed to work, but it also added/modified a heap of other stuff. I wonder what prompted that? Is this perhaps down to the default preference of go mod for semver tags?

@tommyknows
Copy link

Actually I don't know how go mod behaves with vendoring.
I would have modified go.mod and replaced the version with latest, and then run go build -mod vendor, but I'm really just guessing now too 🙂

Regarding the "added /modified a heap", it's hard to say. The diff is too big, but it looks like it updated the vendor-directory?

TBH, in the long term, you should try to remove that vendor dir. If you wanna "try out something quickly", you can clone it to some directory and use the replace directive to point to the local path.
But that's for you to decide once you know your way around modules :)

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 3, 2019

Looks like go get doesn't want to respect -mod=vendor.

I found some nice guidelines on how to update dependencies in vendor/ directory:
https://www.kablamo.com.au/blog/2018/12/10/just-tell-me-how-to-use-go-modules

So to update one dependency (like you did):

go get -u <repo url>
go mod vendor

@jesseduffield
Copy link
Owner Author

update: without setting GOFLAGS=-mod-vendor, I can do:

go get -u github.com/jesseduffield/gocui@master
go mod vendor

And that pulls in my changes, except that now if I do go run main.go it doesn't use the vendor directory, so I need to use go run -mod=vendor main.go, likewise I would now need to use go build -mod=vendor.

@dawidd6 I came across that link too but it doesn't really change the overhead I'm gonna be dealing with now.

@tommyknows the replace directive sounds pretty cool, I'll need to look into it.

I do wanna say for the record that I kinda feel like go modules is going to add more cognitive overhead, not less. The fact that most of that overhead is due to using a vendor directory doesn't really change the fact that I didn't really have any issues with the former solution and it looks like the new solution is going to be a bit of a headache, given I can't just set that GOFLAGS=-mod-vendor and have everything work as usual.

So just to confirm, is the main reason for doing this that people want to store their code outside the go directory? If so, I can understand the desire to switch, but I just have a very bad feeling that this will make development harder than it needs to be.

@tommyknows
Copy link

The main reason is that it is the "standard" versioning tool now in Go - and it should (hopefully) be the last one.

I don't think that modules are more complicated than the solutions that existed before.
I agree, it does infer some overhead when having a vendor-directory. But with modules, there should be no need for a vendor-directory anymore.

I'm interested to know why, apart from vendoring, you think it is more overhead than before?

@jesseduffield
Copy link
Owner Author

jesseduffield commented Jul 3, 2019

just had a look and it appears that in go 1.14 we're gonna be seeing better support for vendor directories: golang/go#30240. So hopefully once that's out this will be much easier, and I can just set an environment variable and forget about it.

@tommyknows I sympathise with the appeal of a standard versioning tool but it seems to still have some maturing to do.

I also disagree about not needing a vendor directory. Off the top of my head there are three good reasons to use it:

  1. Single source of truth. You can see exactly what changes will be made to the repo in a PR because any changes to the vendor directory are clearly visible. Likewise, you can pull down the PR, and then while disconnected from the internet, run the code fine
  2. Easy searching. I search my vendor directory all the time because it's sitting there in my editor and I often need to know how things are being processed downstream. If I wanted to ditch the vendor directory I'd need to include my whole go directory to be able to search what I wanted and even then it wouldn't be clear what was actually vendored and what wasn't.
  3. rapid development that allows for context-switching. I can make a heap of experimental changes in my vendor directory, then git stash them, and switch to another branch to do something else.

As for the overhead, my previous workflow was:

<write code>
go run main.go
<write code>
go run main.go
<make change in gocui fork and push to master>
dep ensure -update github.com/jesseduffield/gocui
<commit changes>

Now my workflow will be to set export GOFLAGS=-mod=vendor in ~/.zshrc and then

<write code>
go run main.go
<write code>
go run main.go
<make change in gocui fork and push to master>
GOFLAGS= go get -u github.com/jesseduffield/gocui@master
go mod vendor
<commit changes>

It's not substantially different but for somebody who's tools all revolve around reducing keypress-counts, this does bother me. I'll also need to ensure that goreleaser and ci are properly making use of the vendor directory (which is a set-and-forget thing but even so). And I'll need to communicate to users that go run -mod=vendor main.go needs to be used etc.

I'm not trying to weasle my way out of using go mods: as I said I'm happy to do it for the sake of satisfying the contributors who want it, I'm just flagging that this will indeed incur a cognitive load larger than the previous one.

EDIT: turns out you can use GOFLAGS= at the beginning of go get to make it work. That's if you want to set GOFLAGS=-mod=vendor in your rc file, which you likely won't. If you opt against that you'll need to use go run -mod=vendor main.go

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 3, 2019

Seems like the main pain point now is that go get refuses to work when -mod=vendor is set.

I see your point and tbh I'm mad at go right now, cause looking at it's source, one can see this:
https://github.com/golang/go/blob/master/src/cmd/go/internal/modget/get.go#L245-L270

It's really strange that -mod=readonly is simply ignored, but with -mod=vendor it refuses to work.

So @jesseduffield I think that the best solution to this for you, would be exporting GOFLAGS=-mod=vendor + making some sort of shell alias specifically for go get with -mod=readonly for now.

@jesseduffield
Copy link
Owner Author

@dawidd6 agreed. At least I know what commands I need to run to satisfy my use cases. I'm going to update the contributing.md and then we'll merge this PR :)

@jesseduffield
Copy link
Owner Author

I've updated the cache key in circlci's config.yml to use go.sum rather than Gopkg.lock. Can anybody think of other places we might need changes on CI? Might we need to add a -mod=vendor somewhere in the commands?

Also I've noticed that go mod added a heap of random files, like licenses etc. I wonder if they will change the size of the binary much

@jesseduffield
Copy link
Owner Author

Seems that it's a slightly smaller binary with go mod, so that's a plus

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 3, 2019

If we want to make use of vendor/ while building on CI, then I think we have to supply an option to gox:
mitchellh/gox#128

The same option as with go if I see correctly.

@jesseduffield
Copy link
Owner Author

done :)

@jesseduffield
Copy link
Owner Author

We're passing CI! Any last words before I press the button?

@dawidd6
Copy link
Collaborator

dawidd6 commented Jul 3, 2019

giphy (37)

Good job!

@jesseduffield jesseduffield merged commit 572ff63 into master Jul 3, 2019
@jesseduffield jesseduffield deleted the go-modules branch July 7, 2019 11:28
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.

5 participants