Go module support #2294

ehazlett · 2018-09-16T03:07:38Z

First of all, sorry for the patch bomb. Here is a cute picture to lighten the mood :)

1. What does this change do, exactly?

Migrates from gvt to Go modules.

2. Please link to the relevant issues.

Build failing when using as a library with Go 1.11 outside $GOPATH (aka 'go modules') #2291
gvt is archived now and recommends to move to Go modules

3. Which documentation changes (if any) need to be made because of this PR?

Updated readme with new build instructions
Updated CONTRIBUTING.md with to reference Go modules

4. Checklist

I have written tests and verified that they fail without my change
No new tests required. There are a couple failing but look unrelated:

ok      github.com/mholt/caddy/caddyhttp/browse 0.017s
--- FAIL: TestVisibleErrorWithPanic (0.00s)
    errors_test.go:157: Expected response body to contain error log line, but it didn't:
        15/Sep/2018:22:54:45 -0400 [PANIC /] src/github.com/mholt/caddy/caddyhttp/errors/errors_test.go:135 - I'm a panic

        goroutine 8 [running]:
        github.com/mholt/caddy/caddyhttp/errors.ErrorHandler.recovery(0xb05280, 0xa87318, 0x0, 0x0, 0xc000116cf0, 0x0, 0x1, 0xb0a2a0, 0xc00013e100, 0xc000106600)
                /home/hatter/go/caddy/src/github.com/mholt/caddy/caddyhttp/errors/errors.go:155 +0x4d4
        panic(0x982420, 0xb01390)
                /usr/local/go/src/runtime/panic.go:513 +0x1b9
        github.com/mholt/caddy/caddyhttp/errors.TestVisibleErrorWithPanic.func1(0xb0a2a0, 0xc00013e100, 0xc000106600, 0xc000055cd8, 0x50, 0xc000055cd0)
                /home/hatter/go/caddy/src/github.com/mholt/caddy/caddyhttp/errors/errors_test.go:135 +0x39
        github.com/mholt/caddy/caddyhttp/httpserver.HandlerFunc.ServeHTTP(0xa87318, 0xb0a2a0, 0xc00013e100, 0xc000106600, 0x0, 0x0, 0xc000116cf0)
                /home/hatter/go/caddy/src/github.com/mholt/caddy/caddyhttp/httpserver/middleware.go:92 +0x44
        github.com/mholt/caddy/caddyhttp/errors.ErrorHandler.ServeHTTP(0xb05280, 0xa87318, 0x0, 0x0, 0xc000116cf0, 0x0, 0x1, 0xb0a2a0, 0xc00013e100, 0xc000106600, ...)
                /home/hatter/go/caddy/src/github.com/mholt/caddy/caddyhttp/errors/errors.go:50 +0x105
        github.com/mholt/caddy/caddyhttp/errors.TestVisibleErrorWithPanic(0xc000106500)
                /home/hatter/go/caddy/src/github.com/mholt/caddy/caddyhttp/errors/errors_test.go:145 +0x1f9
        testing.tRunner(0xc000106500, 0xa87320)
                /usr/local/go/src/testing/testing.go:827 +0xbf
        created by testing.(*T).Run
                /usr/local/go/src/testing/testing.go:878 +0x353
2018/09/15 22:54:45 [WARNING] Unable to open error page '404.html': open 404.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '500.html': open 500.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '404.html': open 404.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '503.html': open 503.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page 'generic_error.html': open generic_error.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '404.html': open 404.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '503.html': open 503.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '/home/hatter/go/caddy/src/github.com/mholt/caddy/caddyhttp/errors/404.html': open /home/hatter/go/caddy/src/github.com/mholt/caddy/caddyhttp/errors/404.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '503.html': open 503.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page '503.html': open 503.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page 'generic_error.html': open generic_error.html: no such file or directory
2018/09/15 22:54:45 [WARNING] Unable to open error page 'generic_error.html': open generic_error.html: no such file or directory

I have squashed any insignificant commits
I will squash commits once reviewed. Due to the large patch I wanted to keep the code changes separate from the vendor update.
This change has comments for package types, values, functions, and non-obvious lines of code
I am willing to help maintain this change if there are issues with it later

