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: replace GOFLAGS with BLDFLAGS #1085

Merged
merged 1 commit into from
Sep 24, 2018
Merged

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Sep 24, 2018

new in go-1.11 GOFLAGS is an environment variable used by go build
with conflicting escaping compared to how we used it in the Makefile

recently discovered this when trying to build a linux package with go-1.11 (it definitely worked with go-1.10)

[pierce@plo-pro nsq]$ go version
go version go1.11 darwin/amd64
[pierce@plo-pro nsq]$ go env | grep GOFLAGS
GOFLAGS=""
[pierce@plo-pro nsq]$ make clean
rm -fr build
[pierce@plo-pro nsq]$ make GOFLAGS='-ldflags="-s -w"' build/nsqd
go build -ldflags="-s -w" -o build/nsqd ./apps/nsqd
go: parsing $GOFLAGS: unknown flag -w"
make: *** [build/nsqd] Error 1
[pierce@plo-pro nsq]$ make GOFLAGS='-ldflags=-s -w' build/nsqd
go build -ldflags=-s -w -o build/nsqd ./apps/nsqd
go: parsing $GOFLAGS: unknown flag -w
make: *** [build/nsqd] Error 1

cc @mreiferson

new in go-1.11 GOFLAGS is an environment variable used by go build
with conflicting escaping compared to how we used it in the Makefile
@mreiferson
Copy link
Member

thanks, is this documented anywhere?

@ploxiln
Copy link
Member Author

ploxiln commented Sep 24, 2018

I don't think we mention this GOFLAGS we had in our Makefile in any nsq docs, and I can't find mention of GOFLAGS in https://golang.org/doc/go1.11 nor in a google search.

@ploxiln
Copy link
Member Author

ploxiln commented Sep 24, 2018

I guess there's this https://golang.org/pkg/cmd/go/internal/base/#GOFLAGS

@ploxiln
Copy link
Member Author

ploxiln commented Sep 24, 2018

[pierce@plo-pro av-infra]$ go version
go version go1.10.3 darwin/amd64
[pierce@plo-pro av-infra]$ go env | grep GOFLAGS
[pierce@plo-pro av-infra]$

@mreiferson
Copy link
Member

I meant the Go docs, np.

I think I follow, we're trying to avoid clobbering a built-in Go environment variable, not that BLDFLAGS has any meaning on its own?

@ploxiln
Copy link
Member Author

ploxiln commented Sep 24, 2018

Correct, I just made up BLDFLAGS to hopefully not conflict with anything in go.

(I was also surprised that make put the variable GOFLAGS in the environment, I thought it would be treated more like a plain shell script variable, I guess I should have know better :P)

@mreiferson
Copy link
Member

Shall we expect an update to your (in)famous blog post? 😬

@mreiferson mreiferson merged commit 0210853 into nsqio:master Sep 24, 2018
@ploxiln ploxiln deleted the nogoflags branch February 2, 2019 06:14
suhailpatel added a commit to monzo/nsq that referenced this pull request Oct 7, 2021
Tweaks the Dockerfile to use Go 1.16 and the latest Dep

Also cherry picks nsqio#1085 as GOFLAGS now
is used as an env var so was conflicting with the flags
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants