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

Address Makefile issues around the cgo build #1446

Merged
merged 9 commits into from
Jul 13, 2023
Merged

Conversation

tashian
Copy link
Contributor

@tashian tashian commented Jun 21, 2023

Ok y’all, I’m pulling something out of the rabbit hole here.

I’ve noticed an issue with the Makefile for step-ca, where we use the variable GOFLAGS.
Actually, this has been confusing to me for a long time and I finally discovered why.
In the Makefile, we run, for example $(GOOS_OVERRIDE) $(GOFLAGS) go build -v -o $(PREFIX)bin/$(BINNAME) $(LDFLAGS) $(PKG)
By default, GOFLAGS is set to the value CGO_ENABLED=0
But if I run make build GOFLAGS="CGO_ENABLED=1", I get:

go: parsing $GOFLAGS: non-flag "CGO_ENABLED=1"
make: *** [Makefile:67: download] Error 1

In this case, GOFLAGS is being used both by the Makefile (as go env variables) and by go itself (as “A space-separated list of -flag=value settings to apply to go commands by default”)

So, then, how does the default case even work, since by default GOFLAGS is equal to CGO_ENABLED=0, a non-flag?
It works because by default GOFLAGS is set locally in the Makefile, and it won’t get exported to go as an env.

The only valid value for GOFLAGS that satisfies both go and the Makefile is “”
And this is the only value we use in automations and docs, because it creates a cgo build.
Or, it’s supposed to.
But sometimes it results in a non-cgo build.

Quick sideshow:
Normally, Go creates cgo binaries by default.
But (I think this is a recent change to Go), if gcc is not in $PATH, it will silently not use cgo.
So, the second issue is that our instructions for cgo builds might silently create a non-cgo build, as a user just encountered.
So we need to tell people to run make with CGO_ENABLED=1 set explicitly.
But there isn’t a way to do that. For some reason, make build CGO_ENABLED=1 does not pass CGO_ENABLED=1 to go.

Bottom line:

  • The way we’re using it, GOFLAGS should be called GO_ENVS
  • We should explicitly set CGO_ENABLED=1 if we want a cgo binary, so that when gcc doesn't exist, go will fail to compile instead of falling back to non-cgo
  • GOFLAGS should be reserved for go

A couple backward-compatible options for getting out of this:

  1. Change GOFLAGS to GO_ENVS to represent go build flags; and if GOFLAGS is passed in as "", silently set GO_ENVS to CGO_ENABLED=1. Otherwise, default it to CGO_ENABLED=0. This is what I did in this PR.
  2. Don't bother with GO_ENVS, just accept CGO_ENABLED=1 into make and export it to go. If GOFLAGS is passed in as "", silently set CGO_ENABLED=1.

Alternatively, we could make a breaking change and just update the docs and cgo-related automations.

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Jun 21, 2023
@tashian tashian requested review from maraino and dopey June 21, 2023 22:03
Copy link
Contributor

@maraino maraino left a comment

Choose a reason for hiding this comment

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

The expansion of the variables, is a little weird GO_ENVS ?= CGO_ENABLED=0 might be better.

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@tashian tashian requested a review from maraino June 22, 2023 22:35
@tashian
Copy link
Contributor Author

tashian commented Jun 22, 2023

@maraino ok I think this is ready.

Tests I've run:

Command Result
make build cgo disabled
make build GOFLAGS="" cgo enabled
make build GOFLAGS="-x" cgo disabled; -x passed to go
make build GO_ENVS="CGO_ENABLED=0" cgo disabled
make build GO_ENVS="CGO_ENABLED=1" cgo enabled
make build GO_ENVS="CGO_ENABLED=0" GOFLAGS="" cgo disabled
make build GO_ENVS="CGO_ENABLED=1" GOFLAGS="-x -trimpath" cgo enabled; -x -trimpath passed to go

But, GO_ENVS only makes sense if we will use it for more than CGO_ENABLED.
Otherwise we should just support:

make build CGO_ENABLED=1

Also: I made a small change to the README that removes a broken link to the docs folder.

@tashian tashian enabled auto-merge June 22, 2023 22:54
Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
tashian and others added 2 commits July 8, 2023 02:49
Co-authored-by: Herman Slatman <hslatman@users.noreply.github.com>
Co-authored-by: Herman Slatman <hslatman@users.noreply.github.com>
@tashian tashian requested review from hslatman and removed request for dopey July 8, 2023 09:50
@tashian tashian merged commit 4059184 into master Jul 13, 2023
13 checks passed
@tashian tashian deleted the carl/check-cgo-deps branch July 13, 2023 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Waiting for discussion / prioritization by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants