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: evaluate os/arch based on target name #4236

Merged
merged 3 commits into from May 20, 2020

Conversation

tstromberg
Copy link
Contributor

@tstromberg tstromberg commented May 20, 2020

Previously, the Makefile was setting a correct GOOS and GOARCH environment variables, but for flag values, it was passing in the native Makefile variables, set to the local OS and architecture that was executing the Makefile.

For example, running make out/skaffold-linux-amd64 on a macOS machine output:

GOOS="linux" GOARCH="amd64" \
	docker build \
		--build-arg GOOS=darwin \
		--build-arg GOARCH=amd64 \
		--build-arg TAGS="release" \
		--build-arg LDFLAGS=" -X 

In this PR, I change from setting an environment variable, to setting a local variable ("os" and "arch"), which is evaluated from the Makefile rule. This local variable is then passed in correctly as command-line arguments. The previous example now emits:

docker build \
		--build-arg GOOS=linux \
		--build-arg GOARCH=amd64 \
		--build-arg TAGS="osusergo netgo static_build release" \
		--build-arg LDFLAGS=" -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.version=v1.10.0-4-gfafbfc163-dirty -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.buildDate=2020-05-20T11:28:19Z -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.gitCommit=fafbfc1636db2cfa49bcd0eb1ba0bd02fa921bb3 -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.gitTreeState=dirty -s -w  -extldflags \"-static\"" \
		-f deploy/cross/Dockerfile \
		-t skaffold/cross \
		.

You'll notice that TAGS and LDFLAGS are now properly evaluated as well.

Fixes #4234

@tstromberg tstromberg changed the title WIP: Makefile: evaluate os/arch based on target name Makefile: evaluate os/arch based on target name May 20, 2020
@@ -75,17 +75,21 @@ install: $(BUILD_DIR)/$(PROJECT)
cross: $(foreach platform, $(SUPPORTED_PLATFORMS), $(BUILD_DIR)/$(PROJECT)-$(platform))

$(BUILD_DIR)/$(PROJECT)-%: $(STATIK_FILES) $(GO_FILES) $(BUILD_DIR) deploy/cross/Dockerfile
GOOS="$(firstword $(subst -, ,$*))" GOARCH="$(lastword $(subst -, ,$(subst .exe,,$*)))" \
$(eval os = $(firstword $(subst -, ,$*)))
Copy link
Member

Choose a reason for hiding this comment

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

its is frustrating, we cant use keys values pairs all though its supported on bash4.
https://stackoverflow.com/questions/6047648/bash4-associative-arrays-error-declare-a-invalid-option
It would have been much easier to read and less error prone.

this change looks good to me.

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #4236 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted Files Coverage Δ
pkg/skaffold/util/cmd.go 47.61% <0.00%> (-4.77%) ⬇️
pkg/skaffold/deploy/kustomize.go 74.71% <0.00%> (-1.51%) ⬇️
pkg/skaffold/deploy/kubectl/cli.go 90.90% <0.00%> (-0.40%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 68.29% <0.00%> (+2.43%) ⬆️

@tstromberg tstromberg merged commit cc67c04 into GoogleContainerTools:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v1.10.0 binaries released for macOS & Windows are actually ELF 64-bit binaries (Linux)
4 participants