Steps (using Go 1.11):

cloned repo
ran go get to get the specific versions for xenolf/lego and russross/blackfriday
updated import paths to use latest xenolf/lego "acmev2" branch
ran go mod vendor to vendor the deps using go mod
updated docs

I've tested on two separate machines and go run build.go works on both with a fresh clone to a non GOPATH dir (if you are in GOPATH you must set export GO111MODULE=on). That being said, if you don't want this huge change feel free to close :) I won't be offended ❤️

CLAassistant · 2018-09-16T03:07:45Z

All committers have signed the CLA.

francislavoie · 2018-09-16T04:33:05Z

I know @elcore was working on this in parallel, so tagging him on this.

elcore · 2018-09-16T08:22:44Z

There are a lot of unnecessary imports (go mod tidy), and there has to be some changes to our Travis configuration. Furthermore, updating Lego should be a separate PR.

francislavoie · 2018-09-19T16:04:04Z

@mholt I think you'd prefer to un-vendor, right? I don't think it's necessary to keep things vendored anymore now that there's a proper lock file with go mod.

@ehazlett interested in fixing up those things @elcore mentioned?

ehazlett · 2018-09-20T02:07:09Z

@francislavoie it's still recommended to keep a vendored copy for now

I'll work on tidying and checking the travis config. thanks!

elcore · 2018-09-20T07:12:03Z

.travis.yml

@@ -5,9 +5,12 @@ addons:
    - quic.clemente.io

 go:
-  - 1.x
+  - 1.11.x


Don't change this

elcore · 2018-09-20T07:13:14Z

.travis.yml


 script:
  - gometalinter --install
  - gometalinter --disable-all -E vet -E gofmt -E misspell -E ineffassign -E goimports -E deadcode --tests --vendor ./...
-  - vendorcheck ./...
-  - go test -race ./...
+  - go test -mod=vendor -race ./...



Add go test -mod=readonly -race ./... before go test -mod=vendor -race ./...

elcore · 2018-09-20T07:14:56Z

.travis.yml

-  - go get -t ./...
-  - go get github.com/golang/lint/golint
-  - go get github.com/FiloSottile/vendorcheck
-  - go get github.com/alecthomas/gometalinter


Keep:

go get github.com/golang/lint/golint go get github.com/alecthomas/gometalinter

elcore · 2018-09-20T07:23:14Z

Please reinitialize vendor, remove the whole folder and run go mod vendor to create a new one.

elcore

Now we need to fix Travis

elcore · 2018-09-20T16:23:52Z

Maybe we should add

- go get github.com/golang/lint/golint
- go get github.com/alecthomas/gometalinter
- gometalinter --install

to before_install with GO111MODULE=off

elcore · 2018-09-20T21:27:00Z

I have done some research, it looks like gometalinter is not compatible with modules. (alecthomas/gometalinter#521), we could replace it with https://github.com/golangci/golangci-lint (seems to be compatible with modules)

@mholt What do you think?

mholt · 2018-09-21T23:30:00Z

@elcore Yes, let's try switching to golangci-lint and see how it goes. :)

Lego is also making the switch: go-acme/lego#644

elcore · 2018-09-24T20:48:47Z

.travis.yml

+      env: GO111MODULE=auto
+    - language: go
+      go: tip
+      env: GO111MODULE=auto


We just need 1.x and tip with GO111MODULE=on

I'm not so sure. I believe travis will clone this to an old GOPATH style directory in which case it will use the old way of vendor (https://github.com/golang/go/wiki/Modules#when-do-i-get-old-behavior-vs-new-module-based-behavior).

elcore · 2018-09-24T20:50:07Z

.travis.yml


 before_install:
  # Decrypts a script that installs an authenticated cookie
  # for git to use when cloning from googlesource.com.
  # Bypasses "bandwidth limit exceeded" errors.
  # See github.com/golang/go/issues/12933
- - if [ "$TRAVIS_PULL_REQUEST" = "false" ]; then openssl aes-256-cbc -K $encrypted_3df18f9af81d_key -iv $encrypted_3df18f9af81d_iv -in dist/gitcookie.sh.enc -out dist/gitcookie.sh -d; fi
+  - if [ "$TRAVIS_PULL_REQUEST" = "false" ]; then openssl aes-256-cbc -K $encrypted_3df18f9af81d_key -iv $encrypted_3df18f9af81d_iv -in dist/gitcookie.sh.enc -out dist/gitcookie.sh -d; fi
+  - go get -u github.com/golangci/golangci-lint/cmd/golangci-lint


https://github.com/golangci/golangci-lint/blob/master/README.md#ci-installation

That's how I had it originally but the everything else was go get so I switched it. I'll update it to that. Thx.

elcore · 2018-09-25T15:15:15Z

.travis.yml

-after_script:
-  - golint ./...
+  - golangci-lint run --deadline=10m --disable-all -E golint -E gofmt -E misspell -E ineffassign -E goimports -E deadcode ./...
+  - go test $TEST_ARGS -race ./...


There should be two tests, one with -mod=readonly and one with -mod=vendor. Currently, it's only running once (probably with -mod=vendor, you don't know which one is running)

