datawire / build-aux

Common Makefile snippets
1 stars 0 forks source link

build-aux: migrate to go.mod, rewrite go-opensource in Go #9

Closed LukeShu closed 5 years ago

LukeShu commented 5 years ago

This is the upstream PR of https://github.com/datawire/apro/pull/258. I did the work downstream in apro.git, so that I could actually tell if things work.

Description

Rewrite go-opensource in Go (issue datawire/build-aux#7)

In datawire/build-aux#7, I mentioned that I'd prefer to do datawire/build-aux#4 (or rather, just the build-aux.git portion of datawire/build-aux#4) first. The reason for that is that I wanted to be able to use external libraries in the Go implementation of go-mkopensource, as currently all of the Go programs in build-aux have to be single-file programs that only use the standard library.

The Go version still has room for improvement; it's mostly a straight-port of the logic from the Bash version (except that it takes advantage of JSON parsing to call go list only once with -json, instead of calling it multiple times with different args). It should be taught to look directly in the module cache like go mod vendor does, instead of looking in ./vendor/. It should have better license recognition. Its license-file detection should mimic go mod vendor's.

Migrate build-aux.git to use go.mod instead of curl (issue datawire/build-aux#8 within the epic of datawire/build-aux#4)

I turned datawire/build-aux#4 in to an epic, and added datawire/build-aux#8 as the build-aux.git portion of it.

Part of that is migrating the stuff in build-aux to use compiled binaries instead of go run. In order to avoid things getting too messy, I moved executables to be in ./build-aux/bin/ instead of directly in ./build-aux/. That's a breaking change for users of build-aux.

However, I also made sure that every program in ./build-aux/bin/ has a variable that refers to it, so build-aux users can just use the variable, instead of actually caring about the location. That doesn't help with this breakage though, as a few programs, like tap-driver didn't have a variable before now.

Other

Closes #7. Closes #8.

LukeShu commented 5 years ago

There was a merge conflict with go.mod where in teleproxy land we'd added an empty build-aux/go.mod to prevent things from crawling build-aux/gopath/. Now that actual Go packages exist in build-aux, maybe it's necessary to prevent crawling by adding a deeper build-aux/gopath/go.mod?

LukeShu commented 5 years ago

There was a merge conflict with go.mod where in teleproxy land we'd added an empty build-aux/go.mod to prevent things from crawling build-aux/gopath/. Now that actual Go packages exist in build-aux, maybe it's necessary to prevent crawling by adding a deeper build-aux/gopath/go.mod?

Yup, I've just now added that change on top.

LukeShu commented 5 years ago

Re: adding documentation outside of commit messages:

I Looked for all commits that had a >1 line commit message, and tried to put important bits in the appropriate files. A lot of it ended up going in to CHANGELOG.md.

I also scanned through the full git log for things that seemed to warrant a CHANGELOG entry but didn't get a >1 line commit message.

But I probably missed some stuff too

LukeShu commented 5 years ago

Regarding the invasiveness of the breaking changes: Following are the changes made to apro as a user of build-aux. I expect that the type of changes here is representative of the type of changes for other users:

$ git diff master..HEAD -- ':!build-aux/'
diff --git a/Makefile b/Makefile
index 27e317f4..ff502b2b 100644
--- a/Makefile
+++ b/Makefile
@@ -83,8 +83,8 @@ go-build: $(foreach _go.PLATFORM,$(go.PLATFORMS),$(foreach lyft.bin,$(lyft.bins)

 # https://github.com/golangci/golangci-lint/issues/587
 go-lint: _go-lint-lyft
-_go-lint-lyft: build-aux/golangci-lint go-get
-       cd vendor-ratelimit && ../build-aux/golangci-lint run -c ../.golangci.yml ./...
+_go-lint-lyft: $(GOLANGCI_LINT) go-get $(go.lock)
+       cd vendor-ratelimit && $(go.lock)$(GOLANGCI_LINT) run -c ../.golangci.yml ./...
 .PHONY: _go-lint-lyft

 #
@@ -217,7 +217,7 @@ docker/max-load.docker: docker/max-load/kubeapply
 docker/max-load.docker: docker/max-load/kubectl
 docker/max-load.docker: docker/max-load/test.sh
 docker/max-load/kubeapply:
-       curl -o $@ --fail https://s3.amazonaws.com/datawire-static-files/kubeapply/$(KUBEAPPLY_VERSION)/linux/amd64/kubeapply
+       curl -o $@ --fail https://s3.amazonaws.com/datawire-static-files/kubeapply/0.3.11/linux/amd64/kubeapply
        chmod 755 $@

 docker/%/kubectl:
@@ -319,17 +319,19 @@ check-local: ## Check: Run only tests that do not talk to the cluster
 check-local: lint go-build
        $(MAKE) tests/local-all.tap.summary
 .PHONY: check-local
-tests/local-all.tap: build-aux/go-test.tap tests/local.tap
-       @./build-aux/tap-driver cat $^ > $@
+tests/local-all.tap: build-aux/go-test.tap tests/local.tap $(TAP_DRIVER)
+       @$(TAP_DRIVER) cat $(sort $(filter %.tap,$^)) > $@
 tests/local.tap: $(patsubst %.test,%.tap,$(wildcard tests/local/*.test))
 tests/local.tap: $(patsubst %.tap.gen,%.tap,$(wildcard tests/local/*.tap.gen))
-tests/local.tap:
-       @./build-aux/tap-driver cat $^ > $@
+tests/local.tap: $(TAP_DRIVER)
+       @$(TAP_DRIVER) cat $(sort $(filter %.tap,$^)) > $@

 tests/cluster.tap: $(patsubst %.test,%.tap,$(wildcard tests/cluster/*.test))
 tests/cluster.tap: $(patsubst %.tap.gen,%.tap,$(wildcard tests/cluster/*.tap.gen))
-tests/cluster.tap:
-       @./build-aux/tap-driver cat $^ > $@
+tests/cluster.tap: $(TAP_DRIVER)
+       @$(TAP_DRIVER) cat $(sort $(filter %.tap,$^)) > $@
+
+tests/cluster/external.tap: $(GOTEST2TAP)

 tests/cluster/oauth-e2e/node_modules: tests/cluster/oauth-e2e/package.json $(wildcard tests/cluster/oauth-e2e/package-lock.json)
        cd $(@D) && npm install
diff --git a/tests/cluster/external.tap.gen b/tests/cluster/external.tap.gen
index 5f8a1beb..ac27018c 100755
--- a/tests/cluster/external.tap.gen
+++ b/tests/cluster/external.tap.gen
@@ -2,7 +2,7 @@

 # -count=1 to disable caching
 go test -json -tags=test -count=1 ./tests/cluster/external_test.go |
-       GO111MODULE=off go run ./build-aux/gotest2tap.go |
+       ./build-aux/bin/gotest2tap |
        # Do this in Bash instead of 'sed' so that it doesn't buffer stdout.
        while IFS= read -r line; do
                case "$line" in