-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Put build number in pre-release version instead of build metadata #17434
Conversation
I think the only way that would happen would be if some commits were made after dropping -pre from VERSION, but before adding it back for the next release. This shouldn't happen if we're careful about the release branch. I guess we'd have to be on the lookout for people still using +123 after this gets merged, and fix any occurrences of that for 0.6.0-dev or later. How does dev.unknown compare to known pre.123? |
also double check version_git.sh just to be sure. not sure how to test this. |
if [ $major = 0 -a $minor -le 5 ]; then | ||
echo "$ver+$nb" | ||
else | ||
echo "$ver.$nb" |
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.
I guess both of these (including the current script) are wrong if you happen to run it on a tag?
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.
Ah yes, should be just echo $ver
if $nb
is zero, I guess. Good opportunity to fix; will do next week.
Maybe we could eventually rewrite these scripts in a nicer programming language, like, say, Julia.
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.
Good opportunity to fix; will do next week.
Done.
I could imagine this scenario: Drop
Yes, that would be the obvious downside of accepting this PR.
IIUC it only gathers the raw data from git but does not have direct contact with the version numbers. |
can we throw in some tests to verify this kind of thing, if the existing version number comparisons don't already cover these? |
6fe018a
to
0c0a31d
Compare
Done. |
Orthogonal to this PR as it is now, but related: Should the "build number" maybe be based on the commit VERSION was last changed instead of on the latest tag that can be found? Seems a bit more robust to me. (If I tag some commit locally as kind of a bookmark or whatever, that should not screw up the version in my builds, should it?) |
Looking at changes to VERSION might indeed be more robust than looking at "most recent tag." We have had instances before of people making local tags and that screwing things up. |
Also in light of #17418 (comment), I propose to open a separte PR that uses |
Rebased on top of #17531, so this should be ready for merge after branching ... if we want it, that is. |
Assumes the change to take place when switching VERSION to 0.6.0-dev.
We'll need the equivalent of JuliaLang/Compat.jl#259, right? Is Or maybe it doesn't need to live in Compat.jl any more and can just be here, since it always refers to this repo... |
Ref #17330.
If accepted, this PR should be merged soonish after VERSION is set to
0.6.0-dev
. (That's the assumption made in contrib/commit-name.sh.) Also, bin/version.sh in Compat would need adjustment. Surprisingly, I haven't found other places that would need to be changed. Surely I missed some?Apart from switching from
0.6.0-dev+123
to0.6.0-dev.123
, this PR handles the situation where the "build number" (=number of commits since latest tag) is zero (which usually means could not be determined) differently. Instead of omitting it, it adds.unknown
if there is a pre-release version part in VERSION. E.g.0.6.0
stays0.6.0
, but0.6.0-dev
becomes0.6.0-dev.unknown
. OTOH, if VERSION is not a pre-release version but the "build number" is non-zero, it is ignored (and a corresponding message is printed). Would this be the right thing to do during preparation of a release?CC @tkelman