elcore · 2018-09-25T15:18:53Z

.travis.yml

-
-after_script:
-  - golint ./...
+  - golangci-lint run --deadline=10m --disable-all -E golint -E gofmt -E misspell -E ineffassign -E goimports -E deadcode ./...


10m sounds very long...

What do you think?

bubbajr

yep

francislavoie · 2018-10-30T00:57:20Z

Just wondering, what's the status on this @ehazlett?

ehazlett · 2018-11-01T03:26:28Z

@francislavoie sorry for the delay. i'll try to get it rebased and travis working.

segfault16 · 2018-12-03T11:53:20Z

Any updates on this?

Signed-off-by: Evan Hazlett <[email protected]>

mholt · 2018-12-03T17:06:10Z

This is the direction I want to take our dependency management, but I need to see how this will affect the build server and everyone's third-party plugins who may or may not use modules. Basically I'll have to take some time and do some local testing.

I haven't looked in depth yet; but why does the change lose about 70K lines of code? Are the dependencies still vendored so that we don't have to depend on third-party repos?

ehazlett · 2018-12-03T17:27:45Z

In my experience, go mod vendor doesn't capture everything (i.e. GRPC proto tooling - I've had to add the deps in Go files to make sure they are vendored - https://github.com/ehazlett/stellar/blob/master/proto.go#L6) so my guess is that's it. There is little code change beyond that. I'm happy to close if this bomb it too big and another approach is wanted.

elcore · 2018-12-03T18:48:34Z

In my experience, go mod vendor doesn't capture everything

