Skip to content
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

Makefile changes to allow easy builds with or without vendoring. Also fixes version bug for both cases. #1095

Merged
merged 11 commits into from
Nov 29, 2019

Conversation

pstibrany
Copy link
Member

@pstibrany pstibrany commented Oct 1, 2019

This PR modifies Makefile to handle builds with or without vendoring.

It uses GOMOD env. variable to set vendoring or not. Content of GOMOD, if defined, is passed to -mod parameter of Go tools. If not defined, it defaults to “vendor”.

Since vendoring or not vendoring modifies import path of common.Version (from Prometheus) as visible by linker, versioning information is now stored directly in loki.Version, and passed to common.Version on start.

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM!

@joe-elliott
Copy link
Member

Unfortunately this is only a partial fix. Linting is controlled here:

https://github.com/grafana/loki/blob/master/.golangci.yml#L7

and this bit of black magic was necessary to make go generate play nice:

https://github.com/grafana/loki/blob/master/pkg/promtail/server/ui/doc.go#L10

@pstibrany
Copy link
Member Author

pstibrany commented Oct 2, 2019

Thanks for pointing out more places.

Unfortunately this is only a partial fix. Linting is controlled here:

https://github.com/grafana/loki/blob/master/.golangci.yml#L7

Not sure if I understand it correctly, but I think this doesn't run from Makefile, does it?

and this bit of black magic was necessary to make go generate play nice:

https://github.com/grafana/loki/blob/master/pkg/promtail/server/ui/doc.go#L10

Fixed now. It uses go generate's support for $ENV variables expansion.

See this output with vendoring enabled:

$ VENDOR=true make all
>> writing assets
GOFLAGS="-mod=vendor" GOOS=darwin go generate -x -v ./pkg/promtail/server/ui
pkg/promtail/server/ui/doc.go
go run -tags=dev assets_generate.go -build_flags="-mod=vendor"
writing assets_vfsdata.go

and with vendoring off:

$ VENDOR=false make all
>> writing assets
GOFLAGS="" GOOS=darwin go generate -x -v ./pkg/promtail/server/ui
pkg/promtail/server/ui/doc.go
go run -tags=dev assets_generate.go -build_flags=""
writing assets_vfsdata.go

@pstibrany
Copy link
Member Author

Unfortunately this is only a partial fix. Linting is controlled here:
https://github.com/grafana/loki/blob/master/.golangci.yml#L7

Not sure if I understand it correctly, but I think this doesn't run from Makefile, does it?

Oh, it does. OK. Let's see if we can do anything about it.

@pstibrany
Copy link
Member Author

Sent PR to GolangCI to support passing modules download mode as a command line argument.

golangci/golangci-lint#781

@pstibrany
Copy link
Member Author

pstibrany commented Oct 17, 2019

Required golangci-lint change is now available in 1.21.0. Due to upcoming changes to go modules, I will rework this little bit to allow any GOMOD=<mode> instead of using VENDOR flag only. Empty will disable -mod parameter, non empty value will use that as a value for -mod parameter. Will default to vendor.

Link to Go modules change in Go 1.14: golang/go#33848

@slim-bean
Copy link
Collaborator

@pstibrany can you revisit this since we've updated to go mod and also updated golangci-lint?

@pstibrany
Copy link
Member Author

@pstibrany can you revisit this since we've updated to go mod and also updated golangci-lint?

@slim-bean sure... I have switched to mostly just running go build manually, without Makefile, most of the time, so it hasn't been pressing issue for me, but I can finish it.

@pstibrany
Copy link
Member Author

pstibrany commented Nov 8, 2019

Instead of using VENDOR=true or VENDOR=false, it now uses GOMOD env. variable, which can be set to empty string, "vendor", "readonly" and in Go 1.14 also to "mod". If not defined at all, Makefile will use "vendor".

Non empty values are passed to -mod parameter to Go tools, and --modules-download-mode parameter for golangci-cli linting tool.

Note that empty string, i.e. no -mod parameter means different things in Go 1.13 and Go 1.14. In Go 1.13 it means no vendoring, in 1.14 it will default to vendoring, if vendor dir is present and go.mod specifies go 1.14. But at least with GOMOD support in Makefile, now it's easy to set for the whole build. [Details here: https://github.com/golang/go/issues/33848#issuecomment-537222782]

@daixiang0
Copy link
Contributor

any update?

@pstibrany
Copy link
Member Author

This is ready for another review.

@pstibrany pstibrany changed the title Extracted -mod=vendor into a variable Makefile changes to allow easy builds with or without vendoring. Also fixes version bug for both cases. Nov 16, 2019
This enables "MOD_VENDOR=  make all" use from command line
GOMOD can be set to empty string, "vendor", "readonly" and in
Go 1.14 to "mod".  If not defined, defaults to "vendor".

Non empty values are passed to -mod parameter to Go tools, and
--modules-download-mode parameter for golangci-cli linting tool.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
…o 1.13.4.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

nice work ! lgtm

@cyriltovena cyriltovena merged commit 49d0d0f into grafana:master Nov 29, 2019
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.

5 participants