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

[master] Fix package version generation #701

Merged
merged 3 commits into from
May 25, 2022

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented May 25, 2022

unify code for pseudo-versions (nightly), and fix for macOS

This unifies the logic/code for generating pseudo-versions for
nightly builds;

  • Generate pseudo-version if the source repository has uncommitted changes
  • Fix code to work on macOS
  • Strip "v" prefix if the passed VERSION has one

deb, rpm: generate versions without special numbers for pre-releases

The script had special handling for pre-releases, because at some point we
used -tp ("technical preview") as suffix for pre-releases instead of the
standard -alpha, -beta, -rc.

The problem arised because of that, was that comparing versions wouldn't work,
as these suffixes are compared in alphabetical order (which meant that tp
would come "after" beta and rc). To work around this, some custom code
was added to insert a numeric version before the tp, beta, and rc.

We no longer plan to use -tp for pre-releases, and instead to just use the
common alpha[.number], beta[.number], rc[.number] suffixes.

This patch removes the custom handling for pre-releases, to simplify the version
that's generated.

Before:

./rpm/gen-rpm-ver . 22.06.0-beta.0
22.06.0 1.0.beta.0 3091da7 22.06.0-beta.0

./deb/gen-deb-ver . 22.06.0-beta.0
22.06.0~1.0.beta.0 22.06.0-beta.0

After:

./rpm/gen-rpm-ver . 22.06.0-beta.0
22.06.0~beta.0 1 0b5a1ae 22.06.0-beta.0

./deb/gen-deb-ver . 22.06.0-beta.0
22.06.0~beta.0 22.06.0-beta.0

deb: fix version to allow for distro upgrades

The existing version format for our packages use distro-codename in the version.
Unfortunately, codename cannot be used to compare versions, which means that
when a user upgrades their distro to a new version, the package won't be updated
until a new release happens.

This patch changes the format of the version to include VERSION_ID, which is
numeric, and can be used in version comparison.

While we're making changes, this also adds an extra pkgRevision number in the
version, which can allow us to do a new build/release of a package in between
upstream releases. This version is not yet configurable (which can be changed
in future).

Resulting packages are now formatted as;

  • name of the package (e.g., "docker-ce")
  • version (e.g., "22.10.6~beta.0")
  • "-0" (mostly "best practice", and allows updating for specific situations)
  • distro (e.g., "ubuntu")
  • VERSION_ID (e.g. "22.04" or "11") this must be "sortable" to make sure that
    packages are upgraded when upgrading to a newer distro version ("codename"
    cannot be used for this, as they're not sorted)
  • pkgRevision (usually "0", see above)
  • SUITE ("codename"), e.g. "jammy" or "bullseye". This is mostly for convenience,
    because some places refer to distro versions by codename, others by version.
    we prefix the codename with a tilde (~), which effectively excludes it from
    version comparison.

Note that while the ${EPOCH}${EPOCH_SEP} is part of the version, it is not
included in the package's filename.

Examples:

docker-ce_22.10.6~beta.0-0~debian.11.0~bullseye_amd64.deb
docker-ce_22.10.6~beta.0-0~ubuntu.22.04.0~jammy_amd64.deb

This unifies the logic/code for generating pseudo-versions for
nightly builds;

- Generate pseudo-version if the source repository has uncommitted changes
- Fix code to work on macOS
- Strip "v" prefix if the passed VERSION has one

Signed-off-by: Sebastiaan van Stijn <[email protected]>
The script had special handling for pre-releases, because at some point we
used `-tp` ("technical preview") as suffix for pre-releases instead of the
standard `-alpha`, `-beta`, `-rc`.

The problem arised because of that, was that comparing versions wouldn't work,
as these suffixes are compared in _alphabetical_ order (which meant that `tp`
would come "after" `beta` and `rc`). To work around this, some custom code
was added to insert a numeric version _before_ the `tp`, `beta`, and `rc`.

We no longer plan to use `-tp` for pre-releases, and instead to just use the
common `alpha[.number]`, `beta[.number]`, `rc[.number]` suffixes.

This patch removes the custom handling for pre-releases, to simplify the version
that's generated.

Before:

    ./rpm/gen-rpm-ver . 22.06.0-beta.0
    22.06.0 1.0.beta.0 3091da7 22.06.0-beta.0

    ./deb/gen-deb-ver . 22.06.0-beta.0
    22.06.0~1.0.beta.0 22.06.0-beta.0

After:

    ./rpm/gen-rpm-ver . 22.06.0-beta.0
    22.06.0~beta.0 1 0b5a1ae 22.06.0-beta.0

    ./deb/gen-deb-ver . 22.06.0-beta.0
    22.06.0~beta.0 22.06.0-beta.0

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

Before:

docker-ce_22.06.0-beta.0~3-0~debian-bullseye_amd64.deb

After:

docker-ce_22.06.0~beta.0-0~debian-bullseye_amd64.deb

Looking where the 0~ prefix in 0~debian-bullseye comes from (and if we need that one or if that also can be removed?)

@thaJeztah
Copy link
Member Author

thaJeztah commented May 25, 2022

rpm packages, e.g. Fedora 35 now look like;

docker-ce-22.06.0~beta.0-1.fc35.x86_64.rpm

comparing to an old beta release (which used beta1, without a .) for Fedora 31;

docker-ce-20.10.0-1.1.beta1.fc31.x86_64.rpm   

So the -1 after 20.10.0 is moved to now be after beta.0 - I think that's the expected result, because 22.06.0~beta.0 is considered the Version: (previously, 20.10.0 was seen as version because there's no ~ ?) and the -1 would then match the Release:

