threads / patch / 58840

patchchainlint.pl: fix /proc/cpuinfo regexp

Subject: [PATCH] chainlint.pl: fix /proc/cpuinfo regexp

## tl;dr

6 messages between Nov 22, 2022 and Nov 23, 2022. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Andreas Hasenack via GitGitGadget· Nov 22, 2022, 20:27 UTC · lore
From: Andreas Hasenack <andreas.hasenack@canonical.com>

git commit 29fb2ec384a867ca577335a12f4b45c184e7b642 introduced a function that gets the number of cores from /proc/cpuinfo on some systems, notably linux.

The regexp it uses (^processor\s*:) fails to match the desired lines in the s390x architecture, where they look like this:

processor 0: version = FF, identification = 148F67, machine = 2964

As a result, on s390x that function returns 0 as the number of cores, and the chainlint.pl script exits without doing anything.

Signed-off-by: Andreas Hasenack <andreas.hasenack@canonical.com>
---
    chainlint.pl: fix /proc/cpuinfo regexp
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1385%2Fpanlinux%2Fupstream-fix-cpuinfo-regexp-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1385/panlinux/upstream-fix-cpuinfo-regexp-v1
Pull-Request: https://github.com/git/git/pull/1385
 t/chainlint.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/chainlint.pl +1 −1
diff --git a/t/chainlint.pl b/t/chainlint.pl
index 976db4b8a01..31cc086f964 100755
--- a/t/chainlint.pl
+++ b/t/chainlint.pl
@@ -656,7 +656,7 @@ sub ncores {
 	# Windows
 	return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
 	# Linux / MSYS2 / Cygwin / WSL
-	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor\s*:/, <>)); } if -r '/proc/cpuinfo';
+	do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
 	# macOS & BSD
 	return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
 	return 1;

base-commit: a0789512c5a4ae7da935cd2e419f253cb3cb4ce7

-- 
gitgitgadget
Eric Sunshine· Nov 22, 2022, 20:57 UTC · re: Andreas Hasenack via GitGitGadget · lore

Re: [PATCH] chainlint.pl: fix /proc/cpuinfo regexp

On Tue, Nov 22, 2022 at 3:29 PM Andreas Hasenack via GitGitGadget <gitgitgadget@gmail.com> wrote:

> git commit 29fb2ec384a867ca577335a12f4b45c184e7b642 introduced a
> function that gets the number of cores from /proc/cpuinfo on some
> systems, notably linux.
Thanks for diagnosing this problem and quickly submitting a patch.
I forgot to mention earlier that we typically cite other commits like this:
    29fb2ec384 (chainlint.pl: validate test scripts in parallel, 2022-09-01)
which you can generate automatically with:
    git log --pretty=reference -1 29fb2ec384
Show 7 quoted lines
> The regexp it uses (^processor\s*:) fails to match the desired lines in
> the s390x architecture, where they look like this:
>
> processor 0: version = FF, identification = 148F67, machine = 2964
>
> As a result, on s390x that function returns 0 as the number of cores,
> and the chainlint.pl script exits without doing anything.
Makes sense. Well explained.

A separate problem is that chainlint.pl doesn't fall back to a sensible non-zero value if ncores() returns 0 (or some other nonsense value). That is, of course, outside the scope of the well-focused problem fix which this standalone patch addresses. I may end up submitting a fix separately to make it fall back sensibly.

Show 12 quoted lines
> Signed-off-by: Andreas Hasenack <andreas.hasenack@canonical.com>
> ---
> diff --git a/t/chainlint.pl b/t/chainlint.pl
> @@ -656,7 +656,7 @@ sub ncores {
>         # Windows
>         return $ENV{NUMBER_OF_PROCESSORS} if exists($ENV{NUMBER_OF_PROCESSORS});
>         # Linux / MSYS2 / Cygwin / WSL
> -       do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor\s*:/, <>)); } if -r '/proc/cpuinfo';
> +       do { local @ARGV='/proc/cpuinfo'; return scalar(grep(/^processor[\s\d]*:/, <>)); } if -r '/proc/cpuinfo';
>         # macOS & BSD
>         return qx/sysctl -n hw.ncpu/ if $^O =~ /(?:^darwin$|bsd)/;
>         return 1;

As mentioned elsewhere[1], this code may eventually be dropped altogether, but this fix is good to have in the meantime. Thanks.

[1]: https://lore.kernel.org/git/CAPig+cQrXb-YUSzmfgJ2PRoiOP3goVACRCrX9C39kf3oDH+BHg@mail.gmail.com/
Junio C Hamano· Nov 23, 2022, 01:21 UTC · re: Eric Sunshine · lore

Re: [PATCH] chainlint.pl: fix /proc/cpuinfo regexp

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 10 quoted lines
> Makes sense. Well explained.
>
> A separate problem is that chainlint.pl doesn't fall back to a
> sensible non-zero value if ncores() returns 0 (or some other nonsense
> value). That is, of course, outside the scope of the well-focused
> problem fix which this standalone patch addresses. I may end up
> submitting a fix separately to make it fall back sensibly.
> ...
> As mentioned elsewhere[1], this code may eventually be dropped
> altogether, but this fix is good to have in the meantime. Thanks.
Thanks, both.  Will apply.
Andreas Hasenack· Nov 23, 2022, 12:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] chainlint.pl: fix /proc/cpuinfo regexp

Should I still change the commit message regarding where I mention the commit that introduced this, as explained by Eric? From the activity I saw overnight it looks like things were already picked up and it doesn't matter anymore.

On Tue, Nov 22, 2022 at 10:21 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > Makes sense. Well explained.
> >
> > A separate problem is that chainlint.pl doesn't fall back to a
> > sensible non-zero value if ncores() returns 0 (or some other nonsense
> > value). That is, of course, outside the scope of the well-focused
> > problem fix which this standalone patch addresses. I may end up
> > submitting a fix separately to make it fall back sensibly.
> > ...
> > As mentioned elsewhere[1], this code may eventually be dropped
> > altogether, but this fix is good to have in the meantime. Thanks.
>
> Thanks, both.  Will apply.
Eric Sunshine· Nov 23, 2022, 15:52 UTC · re: Andreas Hasenack · lore

Re: [PATCH] chainlint.pl: fix /proc/cpuinfo regexp

On Wed, Nov 23, 2022 at 7:17 AM Andreas Hasenack <andreas@canonical.com> wrote:
> Should I still change the commit message regarding where I mention the
> commit that introduced this, as explained by Eric? From the activity I
> saw overnight it looks like things were already picked up and it
> doesn't matter anymore.

No need to resend. Junio tweaked the commit message[1] when he picked up the patch.

[1]: https://github.com/git/git/commit/1f51b77f4f3f4d05f249f80713b2969e4069c109
Junio C Hamano· Nov 23, 2022, 23:53 UTC · re: Andreas Hasenack · lore

Re: [PATCH] chainlint.pl: fix /proc/cpuinfo regexp

Andreas Hasenack <andreas@canonical.com> writes:
> Should I still change the commit message regarding where I mention the
> commit that introduced this, as explained by Eric? From the activity I
> saw overnight it looks like things were already picked up and it
> doesn't matter anymore.

Sorry, I should have said "will apply with tweaked log message" to reduce the need for a round-trip. Please double check what is queued in 'seen' and send in replacements before it gets merged down to 'next' if there are still things you may want to update. If you are happy with what is in 'seen', no further action is needed at least for now.

Thanks.

← back to recent threads