perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

Rename Perl version macro

Open atoomic opened this issue 4 years ago • 21 comments

This commit introduces Semantic Versioning macro for Perl version.

The previously named variables: PERL_REVISION, PERL_VERSION, PERL_SUBVERSION are renamed as PERL_VERSION_MAJOR, PERL_VERSION_MINOR, and PERL_VERSION_PATCH

A new variable PERL_VERSION_HEX is also introduced.

References: https://semver.org

atoomic avatar Aug 12 '20 16:08 atoomic

@tonycoz I marked this PR as draft, as this need discussion and performed the several changes you requested via cfbc8487e41595e5c9e55b8f6b1e72111f03c0c0

atoomic avatar Aug 13 '20 15:08 atoomic

@atoomic, can you provide an update on the status of this p.r. for semantic versioning?

Thank you very much. Jim Keenan

jkeenan avatar Jan 26 '21 01:01 jkeenan

There are several question here:

  1. Should we add these symbols at all?
  2. Should they be aliases for PERL_REVISION, PERL_VERSION, and PERL_SUBVERSION; or should it be the other way around.
  3. That do they really mean. Should they be interpreted as strict semantic versioning, or as the current versioning scheme.

Unless the answer to the first question is no, all of these should be answered (though that answer may depend on the third question).

Leont avatar Jan 27 '21 17:01 Leont

There are several question here:

1. Should we add these symbols at all?

2. Should they be aliases for PERL_REVISION, PERL_VERSION, and PERL_SUBVERSION; or should it be the other way around.

3. That do they really mean. Should they be interpreted as strict semantic versioning, or as the current versioning scheme.

Unless the answer to the first question is no, all of these should be answered (though that answer may depend on the third question).

No one has responded to @Leont's questions in a year-and-a-half. There's no reason to retain p.r.'s that are in Draft status. I recommend that this p.r. be closed.

jkeenan avatar Jul 03 '22 17:07 jkeenan

I'm pretty sure this change is obsolete, or way, way ahead of its time.

I think the intent was new symbols for a new Perl 7, while maintaining the old names for compatibility with code that only checked PERL_VERSION and PERL_SUBVERSION when checking for compatibility/new features.

tonycoz avatar Jul 04 '22 00:07 tonycoz

I'm not sure it's obsolete. There is still the intention to be able to bump the first component of the version number beyond 5 and into 7, 8, and beyond at some point in the future.

There are many obstacles to doing this, but one of them is the fact that so much existing code on CPAN has a ppport.h which insists on PERL_REVISION == 5.

The new macro form at least partly solves this, and does so by renaming the fields to what has been generally accepted industry-wide as a more standard naming structure of MAJOR.MINOR.PATCH. Overall it's a good idea, and I'm in favour of it.

leonerd avatar Jul 05 '22 22:07 leonerd

I'm not sure it's obsolete. There is still the intention to be able to bump the first component of the version number beyond 5 and into 7, 8, and beyond at some point in the future.

There are many obstacles to doing this, but one of them is the fact that so much existing code on CPAN has a ppport.h which insists on PERL_REVISION == 5.

The new macro form at least partly solves this, and does so by renaming the fields to what has been generally accepted industry-wide as a more standard naming structure of MAJOR.MINOR.PATCH. Overall it's a good idea, and I'm in favour of it.

That makes sense, and answers at least the first two of my questions.

Leont avatar Jul 07 '22 01:07 Leont

This merge request was created more than two years ago. I reviewed it today. @Leont indicates that 2 of his 3 questions have been answered, but that appears to leave this one:

[What do @atoomic's new or renamed variables] really mean? Should they be interpreted as strict semantic versioning, or as the current versioning scheme?

Could we focus discussion on that?

Also: @atoomic, this p.r. has acquired many merge conflicts. Could you clear them up?

Thank you very much. Jim Keenan

jkeenan avatar Sep 16 '22 22:09 jkeenan

To clarify something, these symbols already exist as aliases to the classic version defines. This PR changes the rest of the code to use the new symbols, and switches the classic defines to be aliases for the new ones.

Even without any change in policy, I think this is a good change to make. I think it's reasonable to refer to the different components of the version as MAJOR, MINOR, and PATCH under the current versioning scheme. And switching everything to use these could allow the old defines to be given "fake" values to assist in bumping the major version past 5.

A policy change related to semver should be considered separately. I doubt we're anywhere close to wanting to make that change. Generally semver is interpreted to also apply to binary compatibility. This would mean bumping the major version each year under our current development process. I don't think this is generally what people want.

haarg avatar Sep 17 '22 09:09 haarg

I'm definitely keen to see this go in. The new variable names are neater and easier to remember (major.minor.patch is industry-standard, as compared our previous revision.version.subversion - what even is that??!) and also have a certain visual neatness of all being the same length so for that reason alone it's already an improvement.

Additionally it would give us the flexibility to bump the major version up to 7 or beyond at some future date, which is a change we currently cannot make. So keeping that in mind this is also good.

I also feel this is purely a technical-mechanism change. We're not saying "adopt semver" with all the policy suggestions that makes. The code and the commit message don't mention semver, but this PR description currently still does. As long as we all accept that this change itself does not introduce semver I don't think that matters.. but maybe you want to edit the PR description as well to remove that mention if the discussion continues.

But I don't think it has to. The principle seems sound, I have no unanswered questions. If you fix up the conflicts and generally make this merge-ready I'd be happy to give it a review in the hope of merging it and finally putting this one to bed.

leonerd avatar Sep 17 '22 10:09 leonerd

The new variable names are neater and easier to remember

I'm not sure that is relevant, given that everyone really ought to be using PERL_VERSION_GE and PERL_VERSION_LT anyway.

Leont avatar Sep 17 '22 14:09 Leont

I can rebase and rework that Pull Request but I will only do that if there is any interest in merging this or part of this Pull Request. If the consensus is that this does not provide value, then let's close the pull request.

atoomic avatar Sep 19 '22 14:09 atoomic

On 9/19/22 08:55, ℕicolas ℝ. wrote:

I can rebase and rework that Pull Request but I will only do that if there is any interest in merging this or part of this Pull Request. If the consensus is that this does not provide value, then let's close the pull request.

I would like this done, so +1

— Reply to this email directly, view it on GitHub https://github.com/Perl/perl5/pull/18059#issuecomment-1251137622, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2DH7W3NUKTOP73LDVSQDV7B5F3ANCNFSM4P454XOQ. You are receiving this because your review was requested.Message ID: @.***>

khwilliamson avatar Sep 19 '22 21:09 khwilliamson

On Mon, 19 Sept 2022, 22:31 Karl Williamson, @.***> wrote:

On 9/19/22 08:55, ℕicolas ℝ. wrote:

I can rebase and rework that Pull Request but I will only do that if there is any interest in merging this or part of this Pull Request. If the consensus is that this does not provide value, then let's close the pull request.

I would like this done, so +1

Karl, t is not entirely clear what "this" is. I assume you want it merged, but your statement supports both interpretations.

Yves

Yves

demerphq avatar Sep 19 '22 22:09 demerphq

On 9/19/22 16:08, Yves Orton wrote:

On Mon, 19 Sept 2022, 22:31 Karl Williamson, @.***> wrote:

On 9/19/22 08:55, ℕicolas ℝ. wrote:

I can rebase and rework that Pull Request but I will only do that if there is any interest in merging this or part of this Pull Request. If the consensus is that this does not provide value, then let's close the pull request.

I would like this done, so +1

Karl, t is not entirely clear what "this" is. I assume you want it merged, but your statement supports both interpretations.

Sorry. I think this PR is a step forward, and should be merged when it is ready to be so.

khwilliamson avatar Sep 20 '22 14:09 khwilliamson

@atoomic the consensus seems to be that people do want to merge this, but that there are some minor changes required before it merges. Also the PR is currently in conflict. Do you want to get it up to date or should someone else pick it up?

demerphq avatar Feb 08 '23 07:02 demerphq

No problems @demerphq I will revisit it an extra time :-) and submit and updated PR

atoomic avatar Feb 08 '23 16:02 atoomic

No problems @demerphq I will revisit it an extra time :-) and submit and updated PR

@atoomic, are we going forward with this pull request? (It now has merge conflicts.) Thanks.

jkeenan avatar Jul 31 '23 16:07 jkeenan

It'd be good to get this tidied up and ready to go. Can someone - @atoomic or elsewise - rebase and tidy it up ready to merge?

In the jkeenan/semantic-versioning branch in the main repository, I have attempted to rebase @atoomic's branch on blead.

https://github.com/Perl/perl5/tree/jkeenan/semantic-versioning

https://github.com/Perl/perl5/compare/jkeenan/semantic-versioning?expand=1

Since it took me 4 tries to get it right, please review it carefully. I'm not sure how or whether I make that code the content of this pull request. Please advise.

jkeenan avatar Oct 19 '23 18:10 jkeenan

It'd be good to get this tidied up and ready to go. Can someone - @atoomic or elsewise - rebase and tidy it up ready to merge?

In the jkeenan/semantic-versioning branch in the main repository, I have attempted to rebase @atoomic's branch on blead.

https://github.com/Perl/perl5/tree/jkeenan/semantic-versioning

https://github.com/Perl/perl5/compare/jkeenan/semantic-versioning?expand=1

Since it took me 4 tries to get it right, please review it carefully. I'm not sure how or whether I make that code the content of this pull request. Please advise.

@atoomic @leonerd can you please review above? Thanks.

jkeenan avatar Dec 09 '23 13:12 jkeenan

It'd be good to get this tidied up and ready to go. Can someone - @atoomic or elsewise - rebase and tidy it up ready to merge?

In the jkeenan/semantic-versioning branch in the main repository, I have attempted to rebase @atoomic's branch on blead.

https://github.com/Perl/perl5/tree/jkeenan/semantic-versioning

https://github.com/Perl/perl5/compare/jkeenan/semantic-versioning?expand=1

Since it took me 4 tries to get it right, please review it carefully. I'm not sure how or whether I make that code the content of this pull request. Please advise.

My branch which attempted to rebase @atoomic's original p.r. on blead has now been updated here: https://github.com/Perl/perl5/compare/blead...jkeenan:perl5:semantic-version-upgrade-20240324?expand=1

jkeenan avatar Apr 14 '24 20:04 jkeenan