Set version and revision at linked time by crazy-max · Pull Request #234 · docker/docker-credential-helpers

crazy-max · 2022-08-21T14:21:24Z

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

codecov-commenter · 2022-08-21T14:22:19Z

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
credentials/credentials.go	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

thaJeztah · 2022-08-21T14:25:08Z

oh, sorry; needs a rebase now 😅

Dockerfile

credentials/credentials.go

thaJeztah · 2022-08-21T14:39:19Z

credentials/version.go

+	// Package is filled at linking time
+	Package = "github.com/docker/docker-credential-helpers"
+
+	// Version holds the complete version number. Filled in at linking time.
+	Version = "v0.0.0+unknown"
+
+	// Revision is filled with the VCS (e.g. git) revision being used to build
+	// the program at linking time.
+	Revision = ""


Are we already at a point where we could use Go's built-in features for this?

If so; can they still be overridden?

Hum good idea we should look at this in a follow-up

Makefile

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

thaJeztah · 2022-08-26T21:16:39Z

Hmm... so for fun, I tried make deb (we need to update that one to something more current, and to not depend on the distro go package); that's something for a follow-up, but I think this part may be related to this PR:

 > [11/11] RUN /build/build-deb v0.6.4-39-geb147ed xenial:
#16 0.312 + set -e
#16 0.312 + version=v0.6.4-39-geb147ed
#16 0.312 + distro=xenial
#16 0.313 ++ awk -F ': ' '$1 == "Maintainer" { print $2; exit }' debian/control
#16 0.315 + maintainer='Docker <support@docker.com>'
#16 0.315 + cat
#16 0.317 ++ date --rfc-2822
#16 0.319 + mkdir -p src/github.com/docker/docker-credential-helpers
#16 0.321 + ln -s /build/credentials /build/src/github.com/docker/docker-credential-helpers/credentials
#16 0.322 + ln -s /build/secretservice /build/src/github.com/docker/docker-credential-helpers/secretservice
#16 0.323 + ln -s /build/pass /build/src/github.com/docker/docker-credential-helpers/pass
#16 0.324 + dpkg-buildpackage -us -uc
#16 0.402 dpkg-buildpackage: warning:     debian/changelog(l1): version 'v0.6.4-39-geb147ed' is invalid: version number does not start with digit
#16 0.402 LINE: docker-credential-helpers (v0.6.4-39-geb147ed) xenial; urgency=low
#16 0.403 dpkg-buildpackage: error: version number does not start with digit
#16 0.404 dpkg-buildpackage: source package docker-credential-helpers
#16 0.404 dpkg-buildpackage: source version v0.6.4-39-geb147ed
------
executor failed running [/bin/sh -c /build/build-deb ${VERSION} ${DISTRO}]: exit code: 255
make: *** [deb] Error 1

crazy-max · 2022-08-26T21:18:19Z

Hmm... so for fun, I tried make deb (we need to update that one to something more current, and to not depend on the distro go package); that's something for a follow-up, but I think this part may be related to this PR:

Yes I have a branch ready to fix this. Pretty much like #209 but specs not inline.

thaJeztah · 2022-08-26T21:25:30Z

I tried changing

docker-credential-helpers/deb/build-deb

Line 5 in 5124911

version=$1

to

