Skip to content

Migrate to gomods #2504

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

Merged
merged 4 commits into from
Mar 30, 2019
Merged

Migrate to gomods #2504

merged 4 commits into from
Mar 30, 2019

Conversation

stp-ip
Copy link

@stp-ip stp-ip commented Mar 5, 2019

No description provided.

@francislavoie
Copy link
Member

Just to clarify, how does it differ from #2294?

@stp-ip
Copy link
Author

stp-ip commented Mar 5, 2019

Currently checking the effect and where pain points would lay. We are migrating a plugin to gomod and recently migrated CoreDNS, which is based on Caddy to gomods. But also happy to close this as Matt seems to still investigate things.

@mholt
Copy link
Member

mholt commented Mar 5, 2019

Moving to modules is one of the last things we need to do before a 1.0. Feel free to experiment here -- although there is already another PR open to do this.

Updating our build server to support modules is another major task I need to do before we can make the move to modules.

@stp-ip
Copy link
Author

stp-ip commented Mar 5, 2019

Move to go mod + vendor and CI working (appveyor just doesn't use g1.12 yet even so it's defined in the stack 🤔 )

Building Caddy with the vendored mods:

GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build --mod=vendor -o caddy

Building plugins is currently being tested, but should be as simple as:

cd $plugin/caddy
GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build -o caddy

@mholt
Copy link
Member

mholt commented Mar 5, 2019

Cool! So building plugins is basically unchanged.

