Bisect runner tweaks #20016
Bisect runner tweaks #20016
Conversation
|
There are places in this patch where we're "pseudo-patching" Also, in that same file, could we regularize the leading whitespace (i.e., convert tabs to wordspaces) in the parts that define |
|
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.
#19441
There was no feedback on it however.
cheers,
Yves
--
perl -Mre=debug -e "/just|another|perl|hacker/"
|
nwc10
force-pushed
the
bisect-runner-tweaks
branch
from
8763087
to
fc41e15
Compare
|
Sorry for the delay in replying - I wanted to "just" add a bit more, and it took ever longer...
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.
You mean in But that would be a different PR from this one. |
There was a problem hiding this comment.
Most of your proposed revisions deal with the parts of bisect-runner.pl which patch fundamental files in the core distribution and which are beyond my current expertise. So I'm not the best person to comment on those parts of your p.r. Also, though I am a heavy user of Porting/bisect.pl, I rarely have to go as far back in our history as these revisions are concerned with. But apart from the spelling of one variable this LGTM. Thanks for your research.
|
On Fri, 5 Aug 2022 at 16:38, Nicholas Clark ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Porting/bisect-runner.pl
<#20016 (comment)>:
> @@ -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/"
|
|
@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 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
force-pushed
the
bisect-runner-tweaks
branch
from
fc41e15
to
d9b9993
Compare
|
Rebased and fixed the |
|
@jkeenan can you play around with this branch again and just double check it doesnt break any of your bisect work please? |
|
On 2/9/23 21:45, Yves Orton wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In Porting/bisect-runner.pl
<#20016 (comment)>:
> @@ -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
|
|
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! |
|
Since we've never had any regression tests for What I did was to take most of the use-cases in the I successfully ran the following bisections: 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 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 Thank you very much. |
Absolutely. I will do that.
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
force-pushed
the
bisect-runner-tweaks
branch
from
d9b9993
to
a94b325
Compare
Without this fix, the generated Errno.pm will contain no entries, which whilst syntactically valid causes much confusion later on when the bisect run gives bogus results due to non-buggy module code unrelated to the test case failing because %! is wrong.
Without this fix, Errno.pm fails to build.
Else they are implicitly assumed to return int, which can truncate addresses on systems where pointers are larger than ints (such as 64 bit systems).
Strictly we only need this for glibc systems, but it doesn't seem terrible to do it everywhere.
Previously it would skip, which meant you couldn't bisect the cause of some ./Configure failures, which rather defeats the intent of --test-build.
The version number moved beyond 10, and older hints files were not ready for this.
Configure can get stuck and ask questions for which it needs a valid answer before it can continue. As-is, if you redirect stdin from /dev/null (or close the file descriptor) it will (effectively) loop infinitely repeating the same question, because it doesn't like empty string as an answer. Worse - it keeps repeating the question to stdout - eg 'Where is your C library?' Rather than attempting to patch the shell script to detect errors on read (because they only matter the *second* time round the loop, *and* wouldn't handle the /dev/null case), it's easier to patch the relevant loop so that it will abort after too many loop iterations.
This only affects a small range of commits in development releases, but without this change they can loop infinitely, rather than correctly skipping (or failing a build test). An infinite loop (with terminal output) is extremely unhelpful.
…Util This problem was rapidly diagnosed and fixed at the time, but we need to fix the few commits where the problem exists, else we can't bisect other build failures.
This failure gets in the way of bisecting other problems.
Earlier versions of the hints defaulted to using nm, because on older versions of OS X (as was) it worked. It also needs to patch the hints file to force d_stdstdio to "undef". Without this, a build with "d_faststdio" defined (or implicit) will fail badly on current macOS, such as the perl-5.8.0 tag.
The older version assumed an explicit prototype for printf(), which doesn't
fly on arm64 macOS. It might not be robust on some other platforms too,
whereas the "current" (ie 2003 onward) approach still works everywhere.
Change edit_file() to only localise $/ to undef during the read, so that it's
restored to its default ("\n") when the callback is invoked. Without this,
`chomp` "doesn't work" (as expected) in the callback.
Without these various probes rely on implicit function declarations, typically for exit() or printf(). macOS now forbids implicit function declarations, which causes these probes to become compile time errors and hence "fail". This results in Configure assuming that many symbols are missing, and the build fails where it should pass.
Else it can't build 5.003 or 5.002 on macOS.
The hints for macOS set -flat_namespace conditionally based on darwin version, so that newer OSes would default to the native two level namespace. However, the build of miniperl was relying on a flat namespace prior to a refactoring during the 5.9.x series. Hence we need to force this linker flag when building versions before this on current macOS versions.
El Capitan (OS X 10.11) (and later) strip DYLD_LIBRARY_PATH from the environment of /bin/sh, hence setting the existing code that sets this in %ENV assuming that it is visible to the invoked process no longer works. We have to be explicit in every invocation, as part of the command that the shell itself is processing. This hurts us because in 5.8.0 and earlier the hints default macOS to build a shared perl library.
Without this some early versions of DB_File won't build on current macOS, and any other platform where the C compiler is agressive about prototypes. This commit refactors code for an existing DB_File patch to split a compound if statement into two ifs.
Else some development versions of 5.003 mysteriously won't build. These versions aren't important in themselves, but their failure makes it hard to bisect real problems.
These fallback functions are defined in util.c, but initially did not have any prototypes in a header.
The C code needs prototypes from these headers.
The regex wasn't handling context diffs, and two of the unified diffs were generated with differing filenames and trailing timestamp text that it wasn't robust against.
*Really* early xsubpp doesn't understand this, and it turns out to be trivial to eliminate it.
This was missing prior to perl-5.001
demerphq
force-pushed
the
bisect-runner-tweaks
branch
from
a94b325
to
eb2a178
Compare
Patches to enable bisect.pl to build older perls on current Debian