Skip to content

Use golang 1.14 in builds #121

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.

Sign up for GitHub

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

Jump to bottom
Merged
timvaillancourt merged 1 commit into from
Jun 18, 2020
Merged

Use golang 1.14 in builds #121

timvaillancourt merged 1 commit into from
Jun 18, 2020

Conversation

@timvaillancourt
Copy link
Contributor

@timvaillancourt timvaillancourt commented Jun 10, 2020 •
edited
Loading

This PR moves the repo to use golang version 1.14.

go mod tidy was ran after the change to 1.14, causing this error:

tim@Tims-MacBook-Pro freno % go build cmd/freno/main.go         
go: inconsistent vendoring in /Users/tim/go/src/github.com/github/freno:
	github.com/armon/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/boltdb/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/bradfitz/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/go-sql-driver/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/hashicorp/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/julienschmidt/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/outbrain/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/patrickmn/[email protected]+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	github.com/rcrowley/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	golang.org/x/[email protected]: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	vitess.io/[email protected]+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

run 'go mod vendor' to sync, or use -mod=mod or -mod=readonly to ignore the vendor directory

So I also ran go mod vendor, which caused the changes to the vendor/ dir

This go mod vendor error is new in 1.14, explained below:

Vendoring

When the main module contains a top-level vendor directory and its go.mod file specifies go 1.14 or higher, the go command now defaults to -mod=vendor for operations that accept that flag. A new value for that flag, -mod=mod, causes the go command to instead load modules from the module cache (as when no vendor directory is present).

When -mod=vendor is set (explicitly or by default), the go command now verifies that the main module's vendor/modules.txt file is consistent with its go.mod file.

@timvaillancourt timvaillancourt requested a review from jfudally June 10, 2020 12:11
davecheney
davecheney approved these changes Jun 15, 2020
View reviewed changes
Copy link

@davecheney davecheney left a comment

Choose a reason for hiding this comment

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

This change looks like it includes a change to the protobuf library. Was this intentional?

@timvaillancourt
Copy link
Contributor Author

timvaillancourt commented Jun 15, 2020

This change looks like it includes a change to the protobuf library. Was this intentional?

@davecheney yes and no, go mod tidy is making that change automatically

@davecheney
Copy link

davecheney commented Jun 16, 2020

What happens if you run go mod tidy without touching the go version? Can you do that as a separate step?

@timvaillancourt
Copy link
Contributor Author

timvaillancourt commented Jun 16, 2020 •
edited
Loading

@davecheney running go mod tidy on master causes no changes but maybe I'm not following

go mod tidy must be ran after swapping to the 1.14 due to the error in my first reply. This error is new in 1.14

EDIT: is there a reason to stay on an older protobuf version? Perhaps we can pin the version if so

@davecheney
Copy link

davecheney commented Jun 17, 2020

Thanks for confirming, the operation of go mod is opaque and confusing.

Re: staying on older version of protobuf; none that I know of other than it will cause problems in the future when you have to update to get a bug fix and the jump is larger.

@timvaillancourt
Copy link
Contributor Author

timvaillancourt commented Jun 18, 2020 •
edited
Loading

Thanks for confirming, the operation of go mod is opaque and confusing.

Yeah it really is 😄! I'm not sure why it is so insistent on upgrading that package but I haven't seen any issues so far

Re: staying on older version of protobuf; none that I know of other than it will cause problems in the future when you have to update to get a bug fix and the jump is larger.

That's a good point that I'll keep an 👁️ out for

jfudally
jfudally approved these changes Jun 18, 2020
View reviewed changes
Copy link

@jfudally jfudally left a comment

Choose a reason for hiding this comment

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

I don't have any concerns outside of what was already brought up. I think in scenarios like this we sort of have to go for it and see what happens 👍

@timvaillancourt timvaillancourt merged commit 5f8f6db into master Jun 18, 2020
@timvaillancourt timvaillancourt deleted the golang1_14 branch June 18, 2020 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davecheney davecheney davecheney approved these changes

+1 more reviewer

@jfudally jfudally jfudally approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@timvaillancourt @davecheney @jfudally