version=${1##v}

Which fixes the version issue:

#16 0.748 dpkg-source: info: building docker-credential-helpers in docker-credential-helpers_0.6.4-39-geb147ed.m.tar.gz
#16 0.778 dpkg-source: info: building docker-credential-helpers in docker-credential-helpers_0.6.4-39-geb147ed.m.dsc
#16 0.782  debian/rules build
#16 0.785 dh build

But then breaks because we added -trimpath (and the Go version is too old)

#16 0.932 flag provided but not defined: -trimpath

Yeah, looks like that needs more work (was hoping fixing that version would make it "somewhat" work); let's keep that for later

thaJeztah · 2022-08-26T21:32:17Z

make pass
./bin/build/docker-credential-pass version
docker-credential-pass (github.com/docker/docker-credential-helpers) v0.6.4-39-geb147ed.m

make VERSION=v1.2.3 pass
./bin/build/docker-credential-pass version
docker-credential-pass (github.com/docker/docker-credential-helpers) v1.2.3

Hm... this one doesn't look right;

make VERSION=v1.2.3 PKG=hello-package  pass
./bin/build/docker-credential-pass version
 (github.com/docker/docker-credential-helpers) v0.0.0+unknown

AH!

make VERSION=v1.2.3 PKG=hello pass
go build -trimpath -ldflags="-s -w -X hello/credentials.Version=v1.2.3 -X hello/credentials.Revision=eb147ed80c90ebba74d2e331daa47528e1578d67.m -X hello/credentials.Package=hello -X hello/credentials.Name=docker-credential-pass" -o ./bin/build/docker-credential-pass ./pass/cmd/

Looks like you're using PKG both for actual package, and for .Package

Makefile

crazy-max · 2022-08-26T22:02:11Z

But then breaks because we added -trimpath (and the Go version is too old)
#16 0.932 flag provided but not defined: -trimpath
Yeah, looks like that needs more work (was hoping fixing that version would make it "somewhat" work); let's keep that for later

Yeah deb pkg is broken since Go 1.18.5. I made quick changes to fix that but I have another branch to merge that logic in the main Dockerfile.

Also added a simple gha job to check deb package builds correctly: https://github.com/docker/docker-credential-helpers/runs/8045166392?check_suite_focus=true

thaJeztah · 2022-08-26T22:22:08Z

Makefile

+.PHONY: build-%
+build-%: # build, can be one of build-osxkeychain build-pass build-secretservice build-wincred
+	$(eval BINNAME ?= docker-credential-$*)
+	go build -trimpath -ldflags="$(GO_LDFLAGS) -X ${PKG}/credentials.Package=${PKG}/$* -X ${PKG}/credentials.Name=docker-credential-$*" -o $(DESTDIR)/$(BINNAME) ./$*/cmd/


Did you update, because this looks to still have PKG used both at the start and as value;

Suggested change

go build -trimpath -ldflags="$(GO_LDFLAGS) -X ${PKG}/credentials.Package=${PKG}/$* -X ${PKG}/credentials.Name=docker-credential-$*" -o $(DESTDIR)/$(BINNAME) ./$*/cmd/

go build -trimpath -ldflags="$(GO_LDFLAGS) -X ${PKG}/credentials.Package=${PACKAGE_NAME}/$* -X ${PKG}/credentials.Name=docker-credential-$*" -o $(DESTDIR)/$(BINNAME) ./$*/cmd/

(mostly thinking that Package here could, e.g., be docker-ce or some other name.

Oh I get it now, so for me Package is Go module or base package. Will change that with a new var like you said..

Yes, so wondering if PKG needs to be configurable in this context 🤔 (could just be hard-coded)

Yes hardcoded is fine, changed to GO_PKG

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

thaJeztah

LGTM, thanks!

crazy-max force-pushed the versioning branch from f05ce44 to d210250 Compare August 21, 2022 14:28

crazy-max requested a review from thaJeztah August 21, 2022 14:29

crazy-max marked this pull request as draft August 21, 2022 14:35

thaJeztah reviewed Aug 21, 2022

View reviewed changes

crazy-max force-pushed the versioning branch from 67c7cbd to df73456 Compare August 21, 2022 14:58

crazy-max marked this pull request as ready for review August 21, 2022 15:01

crazy-max requested a review from thaJeztah August 21, 2022 15:07

crazy-max force-pushed the versioning branch from 499c7e6 to 6ee99cc Compare August 24, 2022 00:54

thaJeztah reviewed Aug 25, 2022

View reviewed changes

Makefile Show resolved Hide resolved

crazy-max added 2 commits August 25, 2022 14:50

Set version and revision at linked time

a37d38a

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

ci: fetch depth 0 at build time for proper versioning

2c31fa4

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max force-pushed the versioning branch from 6ee99cc to 84451d5 Compare August 25, 2022 12:52

crazy-max requested a review from thaJeztah August 25, 2022 13:58

crazy-max force-pushed the versioning branch from 84451d5 to eb147ed Compare August 26, 2022 21:03

thaJeztah reviewed Aug 26, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

crazy-max force-pushed the versioning branch from eb147ed to cc43cb3 Compare August 26, 2022 21:57

crazy-max requested a review from thaJeztah August 26, 2022 22:04

thaJeztah reviewed Aug 26, 2022

View reviewed changes

crazy-max force-pushed the versioning branch from cc43cb3 to c55a34b Compare August 26, 2022 22:25

crazy-max added 3 commits August 27, 2022 00:32

Makefile: mutualize local and Dockerfile build opts

e50298d

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

Fix deb package

48ab0d8

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

ci: add build-deb job

a2d8aac

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max force-pushed the versioning branch from c55a34b to a2d8aac Compare August 26, 2022 22:32

crazy-max requested a review from thaJeztah August 26, 2022 22:35

thaJeztah approved these changes Aug 26, 2022

View reviewed changes

thaJeztah merged commit da1d534 into docker:master Aug 26, 2022

crazy-max deleted the versioning branch August 26, 2022 22:39

	go build -trimpath -ldflags="$(GO_LDFLAGS) -X ${PKG}/credentials.Package=${PKG}/$* -X ${PKG}/credentials.Name=docker-credential-$" -o $(DESTDIR)/$(BINNAME) ./$/cmd/
	go build -trimpath -ldflags="$(GO_LDFLAGS) -X ${PKG}/credentials.Package=${PACKAGE_NAME}/$* -X ${PKG}/credentials.Name=docker-credential-$" -o $(DESTDIR)/$(BINNAME) ./$/cmd/

Conversation

crazy-max commented Aug 21, 2022

Uh oh!

codecov-commenter commented Aug 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Aug 21, 2022

Uh oh!

Uh oh!

Uh oh!

thaJeztah Aug 21, 2022

Choose a reason for hiding this comment

Uh oh!

crazy-max Aug 21, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah commented Aug 26, 2022

Uh oh!

crazy-max commented Aug 26, 2022

Uh oh!

thaJeztah commented Aug 26, 2022

Uh oh!

thaJeztah commented Aug 26, 2022

Uh oh!

Uh oh!

crazy-max commented Aug 26, 2022

Uh oh!

thaJeztah Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

crazy-max Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

crazy-max Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Aug 21, 2022 •

edited

Loading