@tianon
Copy link
Contributor

tianon commented May 25, 2022

The change from 5:22.06.0-beta.0~3-0~debian-bullseye to 5:22.06.0~beta.0-0~debian-bullseye seems like a positive one to me 👍

I agree we should fix the debian-bullseye bit there -- it's wrong for two different reasons:

  1. there should only be one - in a Debian version (it's legal to have more, but it just makes parsing more confusing unnecessarily - the - should separate "upstream version" from "debian revision")

  2. we should replace this with something that becomes numerically bigger for new releases of Debian so that when folks update from Debian 10 to Debian 11 they get a "Debian 11 package" (in case ABIs change so we continue to have working binaries); I like . /etc/os-release && echo "${ID}${VERSION_ID}" as a one-liner for this in recent Debian and Ubuntu (debian10, debian11, ubuntu20.04, etc) but I think that's more complicated to implement here

@thaJeztah
Copy link
Member Author

Yup! Following our slack chat; the line to look at is this one;

cat > "debian/changelog" <<-EOF
$debSource (${EPOCH}${EPOCH_SEP}${DEB_VERSION}-0~${DISTRO}-${SUITE}) $SUITE; urgency=low

That prepends the 0-, and there we should change the ${DISTRO}-${SUITE} to be something that's "sortable" so that upgrading from (say) Debian 11 to Debian 12 will upgrade the package, even if it's the same version of Docker

seemethere
seemethere previously approved these changes May 25, 2022
@thaJeztah
Copy link
Member Author

Trying some variations to see what the distro version would look like, and what combinations we could make that are both "sorting" correctly, and (somewhat) human friendly;

On Debian:

docker run -it --rm debian sh -c '. /etc/os-release && echo "${ID}${VERSION_ID}"'
debian11
  • could we / should we include the codename there as well?
  • if so; would that potentially cause havoc for "unstable" (would it still carry sid as codename instead of bookworm?)

On Ubuntu, VERSION_ID is CalVer, so I hope that'll sort OK; we could strip . from that version (20.04 -> 2004) though

docker run -it --rm ubuntu sh -c '. /etc/os-release && echo "${ID}${VERSION_ID}"'
ubuntu20.04

edit Tianon confirmed that CalVer is fine for Ubuntu

@thaJeztah
Copy link
Member Author

debian:unstable (AKA to-be woodworm) will indeed be a bit of an issue, unless we override things (or do some magic exploring of apt cache);

cat /etc/os-release
PRETTY_NAME="Debian GNU/Linux bookworm/sid"
NAME="Debian GNU/Linux"
ID=debian
HOME_URL="https://www.debian.org/"
SUPPORT_URL="https://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

@tianon
Copy link
Contributor

tianon commented May 25, 2022

IMO we shouldn't be publishing packages for either unstable or testing until they're actually released (or deep in one of the stages of Debian Freeze, and thus only testing).

@thaJeztah
Copy link
Member Author

Gotcha; yes. It's always a bit of a 'chicken and egg' (people want to test those, e.g. Fedora <next>, but they're not finalised yet, but not having packages yet, also doesn't allow them to test our bits), but it's a bit of a "grey" area (if we do publish packages, it could give the indication "so, everything works, right?"

@cpuguy83
Copy link
Contributor

What I do is for every distro release I support I have multiple release channels.
I haven't done a release for a pre-release distro yet... but I'd imagine I'd throw those packages into a nightly channel just because the distro may break my packages up until release time.
Given that packages are in separate repos anyway, it can easily omit the distro release number until one is available.

2nd-ing what @tianon mentioned above around naming.
<version>-<distro-id><distro-relnum>u<revision> is what I use currently.
e.g. 1.1.2-debian11u1 for my lateste runc packages.
<revision> is where I bump for packaging changes and other such things.

@thaJeztah
Copy link
Member Author

Yes, just discussing with Tianon to insert a .<revision> in there, to allow having a new version of a package (without a new release) if it would ever be needed. Trying it out locally; will push a commit once done 👍 (thanks for looking, Brian!)

@thaJeztah
Copy link
Member Author

As to "pre-releases"; we're only pushing those to the test channel (so far I only pushed to download-stage.docker.com, which nobody should be using, so I can "rinse and repeat" there)

@thaJeztah
Copy link
Member Author

Ah, lol. sourcing /etc/os-release also creates $VERSION, which conflicts with "our" $VERSION

cd /go/src/github.com/docker/cli && VERSION=22.04 (Jammy Jellyfish) GITCOMMIT=1496274 LDFLAGS='' GO_LINKMODE=dynamic ./scripts/build/binary && DISABLE_WARN_OUTSIDE_CONTAINER=1 LDFLAGS='' make manpages
/bin/sh: 1: Syntax error: "(" unexpected

Guess I need a bit of subshell magic here

@tianon
Copy link
Contributor

tianon commented May 25, 2022

DIST_VER="$(. /etc/os-release && echo "${ID}${VERSION_ID}")" 👀

The existing version format for our packages use `distro-codename` in the version.
Unfortunately, `codename` cannot be used to compare versions, which means that
when a user upgrades their distro to a new version, the package won't be updated
until a new release happens.

This patch changes the format of the version to include `VERSION_ID`, which is
numeric, and can be used in version comparison.

While we're making changes, this also adds an extra `pkgRevision` number in the
version, which can allow us to do a new build/release of a package in between
upstream releases. This version is not yet configurable (which can be changed
in future).

Resulting packages are now formatted as;

- name of the package (e.g., "docker-ce")
- version (e.g., "22.10.6~beta.0")
- "-0" (mostly "best practice", and allows updating for specific situations)
- distro (e.g., "ubuntu")
- VERSION_ID (e.g. "22.04" or "11") this must be "sortable" to make sure that
  packages are upgraded when upgrading to a newer distro version ("codename"
  cannot be used for this, as they're not sorted)
- pkgRevision (usually "0", see above)
- SUITE ("codename"), e.g. "jammy" or "bullseye". This is mostly for convenience,
  because some places refer to distro versions by codename, others by version.
  we prefix the codename with a tilde (~), which effectively excludes it from
  version comparison.

Note that while the `${EPOCH}${EPOCH_SEP}` is part of the version, it is not
included in the package's *filename*.

Examples:

    docker-ce_22.10.6~beta.0-0~debian.11.0~bullseye_amd64.deb
    docker-ce_22.10.6~beta.0-0~ubuntu.22.04.0~jammy_amd64.deb

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

DIST_VER="$(. /etc/os-release && echo "${ID}${VERSION_ID}")" 👀

Yup! That's ~ what I did

@thaJeztah
Copy link
Member Author

Alright; pushed a commit with the changes discussed above; PTAL and let me know if that looks sane :)

Copy link
Contributor

@tianon tianon left a comment

Choose a reason for hiding this comment

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

👍

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.

4 participants