git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* chainlink.pl /proc/cpuinfo regexp fails on s390x
@ 2022-11-22 17:37 Andreas Hasenack
  2022-11-22 17:57 ` Eric Sunshine
  2022-11-22 23:30 ` Andreas Schwab
  0 siblings, 2 replies; 10+ messages in thread
From: Andreas Hasenack @ 2022-11-22 17:37 UTC (permalink / raw)
  To: git

Hi,

(please CC me on replies, as I'm not subscribed)

git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in
2.38.0 and later, introduced a function that gets the number of cores
from /proc/cpuinfo. It essentially does this on linux:

    do { local @ARGV='/proc/cpuinfo'; return
scalar(grep(/^processor\s*:/, <>)); } if -r '/proc/cpuinfo';

On s390x, the ^processor lines are like this:

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

In other arches (I checked amd64, armhf and arm64), they are like this instead:

processor : 0

As a result, that function is returning 0 on s390x, and that value is
used for the number of jobs the script should execute. Since it's
zero, it exits without doing anything, and that breaks the test and
the build[3] on s390x.

This is trivial and I don't think a PR is necessary, but let me know
if you want one.

That regexp could perhaps be:

/^processor\s*\d*\s*:/

or

/^processor[\s\d]*:/

or something else.


1. https://github.com/git/git/commit/29fb2ec384a867ca577335a12f4b45c184e7b642
2. https://github.com/git/git/commit/29fb2ec384a867ca577335a12f4b45c184e7b642#diff-e7042d714d4be11a06d153e6f2daeb3c3a9766b972522baab8ba113b962086cfR574
3. https://launchpadlibrarian.net/635348769/buildlog_ubuntu-lunar-s390x.git_1%3A2.38.1-1ubuntu1_BUILDING.txt.gz

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 17:37 chainlink.pl /proc/cpuinfo regexp fails on s390x Andreas Hasenack
@ 2022-11-22 17:57 ` Eric Sunshine
  2022-11-22 18:04   ` Andreas Hasenack
  2022-11-22 23:30 ` Andreas Schwab
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Sunshine @ 2022-11-22 17:57 UTC (permalink / raw)
  To: Andreas Hasenack; +Cc: git

On Tue, Nov 22, 2022 at 12:45 PM Andreas Hasenack <andreas@canonical.com> wrote:
> git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in
> 2.38.0 and later, introduced a function that gets the number of cores
> from /proc/cpuinfo. It essentially does this on linux:
>
>     do { local @ARGV='/proc/cpuinfo'; return
> scalar(grep(/^processor\s*:/, <>)); } if -r '/proc/cpuinfo';
>
> On s390x, the ^processor lines are like this:
>
> processor 0: version = FF, identification = 148F67, machine = 2964
>
> In other arches (I checked amd64, armhf and arm64), they are like this instead:
>
> processor : 0
>
> As a result, that function is returning 0 on s390x, and that value is
> used for the number of jobs the script should execute. Since it's
> zero, it exits without doing anything, and that breaks the test and
> the build[3] on s390x.

Thanks for the well-described explanation. I think this is the same
problem reported in [1], however, that thread hasn't seen sufficient
diagnostic information yet to figure out what was going on. So, your
report is very helpful.

There is discussion about altogether ditching the code in question
(see the discussion starting at about [2], for instance), but in the
meantime, fixing this would be a good idea.

> This is trivial and I don't think a PR is necessary, but let me know
> if you want one.
>
> That regexp could perhaps be:
>
> /^processor\s*\d*\s*:/
>
> or
>
> /^processor[\s\d]*:/
>
> or something else.

We would be happy to take a patch if you're interested in submitting
one. Otherwise, I can submit a patch to fix this case. Let me know
your preference.

[1]: https://lore.kernel.org/git/f1a5f758-d81f-5985-9b5d-2f0dbfaac071@opensuse.org/
[2]: https://lore.kernel.org/git/221122.86cz9fbyln.gmgdl@evledraar.gmail.com/T/#m7626ed18fa0e93a8b0ada9cf6d2d18b08045cb4b

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 17:57 ` Eric Sunshine
@ 2022-11-22 18:04   ` Andreas Hasenack
  2022-11-22 18:13     ` Eric Sunshine
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Hasenack @ 2022-11-22 18:04 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git

Hi Eric,

On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Nov 22, 2022 at 12:45 PM Andreas Hasenack <andreas@canonical.com> wrote:
> > git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in

(...)

> We would be happy to take a patch if you're interested in submitting
> one. Otherwise, I can submit a patch to fix this case. Let me know
> your preference.

Can that be a simple PR in https://github.com/git/git/pulls?

Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 18:04   ` Andreas Hasenack
@ 2022-11-22 18:13     ` Eric Sunshine
  2022-11-22 18:42       ` Andreas Hasenack
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Sunshine @ 2022-11-22 18:13 UTC (permalink / raw)
  To: Andreas Hasenack; +Cc: git

On Tue, Nov 22, 2022 at 1:04 PM Andreas Hasenack <andreas@canonical.com> wrote:
> On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > We would be happy to take a patch if you're interested in submitting
> > one. Otherwise, I can submit a patch to fix this case. Let me know
> > your preference.
>
> Can that be a simple PR in https://github.com/git/git/pulls?

The project doesn't take pull requests directly, but GitGitGadget[1]
will convert a pull request into a mailing list patch. It looks like
https://github.com/git/git/ is one of the repositories with which
GitGitGadget works, so presumably it should work. You could probably
come up with a well-written commit message by paraphrasing your bug
report.

[1]: https://gitgitgadget.github.io/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 18:13     ` Eric Sunshine
@ 2022-11-22 18:42       ` Andreas Hasenack
  2022-11-22 19:39         ` Eric Sunshine
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Hasenack @ 2022-11-22 18:42 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: git

Hi,

On Tue, Nov 22, 2022 at 3:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Tue, Nov 22, 2022 at 1:04 PM Andreas Hasenack <andreas@canonical.com> wrote:
> > On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > We would be happy to take a patch if you're interested in submitting
> > > one. Otherwise, I can submit a patch to fix this case. Let me know
> > > your preference.
> >
> > Can that be a simple PR in https://github.com/git/git/pulls?
>
> The project doesn't take pull requests directly, but GitGitGadget[1]
> will convert a pull request into a mailing list patch. It looks like
> https://github.com/git/git/ is one of the repositories with which
> GitGitGadget works, so presumably it should work. You could probably
> come up with a well-written commit message by paraphrasing your bug
> report.
>
> [1]: https://gitgitgadget.github.io/

Ok, let's see how this goes. I opened a PR in
https://github.com/git/git/pull/1385 and if someone could add the
required /allow, I can take the next steps.

Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 18:42       ` Andreas Hasenack
@ 2022-11-22 19:39         ` Eric Sunshine
  0 siblings, 0 replies; 10+ messages in thread
From: Eric Sunshine @ 2022-11-22 19:39 UTC (permalink / raw)
  To: Andreas Hasenack; +Cc: git

On Tue, Nov 22, 2022 at 1:42 PM Andreas Hasenack <andreas@canonical.com> wrote:
> On Tue, Nov 22, 2022 at 3:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > The project doesn't take pull requests directly, but GitGitGadget[1]
> > will convert a pull request into a mailing list patch. It looks like
> > https://github.com/git/git/ is one of the repositories with which
> > GitGitGadget works, so presumably it should work. You could probably
> > come up with a well-written commit message by paraphrasing your bug
> > report.
>
> Ok, let's see how this goes. I opened a PR in
> https://github.com/git/git/pull/1385 and if someone could add the
> required /allow, I can take the next steps.

It looks like Dscho already /allow'd you, but GitGitGadget is
complaining that you don't have a public email address on file with
GitHub, which GitGitGadget requires so mailing list replies can make
it back to you. Once you correct that, you should be able to /submit.

The patch itself and the commit message look fine.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 17:37 chainlink.pl /proc/cpuinfo regexp fails on s390x Andreas Hasenack
  2022-11-22 17:57 ` Eric Sunshine
@ 2022-11-22 23:30 ` Andreas Schwab
  2022-11-22 23:50   ` Eric Sunshine
  1 sibling, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2022-11-22 23:30 UTC (permalink / raw)
  To: Andreas Hasenack; +Cc: git

On Nov 22 2022, Andreas Hasenack wrote:

> That regexp could perhaps be:
>
> /^processor\s*\d*\s*:/
>
> or
>
> /^processor[\s\d]*:/
>
> or something else.

Something else.

$ getconf _NPROCESSORS_ONLN

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 23:30 ` Andreas Schwab
@ 2022-11-22 23:50   ` Eric Sunshine
  2022-11-23  9:27     ` Andreas Schwab
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Sunshine @ 2022-11-22 23:50 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Andreas Hasenack, git

On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Nov 22 2022, Andreas Hasenack wrote:
> > /^processor[\s\d]*:/
> > or something else.
>
> Something else.
>
> $ getconf _NPROCESSORS_ONLN

Yes, that works too and was (I'm pretty sure) considered during
development. The reason /proc/cpu was chosen over `getconf` was that
opening & reading /proc/cpu should be faster since it doesn't involve
spawning a process. If we did use `getconf`, we'd have to be careful
to degrade gracefully if `getconf` isn't available or if the
configuration parameter (i.e. "_NPROCESSORS_ONLN") isn't known on the
platform.

That said, the submitted patch[1] is probably good enough for now,
especially since there's talk about possibly removing this code
entirely.

[1]: https://lore.kernel.org/git/pull.1385.git.git.1669148861635.gitgitgadget@gmail.com/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-22 23:50   ` Eric Sunshine
@ 2022-11-23  9:27     ` Andreas Schwab
  2022-11-23 19:10       ` Eric Sunshine
  0 siblings, 1 reply; 10+ messages in thread
From: Andreas Schwab @ 2022-11-23  9:27 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Andreas Hasenack, git

On Nov 22 2022, Eric Sunshine wrote:

> On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>> On Nov 22 2022, Andreas Hasenack wrote:
>> > /^processor[\s\d]*:/
>> > or something else.
>>
>> Something else.
>>
>> $ getconf _NPROCESSORS_ONLN
>
> Yes, that works too and was (I'm pretty sure) considered during
> development. The reason /proc/cpu was chosen over `getconf` was that
> opening & reading /proc/cpu should be faster since it doesn't involve
> spawning a process. If we did use `getconf`, we'd have to be careful
> to degrade gracefully if `getconf` isn't available or if the
> configuration parameter (i.e. "_NPROCESSORS_ONLN") isn't known on the
> platform.

getconf is surely more portable than poking in /proc, especially
/proc/cpuinfo is the antipode of portability.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: chainlink.pl /proc/cpuinfo regexp fails on s390x
  2022-11-23  9:27     ` Andreas Schwab
@ 2022-11-23 19:10       ` Eric Sunshine
  0 siblings, 0 replies; 10+ messages in thread
From: Eric Sunshine @ 2022-11-23 19:10 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Andreas Hasenack, git

On Wed, Nov 23, 2022 at 4:27 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Nov 22 2022, Eric Sunshine wrote:
> > On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >> On Nov 22 2022, Andreas Hasenack wrote:
> >> > /^processor[\s\d]*:/
> >> > or something else.
> >>
> >> Something else.
> >> $ getconf _NPROCESSORS_ONLN
> >
> > Yes, that works too and was (I'm pretty sure) considered during
> > development. The reason /proc/cpu was chosen over `getconf` was that
> > opening & reading /proc/cpu should be faster since it doesn't involve
> > spawning a process. If we did use `getconf`, we'd have to be careful
> > to degrade gracefully if `getconf` isn't available or if the
> > configuration parameter (i.e. "_NPROCESSORS_ONLN") isn't known on the
> > platform.
>
> getconf is surely more portable than poking in /proc, especially
> /proc/cpuinfo is the antipode of portability.

No doubt, but for the immediate issue, this tightly-focused fix is
more appropriate and less likely to lead to unexpected additional
problems. I'm not arguing against `getconf`, but saying only that
_this_ patch is fine as-is.

It may very well be a good idea to replace the /proc/cpuinfo probe by
`getconf` in the future (or remove it altogether as mentioned
upstream), but that's outside the scope of this patch and the
immediate problem.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-11-23 19:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-22 17:37 chainlink.pl /proc/cpuinfo regexp fails on s390x Andreas Hasenack
2022-11-22 17:57 ` Eric Sunshine
2022-11-22 18:04   ` Andreas Hasenack
2022-11-22 18:13     ` Eric Sunshine
2022-11-22 18:42       ` Andreas Hasenack
2022-11-22 19:39         ` Eric Sunshine
2022-11-22 23:30 ` Andreas Schwab
2022-11-22 23:50   ` Eric Sunshine
2022-11-23  9:27     ` Andreas Schwab
2022-11-23 19:10       ` Eric Sunshine

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).