Skip to content

[20.10] use GO_LDFLAGS instead of LDFLAGS to prevent inheriting unrelated options#3514

Merged
thaJeztah merged 1 commit into
docker:20.10from
thaJeztah:20.10_backport_fix_ldflags
Mar 31, 2022
Merged

[20.10] use GO_LDFLAGS instead of LDFLAGS to prevent inheriting unrelated options#3514
thaJeztah merged 1 commit into
docker:20.10from
thaJeztah:20.10_backport_fix_ldflags

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

⚠️ This is a partial / modified cherry-pick of #3513 - the code in the 20.10 branch is different, so I had to do a "find and replace" after resolving conflicts and cherry-picking.

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
(cherry picked from commit 391e6ad)
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)

@thaJeztah
Copy link
Copy Markdown
Member Author

Ah, dang. Forgot about this one; will have to look into vndr to make it not default to use git:// as protocol; I recall I was looking into that, but never finished.

2022/03/31 09:29:01 Download dependencies
2022/03/31 09:29:02 Starting whole vndr cycle because no package specified
fatal: remote error: 
  The unauthenticated git protocol on port 9418 is no longer supported.
Please see https://github.blog/2021-09-01-improving-git-protocol-security-github/ for more information.
fatal: remote error: 
  The unauthenticated git protocol on port 9418 is no longer supported.
Please see https://github.blog/2021-09-01-improving-git-protocol-security-github/ for more information.
fatal: remote error: 
  The unauthenticated git protocol on port 9418 is no longer supported.
Please see https://github.blog/2021-09-01-improving-git-protocol-security-github/ for more information.
fatal: remote error: 
  The unauthenticated git protocol on port 9418 is no longer supported.
Please see https://github.blog/2021-09-01-improving-git-protocol-security-github/ for more information.
fatal: remote error: 
  The unauthenticated git protocol on port 9418 is no longer supported.
Please see https://github.blog/2021-09-01-improving-git-protocol-security-github/ for more information.
fatal: remote error: 
  The unauthenticated git protocol on port 9418 is no longer supported.
Please see https://github.blog/2021-09-01-improving-git-protocol-security-github/ for more information.

@thaJeztah thaJeztah force-pushed the 20.10_backport_fix_ldflags branch 6 times, most recently from f3e5c40 to 294094c Compare March 31, 2022 11:29
…ated options

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>
(cherry picked from commit 391e6ad)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the 20.10_backport_fix_ldflags branch from 294094c to 6f7a931 Compare March 31, 2022 12:12
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #3514 (294094c) into 20.10 (91bab60) will not change coverage.
The diff coverage is n/a.

❗ Current head 294094c differs from pull request most recent head 6f7a931. Consider uploading reports for the commit 6f7a931 to get more accurate results

@@           Coverage Diff           @@
##            20.10    #3514   +/-   ##
=======================================
  Coverage   58.57%   58.57%           
=======================================
  Files         299      299           
  Lines       21457    21457           
=======================================
  Hits        12569    12569           
  Misses       7970     7970           
  Partials      918      918           

@thaJeztah
Copy link
Copy Markdown
Member Author

Rebased, and dropped the second commit, because vndr wasn't the cause after all; see #3516

@thaJeztah thaJeztah marked this pull request as ready for review March 31, 2022 12:13
@thaJeztah
Copy link
Copy Markdown
Member Author

@ndeloof @tonistiigi PTAL

@thaJeztah
Copy link
Copy Markdown
Member Author

This one is not urgent; would love to have others also to have a peek (perhaps @tonistiigi and @tianon); I think this change should be fine, but just in case there could be any reason we must also set LDFLAGS. For example, when searching for LDFLAGS, I also found some occurrences in vendor files, such as

#cgo windows CFLAGS: -DPACKED_STRUCTURES
#cgo linux LDFLAGS: -ldl
#cgo darwin LDFLAGS: -ldl
#cgo openbsd LDFLAGS: -ldl
#cgo freebsd LDFLAGS: -ldl

And (honestly) I don't know how those interact (perhaps not at all)

@thaJeztah
Copy link
Copy Markdown
Member Author

Thanks @tianon !

alright; let's get this one in ❤️

@thaJeztah thaJeztah merged commit 81d9655 into docker:20.10 Mar 31, 2022
@thaJeztah thaJeztah deleted the 20.10_backport_fix_ldflags branch March 31, 2022 19:14
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