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.

Sign up for GitHub

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

[ENGINE-978] Change revision scheme per ENGINE-970 requirements #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+12 −2
Open

[ENGINE-978] Change revision scheme per ENGINE-970 requirements #5

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bf7237
[ENGINE-978] Change revision scheme per ENGINE-970 requirements
psaintlaurent Aug 27, 2024
f572f87
[ENGINE-978] Change revision scheme per ENGINE-970 requirements
psaintlaurent Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions hack/build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,16 @@ set -e
: "${PACKAGE=github.com/docker/buildx}"
: "${VERSION=$(./hack/git-meta version)}"
: "${REVISION=$(./hack/git-meta revision)}"
: "${PRERELEASE=$(./hack/git-meta version | rev | cut -c3- | rev | cut -d'-' -f 2 | grep -E '(^tp|^rc).*')}"
: "${REVISION_NUMBER=1}"
VERSION=$(printf "v%sm%s%s" "$(./hack/git-meta version | rev | cut -c3- | rev | cut -d'-' -f 1 | cut -c2-)" "${REVISION_NUMBER}" "-${PRERELEASE}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain what this is doing and why it is necessary? Why does the m-version need to be sourced from an environment variable? Why can't we just include the m-version part in our git tags?

Copy link
Author

@psaintlaurent psaintlaurent Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

By m-version, I assume you mean the REVISION_NUMBER. I was under the impression that was going to be included from the pipeline going forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The package revision is going to be provided from the packaging pipeline going forward. I don't think it makes sense for the packaging pipeline to pass the software version into the build script piecemeal. @aepifanov's release process proposal -- which you reviewed with the rest of the team -- is for the packaging pipeline to take a Git tag name as a parameter and use the tag name as the version of the software being built. Since the tag is going to encode the full software version; revision, prerelease and all; there would be little value in passing the version components separately. The pipeline would have to parse the version string, only to have the build script re-concatenate it into an identical string.

Note that we will be creating multiple tags which reference the same commit, e.g. to rebuild an RC as a GA. That means we can't rely on the hack/git-meta version command. It looks for which tags reference the commit, but if more than one tag references the commit it might choose the wrong tag. This hack/build script already affords overriding the automatically inferred software version by setting the VERSION environment variable. Why not simply leverage that, and have our packaging script set VERSION to the git tag name when invoking the build?

Copy link
Author

Choose a reason for hiding this comment

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

The proposal doesn't have much detail regarding buildx.


: "${CGO_ENABLED=0}"
: "${GO_PKG=github.com/docker/buildx}"
: "${GO_EXTRA_FLAGS=}"
: "${GO_LDFLAGS=-X ${GO_PKG}/version.Version=${VERSION} -X ${GO_PKG}/version.Revision=${REVISION} -X ${GO_PKG}/version.Package=${PACKAGE}}"
: "${GO_EXTRA_LDFLAGS=}"


set -x
CGO_ENABLED=$CGO_ENABLED go build -mod vendor -trimpath ${GO_EXTRA_FLAGS} -ldflags "${GO_LDFLAGS} ${GO_EXTRA_LDFLAGS}" -o "${DESTDIR}/docker-buildx" ./cmd/buildx
5 changes: 3 additions & 2 deletions tests/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package tests

import (
"regexp"
"strings"
"testing"

"github.com/moby/buildkit/util/testutil/integration"
"github.com/stretchr/testify/require"
"golang.org/x/mod/module"
"golang.org/x/mod/semver"
)

var versionTests = []func(t *testing.T, sb integration.Sandbox){
Expand Down Expand Up @@ -45,7 +45,8 @@ func testVersion(t *testing.T, sb integration.Sandbox) {
// This defaults to something that's still compatible
// with semver.
version := fields[1]
require.True(t, semver.IsValid(version), "Second field was not valid semver: %+v", version)
regex, err := regexp.CompilePOSIX("^v[0-9]+.[0-9]+.[0-9]+(m[0-9]+|m[0-9]+-rc[0-9]+|m[0-9]+-tp[0-9]+)$")
require.True(t, err == nil && regex.MatchString(version), "Second field was not valid: %+v", version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
regex, err := regexp.CompilePOSIX("^v[0-9]+.[0-9]+.[0-9]+(m[0-9]+|m[0-9]+-rc[0-9]+|m[0-9]+-tp[0-9]+)$")
require.True(t, err == nil && regex.MatchString(version), "Second field was not valid: %+v", version)
regex := regexp.MustCompilePOSIX("^v[0-9]+.[0-9]+.[0-9]+(m[0-9]+|m[0-9]+-rc[0-9]+|m[0-9]+-tp[0-9]+)$")
require.True(t, regex.MatchString(version), "Second field was not valid: %+v", version)


// Revision should be empty or should look like a git hash.
if len(fields) > 2 && len(fields[2]) > 0 {
Expand Down
Loading