Skip to content

use GO_LDFLAGS instead of LDFLAGS to prevent inheriting unrelated options#3513

Merged
ndeloof merged 1 commit into
docker:masterfrom
thaJeztah:fix_ldflags
Mar 31, 2022
Merged

use GO_LDFLAGS instead of LDFLAGS to prevent inheriting unrelated options#3513
ndeloof merged 1 commit into
docker:masterfrom
thaJeztah:fix_ldflags

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

relates to docker/docker-ce-packaging#666

When building on Fedora 36, the build failed. I suspect this is because the
rpm tools also set LDFLAGS, but with options that cannot be used;

GO_LINKMODE=dynamic
+ ./scripts/build/binary
  /go/src/github.com/docker/cli ~/rpmbuild/BUILD/src
  Building dynamic docker-linux-arm64
  + go build -o build/docker-linux-arm64 -tags ' pkcs11' -ldflags '-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -Wl,--build-id=sha1 -Wl,-dT,/root/rpmbuild/BUILD/src/.package_note-docker-ce-cli-0.0.0.20220330082637.68cad50-0.fc36.aarch64.ld -w -X "github.com/docker/cli/cli/version.GitCommit=68cad50" -X "github.com/docker/cli/cli/version.BuildTime=2022-03-30T20:05:36Z" -X "github.com/docker/cli/cli/version.Version=0.0.0-20220330082637-68cad50" -X "github.com/docker/cli/cli/version.PlatformName=Docker Engine - Community"' -buildmode=pie github.com/docker/cli/cmd/docker
# github.com/docker/cli/cmd/docker
flag provided but not defined: -Wl,-z,relro
usage: link [options] main.o

This patch changes the variable we use to GO_LDFLAGS, taking a similar approach
as containerd, and various other projects using this name: https://grep.app/search?q=GO_LDFLAGS

Signed-off-by: Sebastiaan van Stijn github@gone.nl

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

…ions

When building on Fedora 36, the build failed. I suspect this is because the
rpm tools also set LDFLAGS, but with options that cannot be used;

    GO_LINKMODE=dynamic
    + ./scripts/build/binary
      /go/src/github.com/docker/cli ~/rpmbuild/BUILD/src
      Building dynamic docker-linux-arm64
      + go build -o build/docker-linux-arm64 -tags ' pkcs11' -ldflags '-Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -Wl,--build-id=sha1 -Wl,-dT,/root/rpmbuild/BUILD/src/.package_note-docker-ce-cli-0.0.0.20220330082637.68cad50-0.fc36.aarch64.ld -w -X "github.com/docker/cli/cli/version.GitCommit=68cad50" -X "github.com/docker/cli/cli/version.BuildTime=2022-03-30T20:05:36Z" -X "github.com/docker/cli/cli/version.Version=0.0.0-20220330082637-68cad50" -X "github.com/docker/cli/cli/version.PlatformName=Docker Engine - Community"' -buildmode=pie github.com/docker/cli/cmd/docker
    # github.com/docker/cli/cmd/docker
    flag provided but not defined: -Wl,-z,relro
    usage: link [options] main.o

This patch changes the variable we use to `GO_LDFLAGS`, taking a similar approach
as containerd, and various other projects using this name: https://grep.app/search?q=GO_LDFLAGS

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #3513 (391e6ad) into master (68cad50) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3513   +/-   ##
=======================================
  Coverage   59.00%   59.00%           
=======================================
  Files         284      284           
  Lines       23839    23839           
=======================================
  Hits        14066    14066           
  Misses       8914     8914           
  Partials      859      859           

Copy link
Copy Markdown
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Indeed we call ./scripts/build/binary directly on docker-ce-packaging so we are not sandboxed unfortunately.

@thaJeztah
Copy link
Copy Markdown
Member Author

Right, so I think moby/moby resets LDFLAGS at the start (would have to double check), so that would avoid the problem as well, but I was thinking that "have a GO_LDFLAGS allows people to (if needed for anything) set an initial set of flags (before our scripts add the options we set), which could be useful.

@ndeloof ndeloof merged commit abe41ad into docker:master Mar 31, 2022
@thaJeztah thaJeztah deleted the fix_ldflags branch March 31, 2022 10:54
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.

4 participants