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

Update go mod to use go1.21 and update compiler switches/ flags for static linking #384

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vedantjoshi84
Copy link
Contributor

No description provided.

Copy link
Contributor

@cruizen cruizen left a comment

Choose a reason for hiding this comment

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

Hi @vedantjoshi84 , I was talking to @mithilarun that the make build target looks right with the additional ldflags but we somehow still don't have a statically linked executable. He brought to my notice that the Teamcity build calls the script build.bash that calls the target make container-build. This target uses golang:1.18 and more importantly doesn't use the same flags (CGO_ENABLED=0 and the ldflags added recently).

Makefile Outdated
@@ -29,7 +29,7 @@ GOFLAGS ?= ""
default: $(BIN)

container-build:
docker run --rm --env XDG_CACHE_HOME=$(XDG_CACHE_HOME) --env SEGMENT_KEY_PRD_PMKFT=$(SEGMENT_KEY_PRD_PMKFT) --env VERSION_OVERRIDE=${VERSION_OVERRIDE} --env GOPATH=/tmp --env GOFLAGS=$(GOFLAGS) --user $(CONT_USER):$(CONT_GRP) --volume $(PWD):$(PACKAGE_GOPATH) $(GIT_STORAGE_MOUNT) --workdir $(PACKAGE_GOPATH) golang:1.18 make
docker run --rm --env XDG_CACHE_HOME=$(XDG_CACHE_HOME) --env SEGMENT_KEY_PRD_PMKFT=$(SEGMENT_KEY_PRD_PMKFT) --env VERSION_OVERRIDE=${VERSION_OVERRIDE} --env GOPATH=/tmp --env GOFLAGS=$(GOFLAGS) --user $(CONT_USER):$(CONT_GRP) --volume $(PWD):$(PACKAGE_GOPATH) $(GIT_STORAGE_MOUNT) --workdir $(PACKAGE_GOPATH) golang:1.18 make LDFLAGS="-s -w -extldflags '-static'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
docker run --rm --env XDG_CACHE_HOME=$(XDG_CACHE_HOME) --env SEGMENT_KEY_PRD_PMKFT=$(SEGMENT_KEY_PRD_PMKFT) --env VERSION_OVERRIDE=${VERSION_OVERRIDE} --env GOPATH=/tmp --env GOFLAGS=$(GOFLAGS) --user $(CONT_USER):$(CONT_GRP) --volume $(PWD):$(PACKAGE_GOPATH) $(GIT_STORAGE_MOUNT) --workdir $(PACKAGE_GOPATH) golang:1.18 make LDFLAGS="-s -w -extldflags '-static'"
docker run --rm --env XDG_CACHE_HOME=$(XDG_CACHE_HOME) --env SEGMENT_KEY_PRD_PMKFT=$(SEGMENT_KEY_PRD_PMKFT) --env VERSION_OVERRIDE=${VERSION_OVERRIDE} --env GOPATH=/tmp --env GOFLAGS=$(GOFLAGS) --user $(CONT_USER):$(CONT_GRP) --volume $(PWD):$(PACKAGE_GOPATH) $(GIT_STORAGE_MOUNT) --workdir $(PACKAGE_GOPATH) golang:1.18 make ldflags="-s -w -extldflags '-static'"

Copy link
Contributor

Choose a reason for hiding this comment

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

LDFLAGS is lowercase in the build target. Uppercase here.

Copy link
Member

Choose a reason for hiding this comment

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

@vedantjoshi84 I see that the LDFLAGS variable isn't used anymore. Why not make these changes to that variable directly?

Copy link
Contributor

@cruizen cruizen Sep 14, 2024

Choose a reason for hiding this comment

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

You should be able to pass ldflags as part of --env GOFLAGS as follows. Can you verify (the formatting may be incorrect here)?

container-build:
    docker run --rm \
--env XDG_CACHE_HOME=$(XDG_CACHE_HOME) \
--env SEGMENT_KEY_PRD_PMKFT=$(SEGMENT_KEY_PRD_PMKFT) \
--env VERSION_OVERRIDE=${VERSION_OVERRIDE} \
--env GOPATH=/tmp \
--env GOFLAGS="$(GOFLAGS) -ldflags '-s -w -extldflags \"-static\"'" \
--user $(CONT_USER):$(CONT_GRP) \
--volume $(PWD):$(PACKAGE_GOPATH) $(GIT_STORAGE_MOUNT) \
--workdir $(PACKAGE_GOPATH) \
golang:1.21 make

@cruizen cruizen changed the title Update go mod version Update go mod to use go1.21 and update compiler switches/ flags for static linking Aug 30, 2024
@cruizen cruizen self-requested a review September 11, 2024 04:37
Copy link
Contributor

@cruizen cruizen left a comment

Choose a reason for hiding this comment

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

Suggested a change in container-build

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.

3 participants