(Yeah, AppVeyor is being slow, or something: appveyor/ci#2875 -- you'll unfortunately have to ignore those results for now.)

Is the reason that we still have a vendor folder because some projects don't support modules?

@stp-ip
Copy link
Author

stp-ip commented Mar 5, 2019

The vendor folder is there for vendoring.

The build should also work with removing -mod=vendor, which wouldn't take vendor/ into account.

I kept or readded vendoring due to the usual concerns of third parties going away.

@stp-ip
Copy link
Author

stp-ip commented Mar 5, 2019

Also on the plugins yes. As long as they are mentioned within caddy's run.go file they should be fine.
Related PR from a move from internal plugin to external while supporting go mod from CoreDNS: https://github.com/coredns/coredns/pull/2651/files

@mholt
Copy link
Member

mholt commented Mar 5, 2019

I'm actually really OK with getting rid of the vendor folder entirely, as long as we can pin dependencies with modules instead. (I plan on having a separate origin/repo that consists of basically a GOPATH just for Caddy, so that if a third party dependency disappears, we can restore a fork of it to somewhere and then use that. I don't expect that to happen too often, but this way we'll have a public backup.)

@stp-ip
Copy link
Author

stp-ip commented Mar 5, 2019

Also public backups of modules could be easily produced by setting a module proxy, which stores all used modules. Example: Athens.

Will remove vendor.

Also will write up a way for external plugin authors to use gomods to build their plugin into caddy. Still researching the best way.

@mholt
Copy link
Member

mholt commented Mar 5, 2019

Thanks for your effort on this!

It's kinda weird/tricky too because Caddy is both a command and a library -- AFAIK, libraries aren't supposed to use modules, are they? Or does that rule only apply for vendor folders.

@stp-ip
Copy link
Author

stp-ip commented Mar 5, 2019

Modules should make this a bit easier being a command and a library. For example CoreDNS moved to go modules and has the same problem, but uses caddy as a dependency.

Afaik the vendor folder is only taken into account on the specific project not from dependencies.

@francislavoie
Copy link
Member

Wow. +133 −795,884 🎉 That's a nice number.

@stp-ip
Copy link
Author

stp-ip commented Mar 6, 2019

Example makefile to let a plugin test/build with caddy. This should be seen as a PoC idea not production ready.
Currently this works around cyclic deps in a hacky way by loading in caddy and appending a replace statement to go.mod:

BIN := caddy_custom
MAINTAINER := custom
VERSION := 0.0.0
IMAGE := $(MAINTAINER)/$(BIN):$(VERSION)

BUILD_GOOS := $(if $(GOOS),$(GOOS),linux)
BUILD_GOARCH := $(if $(GOARCH),$(GOARCH),amd64)

CONTAINER ?= $(BIN)

.DEFAULT_GOAL := build

recipe:
	find caddy-copy/caddyhttp/httpserver -name 'plugin.go' -type f -exec sed -i -e "s/gopkg/your-plugin-name/" -- {} +
	find caddy-copy/caddy/caddymain -name 'run.go' -type f -exec sed -i -e "s/\/\/ This is where other plugins get plugged in (imported)/_ \"github.com\/miekg\/caddy-prometheus\"/" -- {} +
	find caddy-copy/caddy/caddymain -name 'run.go' -type f -exec sed -i -e '/_ "github.com\/miekg\/caddy-prometheus"/a _ "github.com\/your\/plugin\/caddy"' -- {} +
	find caddy-copy/caddy/caddymain -name 'run.go' -type f -exec sed -i -e 's/var EnableTelemetry = true/var EnableTelemetry = false/' -- {} +
	cd caddy-copy/caddy && \
	GO111MODULE=on CGO_ENABLED=0 GOARCH=$(BUILD_GOARCH) GOOS=$(BUILD_GOOS) go build -ldflags="-s -w"
	mv caddy-copy/caddy/caddy ./$(BIN)

dependencies:
	if [ -d caddy-copy ]; then cd caddy-copy && git checkout . && git pull && cd ..; else git clone https://github.com/mholt/caddy caddy-copy; fi
	cd caddy-copy && \
	if [ -f go.mod ]; then echo "already initialized go modules"; else go mod init; fi && \
	echo "replace github.com/your/plugin/ => ../" >> go.mod && \
	echo "replace github.com/mholt/caddy => ../caddy-copy" >> go.mod && \
	GO111MODULE=on go get github.com/lucas-clemente/quic-go@master && \
	GO111MODULE=on go get github.com/russross/blackfriday@master && \
	GO111MODULE=on go get github.com/miekg/caddy-prometheus && \
	GO111MODULE=on go get github.com/your/plugin@master && \
	cd ..

build: dependencies recipe

test: dependencies
	GO111MODULE=on go test -v `go list ./... | grep -v caddy-copy`

image-build: docker-build
	docker build -t $(IMAGE) .

docker-run: image-build
	docker run --name $(CONTAINER) $(IMAGE)

docker-test:
	docker run --network=host -v $(shell pwd):/source -v $(GOPATH)/pkg/mod:/go/pkg/mod golang:1.12-alpine /bin/sh -c "cd /source && apk add git gcc musl-dev make && GOROOT=\"/usr/local/go\" make test"

docker-build:
	docker run --network=host -v $(shell pwd):/source -v $(GOPATH)/pkg/mod:/go/pkg/mod golang:1.12-alpine /bin/sh -c "cd /source && apk add git gcc musl-dev make && make build && rm -rf caddy-copy"

version:
	@echo $(VERSION)

It basically pulls in a caddy copy, write the plugin into go.run and plugins.go adds a replace statement to go.mod so that the caddy-copy uses local source code instead of pulling outside versions.
Then it tests/builds this specific caddy version.

Current downside. The plugin is not easily go gettable and needs a makefile. Additionally the version of caddy isn't pinned down (could be done in code by checking out a specific tag so).

So if anyone has a better solution to make this more go module native, I'm all ears.

@mholt
Copy link
Member

mholt commented Mar 7, 2019

Woah, that's complicated. So modules won't "just work" with Caddy plugins? AFAIK there shouldn't be cyclic dependencies, as Go programs won't build if the imports cycle.

@stp-ip
Copy link
Author

stp-ip commented Mar 7, 2019

Most of the complication comes from adding in the plugin and from building within a plugin.
If the plugin is already added and we build from a caddy source it gets simpler. But most of this seems to be similar than before. Or what would be the normal development/build flow be with a plugin?

I can take another stab at simplifying this end of month probably.

@mholt
Copy link
Member

mholt commented Mar 7, 2019

It seems I still have a lot to learn about modules then. Currently, "adding" a plugin simply means adding an import statement in run.go. I am surprised it has to be harder than that...

@stp-ip
Copy link
Author

stp-ip commented Mar 7, 2019

From a plugin perspective:

Non module way (manual):

  • switch repository to $GOPATH/src/github.com/mholt/caddy
  • add plugin import to run.go
  • build go binary within caddy repo
  • use binary

Non module way (automatic):

  • load in copy of cady repository to caddy-copy
  • add plugin import to run.go
  • build go binary within caddy-copy
  • use binary

Module way (manual):

  • switch to caddy repository
  • add plugin import to run.go
  • add replace statement to use local version of plugin instead of downloaded code
  • build go binary within caddy repo
  • use binary

Module way (automatic):

  • load in copy of cady repository to caddy-copy
  • add plugin import to run.go
  • add replace statement to use local version instead of downloaded code
  • build go binary within caddy-copy
  • use binary

So in my mind not that more complicated.
It's easier, when you always view it from a caddy repo. Where it pulls in the plugin code.
That way it would also be easier with modules.

That's also where the cyclic dependency comes from.
Viewing it from the plugin POV:

  • Plugin wants to pull in caddy.
  • Caddy then pulls in the plugin (if it by default knows about it, which might not be the case)
  • Go complains

Maybe I'm missing some essential simplifications and happy to take another look as I said. For now the makefile enables a plugin to not mess with any local caddy environment and run a full build self-contained during CI.

@elcore
Copy link
Collaborator

elcore commented Mar 8, 2019

I don't see an issue here, adding a plugin and compiling works fine for me.

I used the git plugin as a test. Am I missing something?

Edit:

I successfully implemented modules into caddy-git, I just had to pull your branch into the local module storage (as your branch is not in master). Everything was working fine, nevertheless it's kind of hacky. I think this will be resolved after this PR is merged.

Feel free to correct me.

@stp-ip
Copy link
Author

stp-ip commented Mar 24, 2019

@elcore can you elaborate, which specific steps you took and how/if they were different than from before. Just want to make sure I get the context on your lgtm.

@stp-ip
Copy link
Author

stp-ip commented Mar 24, 2019

Also in terms of tooling there is another non makefile way, which is using a tools.go file and go:generate to call the tools. This isn't so much about CI, but about build tools.

More info as it's mostly just to provide more context into go modules:
https://github.com/go-modules-by-example/index/blob/master/010_tools/README.md

@elcore
Copy link
Collaborator

elcore commented Mar 24, 2019

Here is what I wrote to Matt.

You have to run:

GO111MODULE=on go get github.com/lucas-clemente/quic-go@master
GO111MODULE=on go get github.com/russross/blackfriday@master

Because it's looking for the latest released version over the network

  1. Move the Caddy source code to go/pkg/mod/github.com/mholt/[email protected]

Afterwards it's working fine

Correct. You have to do steps 1 and 2 after running go mod init on a plugin, otherwise there will be some dependency issues.
After we have released Caddy as a module this will not be necessary according to my tests, but I am not 100% sure.

@mholt
Copy link
Member

mholt commented Mar 29, 2019

I've been working on this today.

I got the build server modified in my dev machine by adding GO111MODULE=on to go commands, removing -t from go get, and running an extra go get to work around golang/go#27215 for now. Builds are working.

But this is really hard to test fully in a dev environment, from a fork, on a branch.

I think I want to merge this, tag a v1.0.0beta1 release, and deploy the build server and test it out that way. It'll help us identify the bugs before the big 1.0 stable.

@mholt
Copy link
Member

mholt commented Mar 30, 2019

Well we're gonna need to do this sooner or later, might as well do it sooner and figure out exactly what will need to be fixed as soon as possible.

I pushed a change that fixes the merge conflict. Will merge this in a few minutes, and pray I don't regret it. 🙏 😅

@mholt mholt self-assigned this Mar 30, 2019
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

I APPROVE

Thank you for your assistance, everyone! @stp-ip and @elcore, thanks for making this happen.

@mholt mholt merged commit 3841517 into caddyserver:master Mar 30, 2019
@mholt mholt mentioned this pull request Mar 30, 2019
4 tasks
@mholt mholt added this to the 1.0 milestone Mar 30, 2019
@stp-ip stp-ip deleted the gomod branch April 24, 2019 13:34
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.

4 participants