-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: Set client version for non-release binaries and version in guix based on git tags #5653
Conversation
I don't understand the dev / gitarc postfixes |
Basically, when you clone the repo and build binaries it's Also, if you tag and try to build release binaries but forget to set If you don't like their names, I'm open for suggestions :) |
But what circumstances will result in |
release binaries: no suffixes non-release binaries:
|
When you say "non-release binaries" do you mean building from any commit that doesn't have a tag directly on it? |
This pull request has conflicts, please rebase. |
I mean binaries with |
2d1279a
to
767e8b7
Compare
rebased to fix merge conflicts after rc bump (#5656), no other changes |
But in that case wouldn't building v20.0.0-beta.8 result in being shown v20.0.0-beta.8-dev? |
Ah, right, I missed this in my PR description 🙈 (fixed). My initial idea was to make it show the tag only but then I realized that it would be possible to unintentionally create non-release binaries that would report the same version release binaries do so I added suffixes. It's a nice safety belt imo but if that's not needed I can drop them, np. |
else | ||
git rev-parse --short=12 HEAD | ||
fi | ||
recent_tag="$(git describe --abbrev=12 --dirty 2> /dev/null)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is git describe --abbrev=12
produce same output on different platform/copies of repo/etc?
This string should be reproducable at any system/git status because it is included in binaries.
If it is not true, this issue will revive with new face: #5222
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be identical (assuming 12 digits is enough)
--abbrev=
Instead of using the default number of hexadecimal digits (which will vary according to the number of objects in the repository with a default of 7) of the abbreviated object name, use digits, or as many digits as needed to form a unique object name. An of 0 will suppress long format, only showing the closest tag.
https://www.git-scm.com/docs/git-describe#Documentation/git-describe.txt---abbrevltngt
This pull request has conflicts, please rebase. |
767e8b7
to
47e38de
Compare
f26c36e
to
15ba58f
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
doc/release-process.md
Outdated
|
||
Before every minor and major release: | ||
|
||
* Update [bips.md](bips.md) to account for changes since the last release. | ||
* Update version in `configure.ac` (don't forget to set `CLIENT_VERSION_IS_RELEASE` to `true`) (don't forget to set `CLIENT_VERSION_RC` to `0`) | ||
* Update version in `configure.ac` (don't forget to set `CLIENT_VERSION_IS_RELEASE` to `true`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: IMO ok to have it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLIENT_VERSION_RC was dropped
define(_CLIENT_VERSION_MAJOR, 20) | ||
define(_CLIENT_VERSION_MINOR, 1) | ||
define(_CLIENT_VERSION_BUILD, 0) | ||
define(_CLIENT_VERSION_RC, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropping the RC here? Ah, rc is purely based on tag now
I don't think this is working as intended with a tag at head
building from
|
Even with a more normal looking tag this is problematic
|
Ahh; I must do |
This pull request has conflicts, please rebase. |
15ba58f
to
560cebc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
6203314
to
502673a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
502673a
to
42ea1ef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
42ea1ef
to
53c2c13
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…e it's aligned with scripts in `contrib/guix`
53c2c13
to
d0b3e25
Compare
Guix Automation has began to build this PR tagged as v20.1.0-devpr5653.d0b3e250. A new comment will be made when the image is pushed. |
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v20.1.0-devpr5653.d0b3e250. The image should be on dockerhub soon. |
|
Good catch! Should be working now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK for squash merge; I'm a little concerned about ability to have multiple tags on one commit; but I guess we can just push a new empty commit to push a new tag
I think even in this case with two tags on the same commit it should work as expected; ie the later tag gets priority; tested via
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM didn't test though
Issue being fixed or feature implemented
Client version string is inconsistent. Building
v20.0.0-beta.8
tag locally produces binaries that reportv20.0.0-beta.8
version but binaries built in guix would reportv20.0.0rc1-g3e732a952226a20505f907e4fd9b3fdbb14ea5ee
instead. Building any commit afterv20.0.0-beta.8
locally would result in versions likev20.0.0rc1-8c94153d2497
which is close but it's still yet another format. And both versions withrc1
in their names are confusing cause you'd expect them to mentionbeta.8
instead maybe (or is it just me? :D ).What was done?
Change it so that the version string would look like this:
on tag:
v20.0.0-beta.8-dev
orv20.0.0-beta.8-gitarc
v20.0.0-beta.8
post-tag:
v20.0.0-beta.8-1-gb837e08164-gitarc
v20.0.0-beta.8-1-gb837e08164
post-tag format is
recent tag
-commits since that tag
-g+12 chars of commit hash
-dirty (optional)
-dev or gitarc
dev
/gitarc
suffixes should help avoiding confusion with the release versions and they also indicate the way non-release binaries were built.Note that release binaries do not use any of this, they still use
PACKAGE_VERSION
fromconfigure
like before.Also,
CLIENT_VERSION_RC
is no longer used in this setup so it was removed.Few things aren't clear to me yet:
configure.ac
no longer affects the reported version (unless it's an actual release). Are there any downsides I might be missing?develop
once we bump version in configure?v21.0.0-init
?v21.0.0-alpha1
?merge master back into develop
kind of PR is merged? E.g. saydevelop
branch is onv21.0.0-alpha1
tag and we merge v20.1.0 frommaster
back into it. Will this bringv20.1.0
release tag intodevelop
? Will it become the one that will be used from that moment? If so we will probably need another tag ondevelop
every time such PR is merged e.g.v21.0.0-alpha2
(or whatever the next number is).Don't think these are blockers but would like to hear thoughts from others.
How Has This Been Tested?
Built binaries locally, built them using guix at a specific tag and at some commit on top of it.
Breaking Changes
n/a
Checklist: