{"thread":{"id":"58838","subject":"chainlink.pl /proc/cpuinfo regexp fails on s390x","startedAt":"2022-11-22T17:37:59Z","lastAt":"2022-11-23T19:11:19Z","messageCount":10,"participants":["Andreas Hasenack","Eric Sunshine","Andreas Schwab"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"467779","messageId":"CANYNYEHXU8ivgAOa8EO5e9kOcbu6XF7rj+9EcSrbDQE+Rvyw_Q@mail.gmail.com","threadId":"58838","inReplyTo":null,"subject":"chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Andreas Hasenack","fromEmail":"andreas@canonical.com","sentAt":"2022-11-22T17:37:38Z","receivedAt":"2022-11-22T17:37:59Z","isPatch":false,"sender":{"key":"andreas@canonical.com","avatar":"https://gravatar.com/avatar/184265ba468b2b41263e8e984908a651fc48b2b1fb3842090b93ca91a69630de?d=mp&s=160"},"body":"Hi,\n\n(please CC me on replies, as I'm not subscribed)\n\ngit commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in\n2.38.0 and later, introduced a function that gets the number of cores\nfrom /proc/cpuinfo. It essentially does this on linux:\n\n do { local @ARGV='/proc/cpuinfo'; return\nscalar(grep(/^processor\\s*:/, <>)); } if -r '/proc/cpuinfo';\n\nOn s390x, the ^processor lines are like this:\n\nprocessor 0: version = FF, identification = 148F67, machine = 2964\n\nIn other arches (I checked amd64, armhf and arm64), they are like this instead:\n\nprocessor : 0\n\nAs a result, that function is returning 0 on s390x, and that value is\nused for the number of jobs the script should execute. Since it's\nzero, it exits without doing anything, and that breaks the test and\nthe build[3] on s390x.\n\nThis is trivial and I don't think a PR is necessary, but let me know\nif you want one.\n\nThat regexp could perhaps be:\n\n/^processor\\s*\\d*\\s*:/\n\nor\n\n/^processor[\\s\\d]*:/\n\nor something else.\n\n\n1. https://github.com/git/git/commit/29fb2ec384a867ca577335a12f4b45c184e7b642\n2. https://github.com/git/git/commit/29fb2ec384a867ca577335a12f4b45c184e7b642#diff-e7042d714d4be11a06d153e6f2daeb3c3a9766b972522baab8ba113b962086cfR574\n3. https://launchpadlibrarian.net/635348769/buildlog_ubuntu-lunar-s390x.git_1%3A2.38.1-1ubuntu1_BUILDING.txt.gz\n"},{"id":"467785","messageId":"CAPig+cQrXb-YUSzmfgJ2PRoiOP3goVACRCrX9C39kf3oDH+BHg@mail.gmail.com","threadId":"58838","inReplyTo":"CANYNYEHXU8ivgAOa8EO5e9kOcbu6XF7rj+9EcSrbDQE+Rvyw_Q@mail.gmail.com","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-22T17:57:21Z","receivedAt":"2022-11-22T17:58:45Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 22, 2022 at 12:45 PM Andreas Hasenack wrote:\n> git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in\n> 2.38.0 and later, introduced a function that gets the number of cores\n> from /proc/cpuinfo. It essentially does this on linux:\n>\n> do { local @ARGV='/proc/cpuinfo'; return\n> scalar(grep(/^processor\\s*:/, <>)); } if -r '/proc/cpuinfo';\n>\n> On s390x, the ^processor lines are like this:\n>\n> processor 0: version = FF, identification = 148F67, machine = 2964\n>\n> In other arches (I checked amd64, armhf and arm64), they are like this instead:\n>\n> processor : 0\n>\n> As a result, that function is returning 0 on s390x, and that value is\n> used for the number of jobs the script should execute. Since it's\n> zero, it exits without doing anything, and that breaks the test and\n> the build[3] on s390x.\n\nThanks for the well-described explanation. I think this is the same\nproblem reported in [1], however, that thread hasn't seen sufficient\ndiagnostic information yet to figure out what was going on. So, your\nreport is very helpful.\n\nThere is discussion about altogether ditching the code in question\n(see the discussion starting at about [2], for instance), but in the\nmeantime, fixing this would be a good idea.\n\n> This is trivial and I don't think a PR is necessary, but let me know\n> if you want one.\n>\n> That regexp could perhaps be:\n>\n> /^processor\\s*\\d*\\s*:/\n>\n> or\n>\n> /^processor[\\s\\d]*:/\n>\n> or something else.\n\nWe would be happy to take a patch if you're interested in submitting\none. Otherwise, I can submit a patch to fix this case. Let me know\nyour preference.\n\n[1]: https://lore.kernel.org/git/f1a5f758-d81f-5985-9b5d-2f0dbfaac071@opensuse.org/\n[2]: https://lore.kernel.org/git/221122.86cz9fbyln.gmgdl@evledraar.gmail.com/T/#m7626ed18fa0e93a8b0ada9cf6d2d18b08045cb4b\n"},{"id":"467787","messageId":"CANYNYEF+Gsas5s7u3rb3CQeFPL1MoCTweA4e3L90vCD0rPsNgg@mail.gmail.com","threadId":"58838","inReplyTo":"CAPig+cQrXb-YUSzmfgJ2PRoiOP3goVACRCrX9C39kf3oDH+BHg@mail.gmail.com","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Andreas Hasenack","fromEmail":"andreas@canonical.com","sentAt":"2022-11-22T18:04:26Z","receivedAt":"2022-11-22T18:04:45Z","isPatch":false,"sender":{"key":"andreas@canonical.com","avatar":"https://gravatar.com/avatar/184265ba468b2b41263e8e984908a651fc48b2b1fb3842090b93ca91a69630de?d=mp&s=160"},"body":"Hi Eric,\n\nOn Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine wrote:\n>\n> On Tue, Nov 22, 2022 at 12:45 PM Andreas Hasenack wrote:\n> > git commit 29fb2ec384a867ca577335a12f4b45c184e7b642[1], present in\n\n(...)\n\n> We would be happy to take a patch if you're interested in submitting\n> one. Otherwise, I can submit a patch to fix this case. Let me know\n> your preference.\n\nCan that be a simple PR in https://github.com/git/git/pulls?\n\nThanks!\n"},{"id":"467789","messageId":"CAPig+cS-BWJoWgo3UEk0X6fRjsysR0_23ppn9WX02Gy+ugVdOQ@mail.gmail.com","threadId":"58838","inReplyTo":"CANYNYEF+Gsas5s7u3rb3CQeFPL1MoCTweA4e3L90vCD0rPsNgg@mail.gmail.com","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-22T18:13:52Z","receivedAt":"2022-11-22T18:14:08Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 22, 2022 at 1:04 PM Andreas Hasenack wrote:\n> On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine wrote:\n> > We would be happy to take a patch if you're interested in submitting\n> > one. Otherwise, I can submit a patch to fix this case. Let me know\n> > your preference.\n>\n> Can that be a simple PR in https://github.com/git/git/pulls?\n\nThe project doesn't take pull requests directly, but GitGitGadget[1]\nwill convert a pull request into a mailing list patch. It looks like\nhttps://github.com/git/git/ is one of the repositories with which\nGitGitGadget works, so presumably it should work. You could probably\ncome up with a well-written commit message by paraphrasing your bug\nreport.\n\n[1]: https://gitgitgadget.github.io/\n"},{"id":"467796","messageId":"CANYNYEEPZjGEogaSqqCO_qnzLqnrd0d0+z-Nv7v7GhKj_M-x-g@mail.gmail.com","threadId":"58838","inReplyTo":"CAPig+cS-BWJoWgo3UEk0X6fRjsysR0_23ppn9WX02Gy+ugVdOQ@mail.gmail.com","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Andreas Hasenack","fromEmail":"andreas@canonical.com","sentAt":"2022-11-22T18:42:09Z","receivedAt":"2022-11-22T18:42:33Z","isPatch":false,"sender":{"key":"andreas@canonical.com","avatar":"https://gravatar.com/avatar/184265ba468b2b41263e8e984908a651fc48b2b1fb3842090b93ca91a69630de?d=mp&s=160"},"body":"Hi,\n\nOn Tue, Nov 22, 2022 at 3:14 PM Eric Sunshine wrote:\n>\n> On Tue, Nov 22, 2022 at 1:04 PM Andreas Hasenack wrote:\n> > On Tue, Nov 22, 2022 at 2:57 PM Eric Sunshine wrote:\n> > > We would be happy to take a patch if you're interested in submitting\n> > > one. Otherwise, I can submit a patch to fix this case. Let me know\n> > > your preference.\n> >\n> > Can that be a simple PR in https://github.com/git/git/pulls?\n>\n> The project doesn't take pull requests directly, but GitGitGadget[1]\n> will convert a pull request into a mailing list patch. It looks like\n> https://github.com/git/git/ is one of the repositories with which\n> GitGitGadget works, so presumably it should work. You could probably\n> come up with a well-written commit message by paraphrasing your bug\n> report.\n>\n> [1]: https://gitgitgadget.github.io/\n\nOk, let's see how this goes. I opened a PR in\nhttps://github.com/git/git/pull/1385 and if someone could add the\nrequired /allow, I can take the next steps.\n\nThanks!\n"},{"id":"467803","messageId":"CAPig+cQwxOr7udp4Zg=8dHycRDJrLFMZe1HKm5s5Hp=HSjuOcg@mail.gmail.com","threadId":"58838","inReplyTo":"CANYNYEEPZjGEogaSqqCO_qnzLqnrd0d0+z-Nv7v7GhKj_M-x-g@mail.gmail.com","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-22T19:39:55Z","receivedAt":"2022-11-22T19:40:12Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 22, 2022 at 1:42 PM Andreas Hasenack wrote:\n> On Tue, Nov 22, 2022 at 3:14 PM Eric Sunshine wrote:\n> > The project doesn't take pull requests directly, but GitGitGadget[1]\n> > will convert a pull request into a mailing list patch. It looks like\n> > https://github.com/git/git/ is one of the repositories with which\n> > GitGitGadget works, so presumably it should work. You could probably\n> > come up with a well-written commit message by paraphrasing your bug\n> > report.\n>\n> Ok, let's see how this goes. I opened a PR in\n> https://github.com/git/git/pull/1385 and if someone could add the\n> required /allow, I can take the next steps.\n\nIt looks like Dscho already /allow'd you, but GitGitGadget is\ncomplaining that you don't have a public email address on file with\nGitHub, which GitGitGadget requires so mailing list replies can make\nit back to you. Once you correct that, you should be able to /submit.\n\nThe patch itself and the commit message look fine.\n"},{"id":"467828","messageId":"87v8n6o9du.fsf@igel.home","threadId":"58838","inReplyTo":"CANYNYEHXU8ivgAOa8EO5e9kOcbu6XF7rj+9EcSrbDQE+Rvyw_Q@mail.gmail.com","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2022-11-22T23:30:05Z","receivedAt":"2022-11-22T23:30:15Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 22 2022, Andreas Hasenack wrote:\n\n> That regexp could perhaps be:\n>\n> /^processor\\s*\\d*\\s*:/\n>\n> or\n>\n> /^processor[\\s\\d]*:/\n>\n> or something else.\n\nSomething else.\n\n$ getconf _NPROCESSORS_ONLN\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"467830","messageId":"CAPig+cRchF_6t-B-cZHLHAH_E6qNmapTezSfCj9AZ8cfWNzivw@mail.gmail.com","threadId":"58838","inReplyTo":"87v8n6o9du.fsf@igel.home","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-22T23:50:26Z","receivedAt":"2022-11-22T23:50:41Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab wrote:\n> On Nov 22 2022, Andreas Hasenack wrote:\n> > /^processor[\\s\\d]*:/\n> > or something else.\n>\n> Something else.\n>\n> $ getconf _NPROCESSORS_ONLN\n\nYes, that works too and was (I'm pretty sure) considered during\ndevelopment. The reason /proc/cpu was chosen over `getconf` was that\nopening & reading /proc/cpu should be faster since it doesn't involve\nspawning a process. If we did use `getconf`, we'd have to be careful\nto degrade gracefully if `getconf` isn't available or if the\nconfiguration parameter (i.e. \"_NPROCESSORS_ONLN\") isn't known on the\nplatform.\n\nThat said, the submitted patch[1] is probably good enough for now,\nespecially since there's talk about possibly removing this code\nentirely.\n\n[1]: https://lore.kernel.org/git/pull.1385.git.git.1669148861635.gitgitgadget@gmail.com/\n"},{"id":"467860","messageId":"mvm1qpu9g2e.fsf@linux-m68k.org","threadId":"58838","inReplyTo":"CAPig+cRchF_6t-B-cZHLHAH_E6qNmapTezSfCj9AZ8cfWNzivw@mail.gmail.com","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2022-11-23T09:27:05Z","receivedAt":"2022-11-23T09:35:48Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 22 2022, Eric Sunshine wrote:\n\n> On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab wrote:\n>> On Nov 22 2022, Andreas Hasenack wrote:\n>> > /^processor[\\s\\d]*:/\n>> > or something else.\n>>\n>> Something else.\n>>\n>> $ getconf _NPROCESSORS_ONLN\n>\n> Yes, that works too and was (I'm pretty sure) considered during\n> development. The reason /proc/cpu was chosen over `getconf` was that\n> opening & reading /proc/cpu should be faster since it doesn't involve\n> spawning a process. If we did use `getconf`, we'd have to be careful\n> to degrade gracefully if `getconf` isn't available or if the\n> configuration parameter (i.e. \"_NPROCESSORS_ONLN\") isn't known on the\n> platform.\n\ngetconf is surely more portable than poking in /proc, especially\n/proc/cpuinfo is the antipode of portability.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"467882","messageId":"CAPig+cT5dxrS2-xUSYfot7k6v0qV0TXh=xSMe7Y-oMGLMhbjVA@mail.gmail.com","threadId":"58838","inReplyTo":"mvm1qpu9g2e.fsf@linux-m68k.org","subject":"Re: chainlink.pl /proc/cpuinfo regexp fails on s390x","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-23T19:10:56Z","receivedAt":"2022-11-23T19:11:19Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 23, 2022 at 4:27 AM Andreas Schwab wrote:\n> On Nov 22 2022, Eric Sunshine wrote:\n> > On Tue, Nov 22, 2022 at 6:37 PM Andreas Schwab wrote:\n> >> On Nov 22 2022, Andreas Hasenack wrote:\n> >> > /^processor[\\s\\d]*:/\n> >> > or something else.\n> >>\n> >> Something else.\n> >> $ getconf _NPROCESSORS_ONLN\n> >\n> > Yes, that works too and was (I'm pretty sure) considered during\n> > development. The reason /proc/cpu was chosen over `getconf` was that\n> > opening & reading /proc/cpu should be faster since it doesn't involve\n> > spawning a process. If we did use `getconf`, we'd have to be careful\n> > to degrade gracefully if `getconf` isn't available or if the\n> > configuration parameter (i.e. \"_NPROCESSORS_ONLN\") isn't known on the\n> > platform.\n>\n> getconf is surely more portable than poking in /proc, especially\n> /proc/cpuinfo is the antipode of portability.\n\nNo doubt, but for the immediate issue, this tightly-focused fix is\nmore appropriate and less likely to lead to unexpected additional\nproblems. I'm not arguing against `getconf`, but saying only that\n_this_ patch is fine as-is.\n\nIt may very well be a good idea to replace the /proc/cpuinfo probe by\n`getconf` in the future (or remove it altogether as mentioned\nupstream), but that's outside the scope of this patch and the\nimmediate problem.\n"}]}