Bisect runner tweaks

[nwc10]

Patches to enable bisect.pl to build older perls on current Debian

[jkeenan]

There are places in this patch where we're "pseudo-patching" ext/Errno/Errno_pm.PL and, in the process are adding trailing whitespace. Do we want to clean that up?

Also, in that same file, could we regularize the leading whitespace (i.e., convert tabs to wordspaces) in the parts that define sub process_file and sub get_file?

[demerphq]

On Fri, 29 Jul 2022 at 23:43, James E Keenan @.***> wrote:

There are places in this patch where we're "pseudo-patching" ext/Errno/Errno_pm.PL and, in the process are adding trailing whitespace. Do we want to clean that up?

Also, in that same file, could we regularize the leading whitespace ( i.e., convert tabs to wordspaces) in the parts that define sub process_file and sub get_file?

FWIW I contributed a PR to add a tool to Porting that would do this automagically for you with a single invocation of the tool:

$ clean-commit

(assuming it is in your path) I use it all the time. It only touches lines you changed, nothing else, and it is aware of which files it should not touch. Any time I have whitespace issues in a patch I use it to fix them, and if I push something with whitespace issues it's only because I forgot to run it.

https://github.com/Perl/perl5/pull/19441

There was no feedback on it however.

cheers, Yves

perl -Mre=debug -e "/just|another|perl|hacker/"

[nwc10]

Sorry for the delay in replying - I wanted to "just" add a bit more, and it took ever longer...

There are places in this patch where we're "pseudo-patching" ext/Errno/Errno_pm.PL and, in the process are adding trailing whitespace. Do we want to clean that up?

Yes, that's a good observation - for these cases, it doesn't matter to the patch application what that whitespace is, and as it's a "fake" patch (I moved it around in the file from the original) rather than a hunk of an original patch, we don't cause problems later if we want to patch it further.

I've fixed this in the rebase.

Also, in that same file, could we regularize the leading whitespace (i.e., convert tabs to wordspaces) in the parts that define sub process_file and sub get_file?

You mean in ext/Errno/Errno_pm.PL itself - yes, we've done this sort of cleanup in the C code - I think it would be fine to have one commit that makes all whitespace "sane" (for some value of sane), which under git diff -w is "no reported changes". I think that github lets us annotate such commits as "whitespace only" (which helps with blame attribution) but I don't think that it's a standard core git feature yet.

But that would be a different PR from this one.

[demerphq]

On Fri, 5 Aug 2022 at 16:38, Nicholas Clark @.***> wrote:

@.**** commented on this pull request.

In Porting/bisect-runner.pl https://github.com/Perl/perl5/pull/20016#discussion_r938883933:

@@ -86,7 +86,24 @@

my ($target, $match) = @options{qw(target match)};

@.*** = ('sh', '-c', 'cd t && ./perl TEST base/*.t') +# El Capitan (OS X 10.11) (and later) strip DYLD_LIBRARY_PATH +# from the environment of /bin/sh +# https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html +# +# (They could have chosen instead to ignore it and pass it through. It would +# have the same direct effect, but maybe needing more coding. I suspect the +# choice to strip it was deliberate, as it will also eliminate a bunch more +# attack vectors, because it prevents you sneaking an override "into" something +# else you convince the user to run.) + +my $agressive_apple_security = "";

D'oh yes!

Well spotted. Thanks. Will fix.

FWIW, I use this:

cat ../changed_text.pl use strict; use warnings;

