search

atoomic / perl

a repo to show what could be p7
Other
18 stars 8 forks source link

alpha Objective 2: strict-by-default #153

Closed jkeenan closed 3 years ago

jkeenan commented 3 years ago

Objective: Implement 'strict-by-default'.

Modify the guts such that strictures are now on by default. The statement use strict; should become a "no-op" except where a particular file has previously called some variant of no strict;.

No other guts changes except those needed to implement this objective.

Note: If the lead developer feels it would be better to sub-divide this objective into three sub-objectives -- 'strict-vars-by-default', 'strict-subs-by-default', 'strict-refs-by-default' -- that's cool. We'll just need to create tickets for that.

Initially, there will be many files in the test suite and many dual-life modules that will fail to compile once we are strict-by-default. If we were being very purist, the only revisions we would make to such files would be those that get the file to compile and, in the case of a file in the test suite, to get the test to run GREEN.

However, many files in the test suite have already been brought into compliance with both strict-by-default, warnings-by-default and other, later objectives. We can make use of that prior art by appropriately cherry-picking commits to such files from earlier branches.

Acceptance Criteria: The changes needed to have Perl be strict-by-default are clearly available from the commit history and documented in the commit log. The Perl test suite runs GREEN in all major configurations and on all major operating systems for which we can test.

atoomic commented 3 years ago

I think we can target all strict: 'strict-vars-by-default', 'strict-subs-by-default', 'strict-refs-by-default' but if it s an issue we can revisit

Leont commented 3 years ago

I think we can target all strict: 'strict-vars-by-default', 'strict-subs-by-default', 'strict-refs-by-default' but if it s an issue we can revisit

Odds are strict-vars-by-default is 95% of the work

atoomic commented 3 years ago

and the other 5% for strict-refs :-)

atoomic commented 3 years ago

Here is what I think a cleaner commit to enable strict by default https://github.com/atoomic/perl/commit/1034c6d4017c6275b01093a8d5ee322e14d3fd77

jkeenan commented 3 years ago

We are now ready to begin work on Objective 2, strict-by-default.

Commit 800f84ad07 in the alpha branch has been tagged alpha-01 and becomes our branching-off point.

I have created a new branch, alpha-dev-02-strict, which will be our "objective" or "working" branch for this objective. That is, it is the branch into which we will merge work we are doing in our more private branches to reach three sub-objectives:

@atoomic, when you are ready, could you start the work on this branch off by merging what you have been doing privately to achieve the first bullet-point above.

Please let me know when you do so, since at that point I will want to run test suite to get a baseline for what we have to do there. Once we get that baseline, I will then merge in what I have been doing locally to get the test suite into shape for strict-by-default.

Thank you very much. Jim Keenan

atoomic commented 3 years ago

The first point Converting perl internally to run with strict on by default. is done in alpha-dev-02-strict via a08a7c150f9bfcb4ddeda1883a28fbe1864dd802

Right now you cannot compile perl without fixing the strictness issues in multiple locations. I see several options:

  1. cherry pick some fixes from different branches p7, ... other topic branches
  2. try to fix the strictness manually until we can compile perl
  3. enforce a use v5 everywhere so we can get a clean compilation process then go back and get rid of all the use v5

Any other ideas?

jkeenan commented 3 years ago

The first point Converting perl internally to run with strict on by default. is done in alpha-dev-02-strict via a08a7c1

Right now you cannot compile perl without fixing the strictness issues in multiple locations. I see several options:

Thanks for those commits. I've reviewed the diff between a08a7c150 and the starting point of the branch (800f84ad070df976fd5e07a60968328a254c15f6 or tag: alpha-01). I have several comments.

First, while make is not yet completing successfully, make miniprep (or, more precisely, make minitest_prep) is. So let's reframe the problem as "How do we get from make miniprep to make test and then on to make test_prep?"

What comes after make minitest_prep? It is, of course, make minitest, which translates to:

cd t &&   ./perl -Ilib -I. TEST base/*.t comp/*.t cmd/*.t run/*.t io/*.t re/*.t opbasic/*.t op/*.t uni/*.t perf/*.t </dev/tty

Now, locally I believe I have all *.t files in those directories passing when run from alpha-01 with ./perl -Ilib -M strict <testfile>. So I think that if I put my revisions to those files into the branch (or, initially, a local branch forked from the working branch at a08a7c150, we could see if we pass make minitest.

But to do that we should first revert the changes you've made already in these test files:

cpan/ExtUtils-Constant/t/Constant.t
t/comp/require.t
t/comp/use.t
t/porting/diag.t

The changes in Constant.t are premature. Getting cpan/Some-Distro/t/*.t tests to pass with strict-by-default will be the last step in the process (also, paradoxically, the easiest).

Second, once we get make minitest PASSing, we should then basically re-trace the steps of a normal make in Perl 5. To that end, we can capture the output of a similar build (probably threaded) on the same machine and take it one step at a time. (I'm starting a threaded build on Linux on Perl 5 blead right now and will capture make output.)

That would imply that we would "fix" modules under lib/, dist/ and cpan/ in the order in which make processes them.

That in turn would imply that we should revert most or all of the changes in those 4 directories until the point where make needs to handle them.

cpan/DB_File/DB_File_BS
cpan/Pod-Checker/scripts/podchecker.PL
cpan/Pod-Usage/scripts/pod2usage.PL

cpan/Text-Tabs/lib/Text/Tabs.pm
cpan/Text-Tabs/lib/Text/Wrap.pm
...
dist/Exporter/lib/Exporter.pm
...
ext/XS-APItest/APItest_BS
...
lib/Symbol.pm
lib/overload.pm

The only thing where I'm unsure whether we should revert or not is changes like these:

-           if $running_under_some_shell;
+           if 0; # ^ Run only under a shell

But, here again, I think if go through what make does step-by-step, only making changes in the 4 library directories when they are needed, our path is likely to become more clear.

jkeenan commented 3 years ago

Follow-up to previous comment.

@atoomic, I looked at these one commit at a time and made recommendations inline.

$ git log --format=short --reverse HEAD^^^^^^^..HEAD |cat
commit a4786ae2e11465fa0f0ca07318acf81e8e03238c
Author: Nicolas R <nicolas@atoomic.org>

    Fix strict in a few places

Throw out all of this except possibly the change to regen/regen_lib.pl

commit 7e647b7d68188b3e5dad79c92927e93a2efb8618
Author: Nicolas R <nicolas@atoomic.org>

    Enable strict by default

Looks good.

commit c1eb8193866dd20b8b555e336a4f7f4bcea17938
Author: Nicolas R <nicolas@atoomic.org>

    Restore 'use 5.x' behavior as it behaves in Perl 5

Throw out changes to cpan/ExtUtils-Constant/t/Constant.t, t/comp/require.t, t/comp/use.t. Remove changes to t/porting/diag.t -- but save them for later as that's a test not currently present in that file.

commit ae66b9742f9bcbe3710b35a527e07ab44e7f10af
Author: Nicolas R <nicolas@atoomic.org>

    do not promote strict on -e

Looks good (though behavior of -e has not yet been finalized but this looks okay for now).

commit b397aa7bba3e962d4b419b92ebe46fce4f419e64
Author: Todd Rinaldo <toddr@cpan.org>

    No more BS

Remove changes to cpan/DB_File/DB_File_BS and ext/XS-APItest/APItest_BS. We'll add them back in when make insists we do.

commit e9f6ca8a36184e60530f686fea6d6d8d9ec90cd9
Author: Nicolas R <nicolas@atoomic.org>

    set minus_e correctly and disable strict for -e and -E

Looks good, subject to proviso that expected behavior for -e and -E has not been finalized.

commit a08a7c150f9bfcb4ddeda1883a28fbe1864dd802
Author: Nicolas R <atoomic@cpan.org>

    Do not recommend using an undefined variable

For now, let's back out these changes and start adding them back in as we work our way through make.

So, once again, we have to do some backtracking. But I think that if we do the above, then work on getting make minitest passing, we'll have a clear path forward.

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

I've been testing out my suggestions in the previous comment ... but experiencing problems. Re-examining.

atoomic commented 3 years ago

@jkeenan feel free to revert any commit/files and push this directly to this working branch

jkeenan commented 3 years ago

@atoomic, here's an update on what I was able to accomplish toward Objective 2 yesterday.

We Can Now Run make test

I created the jkeenan/alpha-dev-02-restate-the-problem branch which I have pushed upstream so that you have easy visibility into it.

This branch starts from tag alpha-01, the same starting point we're using in the alpha-dev-02-strict branch. At the moment the branch consists of 241 commits.

The first 31 commits of these are the most important. If you squint, you can see that they include the actual changes to the core guts which you made to turn on strict-by-default. Some of your commits I applied verbatim via git am. The other commits in this set can be described as "the minimum changes we had to make to achieve strict-compliance for those files needed to get make test_prep to complete." Some of them touch the same files you were touching in the alpha-dev-02-strict branch, but bring those files into strict-compliance without resorting to use v5 or wrapping the entire file in no strict.

If we think these commits are correct, we would do a hard reset on alpha-01 and reconstitute the alpha-dev-02-strict branch with these commits.

My first hope (as suggested in one of my posts from Aug 07) was to get to make minitest_prep -- which presumes we have gotten to make miniperl -- then run and fix make minitest, and only thereafter tackle getting to make test_prep. I got there, but encountered a problem, to be discussed below, when calling make minitest.

I decided to dodge make minitest and keep moving toward a successful make test_prep. This entailed running make test_prep until it failed, examining the output and figuring out what file had to be brought into strict-compliance. I continued this process until I reached this commit:

commit f4f491767100eb12e6bf001de896bde1071d1fd1 (tag: achieved-make-test-prep)
Author: James E Keenan <jkeenan@cpan.org>
Date:   Fri Aug 7 19:17:09 2020 -0400

    test_use.pm: Adapt to honor strict-by-default

At this point we can now call make test.

The next 200 or so commits constitute my work over the past several weeks in local branches to bring individual test files into strict-compliance. In most cases, one commit reflects work on one test file and (through the miracle of git format-patch, git am and git rebase -i) one commit embodies all the work on a given test file.

Some of these 200+ test files are not PASSing or have tests TODO-ed that are not TODO-ed in the Perl 5 test suite. I tried to indicate that in commit messages.

This series of commits terminates with:

commit eed8a3f1d7763921d407468e693a21e7bd5ece8f
Author: James E Keenan <jkeenan@cpan.org>
Date:   Fri Aug 7 11:03:13 2020 -0400

    t/porting/test_bootstrap.t: Adapt to honor strict-by-default

    Which, in this case, means we no longer need an explicit 'use strict'.

At this point, I hit make test for the first time. I encountered two kinds of problems:

Test Files Having Difficulty Locating 'strict'

I now want to mention the problem alluded to above. In certain test files in the "early" directories (e.g., t/base and t/comp), tests are failing because they cannot find strict.pm.

$ TEST_JOBS=1 make test TEST_FILES="base/*.t comp/*.t"
...
[ make test_prep runs successfully and tests begin; then ... ]
./runtests choose
t/base/cond ............... ok
t/base/if ................. ok
t/base/lex ................ Can't locate strict.pm in @INC (you may need to install the strict module) (@INC contains: /usr/local/lib/perl7/site_perl/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/site_perl/7.0.0 /usr/local/lib/perl7/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/7.0.0) at base/lex.t line 97.
BEGIN failed--compilation aborted at base/lex.t line 97.
FAILED--no leader found
t/base/num ................ ok
t/base/pat ................ ok
t/base/rs ................. ok
t/base/term ............... ok
t/base/translate .......... ok
t/base/while .............. ok
t/comp/bproto ............. ok
t/comp/cmdopt ............. ok
t/comp/colon .............. ok
t/comp/decl ............... ok
t/comp/filter_exception ... Can't locate strict.pm in @INC (you may need to install the strict module) (@INC contains: /usr/local/lib/perl7/site_perl/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/site_perl/7.0.0 /usr/local/lib/perl7/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/7.0.0) at ./test.pl line 22.
BEGIN failed--compilation aborted at ./test.pl line 22.
Compilation failed in require at comp/filter_exception.t line 5.
BEGIN failed--compilation aborted at comp/filter_exception.t line 6.
FAILED--no leader found
t/comp/final_line_num ..... ok
t/comp/fold ............... Can't locate strict.pm in @INC (you may need to install the strict module) (@INC contains: /usr/local/lib/perl7/site_perl/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/site_perl/7.0.0 /usr/local/lib/perl7/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/7.0.0) at comp/fold.t line 120.
BEGIN failed--compilation aborted at comp/fold.t line 120.
FAILED--no leader found
t/comp/form_scope ......... ok
t/comp/hints .............. ok
t/comp/line_debug ......... Can't locate strict.pm in @INC (you may need to install the strict module) (@INC contains: . /usr/local/lib/perl7/site_perl/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/site_perl/7.0.0 /usr/local/lib/perl7/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/7.0.0) at comp/line_debug.t line 28.
BEGIN failed--compilation aborted at comp/line_debug.t line 28.
FAILED--no leader found
t/comp/multiline .......... ok
t/comp/opsubs ............. ok
t/comp/our ................ ok
t/comp/package ............ ok
t/comp/package_block ...... ok
t/comp/parser ............. ok
t/comp/parser_run ......... Can't locate strict.pm in @INC (you may need to install the strict module) (@INC contains: /usr/local/lib/perl7/site_perl/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/site_perl/7.0.0 /usr/local/lib/perl7/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/7.0.0) at ./test.pl line 22.
BEGIN failed--compilation aborted at ./test.pl line 22.
Compilation failed in require at comp/parser_run.t line 9.
BEGIN failed--compilation aborted at comp/parser_run.t line 11.
FAILED--no leader found
t/comp/proto .............. ok
t/comp/redef .............. ok
t/comp/require ............ ok
t/comp/retainedlines ...... Can't locate strict.pm in @INC (you may need to install the strict module) (@INC contains: /usr/local/lib/perl7/site_perl/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/site_perl/7.0.0 /usr/local/lib/perl7/7.0.0/x86_64-linux-thread-multi /usr/local/lib/perl7/7.0.0) at comp/retainedlines.t line 163.
BEGIN failed--compilation aborted at comp/retainedlines.t line 163.
FAILED--no leader found
t/comp/term ............... ok
t/comp/uproto ............. ok
t/comp/use ................ FAILED at test 9
t/comp/utf ................ ok
Failed 7 tests out of 28, 75.00% okay.
    base/lex.t
    comp/filter_exception.t
    comp/fold.t
    comp/line_debug.t
    comp/parser_run.t
    comp/retainedlines.t
    comp/use.t
### Since not all tests were successful, you may want to run some of
[snip]
Elapsed: 3 sec
u=0.03  s=0.02  cu=0.41  cs=0.24  scripts=28  tests=5416
makefile:800: recipe for target 'test' failed
make: *** [test] Error 1

These failures are occurring during both make minitest and make test. I haven't had time to fully diagnose these failures. I have a hunch that these test files -- which go back as far as Perl 1 in 1987 -- were written before strictures were ever invented and always dealt exclusively with global variables. Hence, if we run them against a strict-by-default perl, we have to relax strictures around certain statements to get them to PASS. But calling, e.g., no strict 'vars'; assumes the program knows where to find one of strict.pm or vars.pm.

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

@atoomic, here's an update on what I was able to accomplish toward Objective 2 yesterday.

And now updated to Sun Aug 09.

We Can Now Run make test

I created the jkeenan/alpha-dev-02-restate-the-problem branch which I have pushed upstream so that you have easy visibility into it.

This branch starts from tag alpha-01, the same starting point we're using in the alpha-dev-02-strict branch. At the moment the branch consists of 241 commits.

The first 31 commits of these are the most important. If you squint, you can see that they include the actual changes to the core guts which you made to turn on strict-by-default. Some of your commits I applied verbatim via git am. The other commits in this set can be described as "the minimum changes we had to make to achieve strict-compliance for those files needed to get make test_prep to complete." Some of them touch the same files you were touching in the alpha-dev-02-strict branch, but bring those files into strict-compliance without resorting to use v5 or wrapping the entire file in no strict.

If we think these commits are correct, we would do a hard reset on alpha-01 and reconstitute the alpha-dev-02-strict branch with these commits.

And this is still my plan. We'll need to discuss before proceeding.

[snip]

That's where I left off last night. At this point the output of make test_harness is running to 42,422 lines. That compares to 4,413 lines in Perl 5 blead for the same configuration on the same machine. So we have a ways to go, but my hunch is that small fixes to certain infrastructural files will go a long way.

And now we're down from 42422 lines to 23480 lines. Failed 814 tests out of 2025, 59.80% okay.

[snip]

Today to get some tests passing I put a kludge into t/test.pl for its handling of @INC. However, I now think that was not a good idea and that what I need to do tomorrow is to rewrite t/test.pl so that it is strict-by-default.

Once we resolve the branch situation, here are some files that are failing that you might want to take a look at:

comp/use.t
run/fresh_perl.t
run/switches.t
io/pipe.t
op/fork.t
op/closure.t
op/chdir.t

Thank you very much. Jim Keenan

atoomic commented 3 years ago

Thanks James for the update, your branch seems ok, I would like to consider two changes as part of this branch. I know they are not strict by themselves but they are follow up fixes to the previous state.

  1. restore behavior for use 5.x, block use 6.* and new behavior for use v7

    • c1eb819386 Restore 'use 5.x' behavior as it behaves in Perl 5

  2. do not enforce strict on oneliners when using -e or -E

    • e9f6ca8a36 set minus_e correctly and disable strict for -e and -E
    • ae66b9742f do not promote strict on -e
jkeenan commented 3 years ago

On 8/10/20 11:05 AM, Nicolas R. wrote:

Thanks James for the update, your branch seems ok, I would like to consider thew two changes as part of this branch. I know they are not strict by themselves but they are follow up fixes to the previous state.

  1. restore behavior for |use 5.x|, block |use 6.*| and new behavior for |use v7|

The above sounds like something that should be done before embarking on Objective 2.

Could you write this up as a separate ticket, calling it, say, "Objective 1a: Modify behavior for certain 'use VERSION' statements"?

I would then recommend forking a new branch from tag alpha-01 and working through the implications of the commit above. (Hint: This might include work on t/comp/use.t.)

We'd then both test it and, when okay, merge it back into alpha, tag it something like 'alpha-01a', and then re-fork a branch for Objective 2 for it.

  1. do not enforce strict on oneliners when using |-e| or |-E|

This sounds like something we could include in Objective 2 once we've completed Objective 1a above.

Thank you very much. Jim Keenan

atoomic commented 3 years ago

I created #187 to track these changes

jkeenan commented 3 years ago

Progress report: I've been tracking our progress via two somewhat crude but easily obtainable data points: the number of lines in the output from make test and the total number of failing files reported by make test.

By way of contrast, in Perl 5 blead make test runs to 4413 lines and (unless someone has broken blead) to 0 failing test files.

When we began work on Objective 2, we got 42422 lines in make test output. (I wasn't yet noting down the number of failing files.

At a point on Aug 09 where I started to note both the line count and the count of failures, we were at:

24543 lines Failed 863 tests out of 1983, 56.48% okay.

As of earlier today, we're at:

16421 Failed 390 tests out of 2374, 83.57% okay.

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

Following merge of the branch for Objective 1a into alpha, this branch has been rebased on alpha.

jkeenan commented 3 years ago

I have just tagged commit b5a0552b6a as alpha-02-MC-1.

That means that b5a0552b6a is the first commit at which we believe that the alpha-dev-02-strict working branch is suitable for merging into the main alpha branch.

'MC-1' means the first Merge Candidate for the current working branch. 'alpha-02' refers to Objective 2.

We should treat the alpha-dev-02-strict branch as being in quasi-code-freeze and try to test our codebase at alpha-02-MC-1 on as many platforms and configurations as possible. To allow smoke testers time to run, this testing period is expected to last 36 to 48 hours. If smoke-testing reveals bugs, we will have to make changes in the alpha-dev-02-strict branch, create a new Merge Candidate

When we are satisfied with the results we will merge the branch into alpha, tag alpha with alpha-02 and fork a branch for working on Objective 3, warnings-by-default.

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

We should treat the alpha-dev-02-strict branch as being in quasi-code-freeze and try to test our codebase at alpha-02-MC-1 on as many platforms and configurations as possible. To allow smoke testers time to run, this testing period is expected to last 36 to 48 hours. If smoke-testing reveals bugs, we will have to make changes in the alpha-dev-02-strict branch, create a new Merge Candidate

Smoke-testing has indeed revealed bugs which are being tracked in these tickets:

https://github.com/atoomic/perl/issues/257 https://github.com/atoomic/perl/issues/258

As we make progress we'll make commits to the alpha-dev-02-strict branch and issue new Merge Candidate tags as needed.

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

Having made some progress on https://github.com/atoomic/perl/issues/257, I have applied tag alpha-02-MC-2 to commit 9ecd4bde26408b0471b2fc17ab290bbc769821f8 in the alpha-dev-02-strict branch.

jkeenan commented 3 years ago

Overnight we got a spurious smoke-testing failure due to a race condition in version 2.14 of ExtUtils-Install. I upgraded our branch to use CPAN release 2.16, then brought all files in that directory into strict-compliance. I then applied tag alpha-02-MC-3 at commit 5c307cbe7fe9bc332a98c7095bfe59fae4152d77. (I had to back out oodler577's commit due to a porting test failure.)

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

I have tagged commit da0a32ff74ba2ef2686dc14c210eb832fbff472c in the alpha-dev-02-strict branch as alpha-02-MC-4. This follows the merge of a branch in which @Toddr was working to get us passing on Windows. Let's get some outside testing done at this tag.

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

Tonight I reviewed the status of our simulation branch in the Perl5 repository: http://perl.develop-help.com/?b=smoke-me%2Fjkeenan%2Fcumberland-blues. All test failures are there are either intermittent and covered by issues in this repository or are failing on the same test smokers for Perl 5 blead.

I then rebased the alpha-dev-02-strict branch on alpha, resolving merge conflicts in pp_ctl.c and t/comp/require.t. I then merged alpha-dev-02-strict into alpha and went through the configure-build-test cycle on Linux, FreeBSD, OpenBSD and NetBSD. All tests PASSed.

I then tagged the HEAD of alpha as alpha-02 and push upstream. I deleted upstream/alpha-dev-02-strict from our repository.

This complete our work on Objective 2, "strict-by-default".

Thank you very much. Jim Keenan

jkeenan commented 3 years ago

I then tagged the HEAD of alpha as alpha-02 and push upstream. I deleted upstream/alpha-dev-02-strict from our repository.

This complete our work on Objective 2, "strict-by-default".

Please rebase any branches that were forked from alpha-dev-02-strict on alpha.