go mod vendor captures everything that is required to satisfy a build (if you have run go mod tidy beforehand to "reflect the dependency requirements for all possible combinations of OS, architecture, and build tags" [https://github.com/golang/go/wiki/Modules])

ehazlett · 2018-12-03T19:41:03Z

go mod vendor captures everything that is required to satisfy a build

correct, i didn't explain the best :). Go mod vendor will only capture that as far as I can tell. In my example, we had vendored in those files as they were needed to generate from the protos however, they are not required to build specifically so Go modules removed it. It looks like the same is happening here (as well as cleaning up some duplicates, i.e. vendor/github.com/lucas-clemente/quic-go/vendor/github.com/bifurcation/mint/syntax/tags.go).

@mholt to answer your question, a quick run through of the -191k looks like almost all vendor related. I definitely don't want to cause any issues with third party plugins, etc. To save the headache and time from the maintainers I'm going to close this for now until Go modules come out of experimental. Thanks for the reviews and sorry it was a time sink.

mholt · 2019-03-30T02:37:09Z

Thank you for all the work done on this. Thanks to the help in this thread and in #2504, we have completed the move to Go modules.

Not 100% sure it's correct, but we'll need to iron that out next.

elcore suggested changes Sep 20, 2018

View reviewed changes

elcore added the discussion 💬 The right solution needs to be found label Sep 20, 2018

ehazlett force-pushed the go-mod branch 2 times, most recently from 771a80a to d5661ca Compare September 24, 2018 20:07

elcore suggested changes Sep 24, 2018

View reviewed changes

ehazlett force-pushed the go-mod branch from 29b731f to a45a49b Compare September 24, 2018 21:01

elcore suggested changes Sep 25, 2018

View reviewed changes

bubbajr reviewed Oct 2, 2018

View reviewed changes

ehazlett added 9 commits December 3, 2018 10:08

go mod support

e17fff4

Signed-off-by: Evan Hazlett <[email protected]>

migrate vendor to new go mod vendor

6eb36ad

Signed-off-by: Evan Hazlett <[email protected]>

fixup for acmev2 -> acme

723b8aa

Signed-off-by: Evan Hazlett <[email protected]>

update docs for go module support

0d792c8

Signed-off-by: Evan Hazlett <[email protected]>

go mod tidy

99e6599

Signed-off-by: Evan Hazlett <[email protected]>

update travis ci config

5812aa3

Signed-off-by: Evan Hazlett <[email protected]>

travis updates

16b1728

Signed-off-by: Evan Hazlett <[email protected]>

fixup vendor

d3e9b5f

Signed-off-by: Evan Hazlett <[email protected]>

re-init vendor

9519da7

Signed-off-by: Evan Hazlett <[email protected]>

ehazlett added 9 commits December 3, 2018 10:17

switch travis ci to use golangci-lint

8fab15c

Signed-off-by: Evan Hazlett <[email protected]>

lint fixes

22884b7

Signed-off-by: Evan Hazlett <[email protected]>

add build matrix to travis ci

cef876a

Signed-off-by: Evan Hazlett <[email protected]>

travis fixes

210df17

Signed-off-by: Evan Hazlett <[email protected]>

test args for travis

682baba

Signed-off-by: Evan Hazlett <[email protected]>

test args for travis

cd82211

Signed-off-by: Evan Hazlett <[email protected]>

test args for travis

7d546e1

Signed-off-by: Evan Hazlett <[email protected]>

reduce timeout for ci

fad4be5

Signed-off-by: Evan Hazlett <[email protected]>

fix ci script

c83ac34

Signed-off-by: Evan Hazlett <[email protected]>

ehazlett force-pushed the go-mod branch from 84bbddf to c83ac34 Compare December 3, 2018 16:14

bump golangci-lint

9606342

Signed-off-by: Evan Hazlett <[email protected]>

ehazlett closed this Dec 3, 2018

francislavoie mentioned this pull request Mar 5, 2019

Migrate to gomods #2504

Merged

Uh oh!

Go module support #2294

Go module support #2294

Uh oh!

Conversation

ehazlett commented Sep 16, 2018

1. What does this change do, exactly?

2. Please link to the relevant issues.

3. Which documentation changes (if any) need to be made because of this PR?

4. Checklist

Uh oh!

CLAassistant commented Sep 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francislavoie commented Sep 16, 2018

Uh oh!

elcore commented Sep 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francislavoie commented Sep 19, 2018

Uh oh!

ehazlett commented Sep 20, 2018

Uh oh!

elcore Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

elcore Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elcore Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

elcore commented Sep 20, 2018

Uh oh!

elcore left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elcore commented Sep 20, 2018

Uh oh!

elcore commented Sep 20, 2018

Uh oh!

mholt commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elcore Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

ehazlett Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

elcore Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

ehazlett Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

elcore Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elcore Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

bubbajr left a comment

Choose a reason for hiding this comment

Uh oh!

francislavoie commented Oct 30, 2018

Uh oh!

ehazlett commented Nov 1, 2018

Uh oh!

segfault16 commented Dec 3, 2018

Uh oh!

mholt commented Dec 3, 2018

Uh oh!

ehazlett commented Dec 3, 2018

Uh oh!

elcore commented Dec 3, 2018

Uh oh!

CLAassistant commented Sep 16, 2018 •

edited

Loading

elcore commented Sep 16, 2018 •

edited

Loading

elcore Sep 20, 2018 •

edited

Loading

elcore left a comment •

edited

Loading

mholt commented Sep 21, 2018 •

edited

Loading

elcore Sep 25, 2018 •

edited

Loading