Delete vendored code and update CI to Go 1.13 #158
Conversation
As we now only support Go 1.12 and Go 1.13, there's no need to version things anymore.
|
@ebiggers can you review this? You should be an Admin now, and I've set it up so PRs now have to have approval. |
Is this really true? I tried building with Go 1.10 and it seems to work. Since Go 1.10 is < 2 years old and is the version in Ubuntu 18.04 LTS (https://packages.ubuntu.com/bionic/golang), maybe we should support it for a while longer? Also, this pull request doesn't update the documented prerequisite in README.md: Also the following text would become obsolete and need to be removed: |
Whoops, meant to say that Google only supports v1.12 and v1.13. Any older versions no longer have upstream support and don't get security fixes, so I don't know if we should encourage their use. However, we're not currently using any features that would prevent Go from working on older version (as you saw), it's just a question of properly setting up dependencies. So here's my idea:
@ebiggers, does that sound reasonable?
The official Golang wiki article on Ubuntu makes it clear that Go should be downloaded from the backport PPAs, via Snap, or by downloading it from https://golang.org/dl/.
Thanks I'll go fix those. |
|
It generally sounds fine, but I don't think we should say the minimum version is X if X is not going to be officially supported. If 1.10 is not properly tested and is destined to break at some point, we should say something like "Requires Go 1.11 or higher. Older versions may work but are not officially supported." |
- Go 1.11 is the minimum supported version - Remove all mentions of vendoring
Agreed, the docs,
Let me know what you think. |
There was a problem hiding this comment.
Looks good now, though could you maybe squash the commits into each other and add a better explanation in the commit message? The first commit message which says "we now only support Go 1.12 and Go 1.13" seems to contradict the following change to make 1.11 the minimum.
.travis.yml
Outdated
| install: skip | ||
| env: GO111MODULE=on |
There was a problem hiding this comment.
I'm also wondering whether everyone using Go 1.11 or 1.12 will have to set GO111MODULE=on? Perhaps the Makefile should do it automatically?
There was a problem hiding this comment.
Something like this?
diff --git a/Makefile b/Makefile
index 79e096e..21aace6 100644
--- a/Makefile
+++ b/Makefile
@@ -86,10 +86,10 @@ default: $(BIN)/$(NAME) $(PAM_MODULE)
all: tools gen default format lint test
$(BIN)/$(NAME): $(GO_FILES) $(C_FILES)
- go build $(GO_FLAGS) -o $@ ./cmd/$(NAME)
+ GO111MODULE=on go build $(GO_FLAGS) -o $@ ./cmd/$(NAME)
$(PAM_MODULE): $(GO_FILES) $(C_FILES)
- go build -buildmode=c-shared $(GO_FLAGS) -o $@ ./$(PAM_NAME)
+ GO111MODULE=on go build -buildmode=c-shared $(GO_FLAGS) -o $@ ./$(PAM_NAME)
rm -f $(BIN)/$(PAM_NAME).h
gen: $(BIN)/protoc $(BIN)/protoc-gen-go $(PROTO_FILES)
@@ -100,9 +100,9 @@ format: $(BIN)/goimports
clang-format -i -style=Google $(C_FILES)
lint: $(BIN)/golint $(BIN)/staticcheck $(BIN)/misspell
- go vet ./...
+ GO111MODULE=on go vet ./...
go list ./... | xargs -L1 golint -set_exit_status
- staticcheck ./...
+ GO111MODULE=on staticcheck ./...
misspell -source=text $(FILES)
clean:
@@ -119,7 +119,7 @@ export TEST_FILESYSTEM_ROOT = $(MOUNT)
endif
test:
- go test -p 1 ./...
+ GO111MODULE=on go test -p 1 ./...
test-setup:
dd if=/dev/zero of=$(IMAGE) bs=1M count=20
@@ -174,17 +174,17 @@ TOOLS := $(addprefix $(BIN)/,protoc golint protoc-gen-go goimports staticcheck g
tools: $(TOOLS)
$(BIN)/golint:
- go build -o $@ golang.org/x/lint/golint
+ GO111MODULE=on go build -o $@ golang.org/x/lint/golint
$(BIN)/protoc-gen-go:
- go build -o $@ github.com/golang/protobuf/protoc-gen-go
+ GO111MODULE=on go build -o $@ github.com/golang/protobuf/protoc-gen-go
$(BIN)/goimports:
- go build -o $@ golang.org/x/tools/cmd/goimports
+ GO111MODULE=on go build -o $@ golang.org/x/tools/cmd/goimports
$(BIN)/staticcheck:
- go build -o $@ honnef.co/go/tools/cmd/staticcheck
+ GO111MODULE=on go build -o $@ honnef.co/go/tools/cmd/staticcheck
$(BIN)/gocovmerge:
- go build -o $@ github.com/wadey/gocovmerge
+ GO111MODULE=on go build -o $@ github.com/wadey/gocovmerge
$(BIN)/misspell:
- go build -o $@ github.com/client9/misspell/cmd/misspell
+ GO111MODULE=on go build -o $@ github.com/client9/misspell/cmd/misspell
# Non-go tools downloaded from appropriate repository
PROTOC_VERSION := 3.6.1
There was a problem hiding this comment.
I agree this is the right idea, but Makefile supports a mechanism to set environment variables for all invocations, so I'll just use that.
I'll also add an explict check in the CI that go get github.com/google/fscrypt/cmd/fscrypt works, as that line is mentioned in the README.
There was a problem hiding this comment.
As for the tool related code, i'll fix that in #161
Absolutely agree, I amended the PR description and will squash the commits (as they are bad commit messages). |
As the Go community transitions to using the modules ecosystem, we want to only support one way of managing dependencies. So this change moves to only using Go modules for dependency management.
This means that our effective minimum Go version increases to Go 1.11. We also update the documentation, Makefile, and CI scripts to account for this.