my $commit_message= git log -1 --pretty="format:%s%n%b"; my $diff= ""; foreach my $line (git log -1 --pretty="format:" -p) { next unless $line=~s/^+//; $diff .= $line; } print $commit_message; print $diff; END

$ git rebase --exec 'perl ../changed_text.pl | hunspell -d en_US -l' origin/blead

to spell-check my patches (code addition/modification and comments) and I try to update

~/.hunspell_en_US

with things from the code that it doesn't know about (eg macros and stuff) so it doesnt complain about common constructs. It finds a lot of false positive stuff at first, but once you get it going it seems to work reasonably well.

cheers, Yves

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[demerphq]

@jkeenan I think @nwc10 is taking some time off the project. Maybe forever.

So I think you should fix the spelling yourself and commit it use git commit -a --amend and then force push the branch.

We as core committers are allowed to do this. Before we had github a committer would have made such a change as a routine part of committing the change and github shouldn't change that. You can add a note to the committ message stating you made a tweak as part of the ammendment process. Git distinguishes between author and committer, so when you --amend the committ it will keep @nwc10's authorship.

[demerphq]

Rebased and fixed the $agressive_apple_security variable.

[demerphq]

@jkeenan can you play around with this branch again and just double check it doesnt break any of your bisect work please?

[jkeenan]

On 2/9/23 21:45, Yves Orton wrote:

@.**** commented on this pull request.

---

In Porting/bisect-runner.pl https://github.com/Perl/perl5/pull/20016#discussion_r1102221508:

@@ -86,7 +86,24 @@

my ($target, $match) = @options{qw(target match)};

@.*** = ('sh', '-c', 'cd t && ./perl TEST base/*.t') +# El Capitan (OS X 10.11) (and later) strip DYLD_LIBRARY_PATH +# from the environment of /bin/sh +# https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html +# +# (They could have chosen instead to ignore it and pass it through. It would +# have the same direct effect, but maybe needing more coding. I suspect the +# choice to strip it was deliberate, as it will also eliminate a bunch more +# attack vectors, because it prevents you sneaking an override "into" something +# else you convince the user to run.) + +my $agressive_apple_security = "";

@jkeenan https://github.com/jkeenan ping. Can you please test the rebased version of this branch?

I am testing it. I spent most of yesterday testing it. But since we never had formal regression tests for this program, I have had to come up with a way to exercise the branch in comparison to what's in blead. I have limited amount of time for this today but hope to complete my study this weekend. Please do not try to rush me.

Thank you very much. Jim Keenan

[demerphq]

Now I know you are on it I'm good, sorry you felt rushed, I just wanted an ack so i could scratch it off my list. Good luck and thanks!

[jkeenan]

Since we've never had any regression tests for Porting/bisect.pl and bisect-runner.pl (or for other utility programs under Porting/), we have to take a different approach for this pull request.

What I did was to take most of the use-cases in the EXAMPLES section which I and others have been adding to the documentation in bisect-runner.pl in recent years and run them against the version of Porting/bisect.pl in the bisect-runner-tweaks branch, and then ask, "Did the bisection point to the same "first bad commit" as we did using the blead version in the past?"

I successfully ran the following bisections:

perl Porting/bisect.pl --target=miniperl \
--start=b9d9f9ab79a4bbe3c10743b2b22828dbb8bd2a46 \
--end=09b83c411bd5dcd2722d0acdd5730c75f926811a \
-e 'q|ace| =~ /c(?=.$)/; print ((($#{^CAPTURE} == -1) ? q|AAA| : q|BBB|), "\n") && exit 0'

perl Porting/bisect.pl-Duseithreads \
--start=6256cf2c --end=f6f85064 --module=XML::Parser

perl Porting/bisect.pl \
--start=0c9b04389335c3a662232102a5a5a570e4e7c403 \
--end=269a7913af37c73e7822a085898f90d15d896882 \
--crash -- ./miniperl -Ilib -e '@N = 1..5;map { pop @N } @N;'

perl Porting/bisect.pl \
--start=f1602e0aeea446456c6f795f1844004479db4de3 \
--end=da9ca395ced485afc455e876f420809dc0e64544 \
-DDEBUGGING --target miniperl --crash -- ./miniperl -Ilib -DXvt -Mstrict -e 1

perl Porting/bisect.pl \
--start=fbb9d44aca632b09ec1e12d53fdd2aedb72e78b2 \
--end=198c4f174582886fed1dd36610fff68fbe836c8e \
-DDEBUGGING --target miniperl --crash \
-- ./miniperl -Ilib -DXv -e '{ my $n=1; *foo= sub () { $n }; }'

export ERR="/tmp/err"
perl Porting/bisect.pl \
--start=507614678018ae1abd55a22e9941778c65741ba3 \
--end=d34b46d077dcfc479c36f65b196086abd7941c76 \
--target=test_prep -e 'chdir("t"); system( "./perl harness ../dist/Tie-File/t/43_synopsis.t 2>$ENV{ERR}"); -s $ENV{ERR} and die "See $ENV{ERR} for warnings thrown";'

perl Porting/bisect.pl --start=v5.27.0 --end=v5.27.1 \
--target=miniperl -e '@a = sort{eval"("}1,2'

perl Porting/bisect.pl --start=v5.37.1 --end=v5.37.2 \
--target=miniperl --expect-fail -e '@a = sort{eval"("}1,2'

perl Porting/bisect.pl --start=5624cfff8f --end=b80b9f7fc6 \
--expect-fail -we 'use Scalar::Util; use bigrat; my @w; local $SIG{__WARN__} = sub { die }; print "mercy\n" if Scalar::Util::looks_like_number(1/9)'

So, at least in these nine cases, this pull request does no harm.

In addition, while most of these bisections were running, I observed that ./Configure was being patched in ways consistently with some of @nwc10's commits in this p.r.

Ideally, we would have gotten somebody working on Darwin to review the Darwin-specific commits in this p.r. (All my testing was done on FreeBSD or Linux.) Ideally, we would have had a case to test on the really old Perl versions much of the p.r. is intended to address. But I suspect we'll only get data about that once this is in blead.

So I recommend merging at this point. However, this is a case where I would not like to squash all the commits because that would make understanding what Nick was doing more difficult. On the other hand, there are 26 commits in this p.r. and, other things being equal, that's 26 commits that future uses of Porting/bisect.pl would have to wade through. Is it possible to merge this in a way that for future bisection instances, they only count as one commit? (I've seen talk on IRC recently that we're considering that approach for feature class and if it's feasible we should use that here as well.)

Thank you very much. Jim Keenan

[demerphq]

Is it possible to merge this in a way that for future bisection instances, they only count as one commit

Absolutely. I will do that.

What I did was to take most of the use-cases in the EXAMPLES section

That is really a lot of work @jkeenan. Thank you. I very much appreciate it. Also, I really do apologize for giving you the sense that I was rushing you. I really just wanted an "ack, on it, I'll get back to you", not to rush you. I regret it came across that way, especially given how much manual work you put into this.

Thanks again. I'll see to it this is merged as you requested here.

[demerphq]

Done and dusted @jkeenan. Thank you once again for your testing.

@nwc10 thanks for the patches, applied!