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

alpha objective 2a: restricting to 'use v7' should not be done. #263

Closed
toddr opened this issue Sep 2, 2020 · 21 comments
Closed

alpha objective 2a: restricting to 'use v7' should not be done. #263

toddr opened this issue Sep 2, 2020 · 21 comments
Labels
objective mileposts for 'alpha' development

Comments

@toddr
Copy link
Collaborator

toddr commented Sep 2, 2020

A windows bug cropped up which basically boiled down to the fact that we're erroring when one does require 7.000000.

This should be allowed and it is a mistake that we have not adopted the patch in this branch.

The error message that should not exist in perl is:

C:\perl\t>"C:\perl\perl.exe" -IC:\perl\perl.exe -le "require 7.000000; print qq{VER_OK}"
use v7; is the only supported syntax for Perl 7. at -e line 1.

@atoomic already has a patch for this. it just needs to go into the strict branch. We also need to remove all of the work arounds we've put into this branch to overcome this.

@atoomic atoomic added the objective mileposts for 'alpha' development label Sep 2, 2020
@atoomic
Copy link
Owner

atoomic commented Sep 2, 2020

This a change in the behavior introduced as part of objective 01 #152
This should not target the objective 02 for strict #153,
I suggest we perform that change at the beginning of warnings objective #154

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 2, 2020 via email

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 2, 2020 via email

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 2, 2020

@toddr, how we proceed depends, IMO, in large part on what those "workarounds" you mentioned earlier actually are.

If I understand you correctly, this commit:

$ gitshowf 55a6a9445ccc89766c2f311c055be144f70039e6
commit 55a6a9445ccc89766c2f311c055be144f70039e6
Author:     Todd Rinaldo <[email protected]>
AuthorDate: Wed Sep 2 11:33:16 2020 -0500
Commit:     Todd Rinaldo <[email protected]>
CommitDate: Wed Sep 2 12:30:58 2020 -0500

    Windows doesn't have the version set to 7

diff --git a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm
index 33836141ce..d3a902fdfa 100644
--- a/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm
+++ b/cpan/ExtUtils-MakeMaker/lib/ExtUtils/MM_Unix.pm
@@ -1145,7 +1145,7 @@ WARNING
         }
     }
 
-    if ( $ver && $ver eq '7' ) {
+    if ( $ver && $ver =~ m/^7/ ) {
         $ver = 'v7';
     }
 

... which is currently in the jkeenan/gha-258-win32-20200901 branch but is distinct from all the other commits which you and @atoomic have been doing there for Windows-readiness.

If that is correct, then what I recommend we do is to (i) extract that commit from that branch; (ii) fork a new branch off alpha and cherry-pick that commit into that branch; (iii) let that branch just sit there until we get Objective 2 wrapped up; (iv) make that branch our Objective 2a and work on it as I described earlier in this ticket.

But recommendations (i) thru (iv) assume the workarounds are inconsequential. So we need to know where we have done those workarounds and what we have to undo.

Thank you very much.
Jim Keenan

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 4, 2020

On 9/2/20 12:43 PM, Todd Rinaldo wrote: A windows bug cropped up which basically boiled down to the fact that we're erroring when one does |require 7.000000|. This should be allowed and it is a mistake that we have not adopted the patch in this branch. The error message that should not exist in perl is: |C:\perl\t>"C:\perl\perl.exe" -IC:\perl\perl.exe -le "require 7.000000; print qq{VER_OK}" use v7; is the only supported syntax for Perl 7. at -e line 1. | @atoomic https://github.com/atoomic already has a patch for this. it just needs to go into the strict branch. We also need to remove all of the work arounds we've put into this branch to overcome this.
Can you list those workarounds?

@toddr we're close to wrapping up our work on Objective 2 and merging our working branch back into alpha. If we're going to perform the work you recommend above as "objective 2a", can you list the workarounds, describe the effects of the problem and provide some acceptance criteria?

Thank you very much.
Jim Keenan

@atoomic
Copy link
Owner

atoomic commented Sep 4, 2020

I've got a branch with these changes alpha-dev-02-strict+v7
still need to adjust a few failures

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 4, 2020 via email

@atoomic
Copy link
Owner

atoomic commented Sep 4, 2020

The main issue is that during objective 01 to bump the major version from 5 to 7,
I've added a restriction not allowing any other syntax than use v7 (when the first digit is a 7)
the goal is to make it more relax and still allow use v7.0, use v7.1.... and co
several tests are doing something like use $] for example

atoomic added a commit that referenced this issue Sep 4, 2020
Fix #263

Do not restrict 'use v7' to 'v7'.
With this change we are now also accepting:
`use v7.0`, `use v7.x`...
@atoomic
Copy link
Owner

atoomic commented Sep 4, 2020

The Pull Request #265 fixes the concern there

@atoomic atoomic changed the title restricting to 'use v7' should not be done. alpha objective 2a: restricting to 'use v7' should not be done. Sep 4, 2020
@toddr
Copy link
Collaborator Author

toddr commented Sep 12, 2020

This should be reverted.

cpan/ExtUtils-Constant/t/Constant.t

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 12, 2020

This should be reverted.

cpan/ExtUtils-Constant/t/Constant.t

Can you describe why?

Thank you very much.
Jim Keenan

@toddr
Copy link
Collaborator Author

toddr commented Sep 13, 2020

Yes. the change seems to have clearly been made to support use v7. I suspect there are more like this. It was made in a1869a7 though which might include all of them.

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 21, 2020

With the completion of Objective 2, "strict-by-default" tonight, we are ready to begin work on Objective 2a.

I have created the following branch in our repository to hold our work on this objective:

alpha-dev-02a-restricting

@atoomic, @toddr: Please commit whatever work you've been doing toward this objective into this new branch.

Thank you very much.
Jim Keenan

atoomic added a commit that referenced this issue Sep 21, 2020
Fix #263

Do not restrict 'use v7' to 'v7'.
With this change we are now also accepting:
`use v7.0`, `use v7.x`...
@atoomic
Copy link
Owner

atoomic commented Sep 21, 2020

@jkeenan I pushed two commits to this branch in order to relax the use vX syntax to satisfy this micro objective.

  • fdeb46b Restore use v7.x behavior
  • fbf85d4 Revert change to ExtUtils-Constant/t/Constant.t

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 22, 2020 via email

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 22, 2020 via email

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 22, 2020

@jkeenan I pushed two commits to this branch in order to relax the use vX syntax to satisfy this micro objective.

* [fdeb46b](https://github.com/atoomic/perl/commit/fdeb46bd099d49d85c333218823a25eb17de45cb) Restore use v7.x behavior

* [fbf85d4](https://github.com/atoomic/perl/commit/fbf85d4894935cd819957ccb3ec3e35333e47d12) Revert change to ExtUtils-Constant/t/Constant.t

@toddr, In satisfaction of Objective 2a, are you okay with these two commits?

Thank you very much.
Jim Keenan

@atoomic
Copy link
Owner

atoomic commented Sep 22, 2020

@jkeenan this is indeed the two commits I'm talking about and do not expect to push any additional changes

  • fdeb46b Restore use v7.x behavior
  • fbf85d4 Revert change to ExtUtils-Constant/t/Constant.t

@toddr
Copy link
Collaborator Author

toddr commented Sep 22, 2020

looks right.

@jkeenan
Copy link
Collaborator

jkeenan commented Sep 23, 2020

I have tagged this branch as "alpha-02a-MC-1" at commit fbf85d4. Please conduct any outside testing that you can.

Thank you very much.
Jim Keenan

jkeenan pushed a commit that referenced this issue Sep 24, 2020
Fix #263

Do not restrict 'use v7' to 'v7'.
With this change we are now also accepting:
`use v7.0`, `use v7.x`...
@jkeenan
Copy link
Collaborator

jkeenan commented Sep 24, 2020

I have merged this branch into alpha and deleted it from the repository. We can now proceed to Objective 3, warnings-by-default.

Thank you very much.
Jim Keenan

@jkeenan jkeenan closed this as completed Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
objective mileposts for 'alpha' development
Projects
None yet
Development

No branches or pull requests

3 participants