[PATCH] genmultilib: Add sanity check

[PATCH] genmultilib: Add sanity check

[Christophe Lyon]

When a list of dirnames is provided to genmultilib, its length is
expected to match the number of options.  If this is not the case, the
build fails later for reasons not obviously related to this mistake.
This patch adds a sanity check to help diagnose such cases.

Tested by adding an option to t-aarch64 and no corresponding dirname,
with both bash and dash.

OK for trunk?

gcc/ChangeLog:

	* genmultilib: Add sanity check.
---
 gcc/genmultilib | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gcc/genmultilib b/gcc/genmultilib
index 1e387fb1589..ef121e77d17 100644
--- a/gcc/genmultilib
+++ b/gcc/genmultilib
@@ -141,6 +141,20 @@ multiarch=$9
 multilib_reuse=${10}
 enable_multilib=${11}
 
+# Sanity check: make sure we have as many dirnames as options
+if [ -n "${dirnames}" ]; then
+    options_arr=($options)
+    dirnames_arr=($dirnames)
+    nboptions=${#options_arr[@]}
+    nbdirnames=${#dirnames_arr[@]}
+    if [ $nbdirnames -ne $nboptions ]; then
+	echo 1>&2 "Error calling $0: Number of dirnames ($nbdirnames) does not match number of options ($nboptions)"
+	echo 1>&2 "options: ${options}"
+	echo 1>&2 "dirnames: ${dirnames}"
+	exit 1
+    fi
+fi
+
 echo "static const char *const multilib_raw[] = {"
 
 mkdir tmpmultilib.$$ || exit 1
-- 
2.34.1

Re: [PATCH] genmultilib: Add sanity check

[Joseph Myers]

On Wed, 2 Nov 2022, Christophe Lyon via Gcc-patches wrote:

> +# Sanity check: make sure we have as many dirnames as options
> +if [ -n "${dirnames}" ]; then
> +    options_arr=($options)

This is an sh script; arrays are a bash feature.  Building GCC isn't 
supposed to need bash (or to rely on $(SHELL) being bash, even when bash 
is available - many GNU/Linux systems use dash for /bin/sh), only a POSIX 
shell.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] genmultilib: Add sanity check

[Christophe Lyon]

On 11/2/22 18:29, Joseph Myers wrote:
> On Wed, 2 Nov 2022, Christophe Lyon via Gcc-patches wrote:
> 
>> +# Sanity check: make sure we have as many dirnames as options
>> +if [ -n "${dirnames}" ]; then
>> +    options_arr=($options)
> 
> This is an sh script; arrays are a bash feature.  Building GCC isn't
> supposed to need bash (or to rely on $(SHELL) being bash, even when bash
> is available - many GNU/Linux systems use dash for /bin/sh), only a POSIX
> shell.
> 

That's what I feared, and I did "try to try" to build with dash, but I 
realize now that changing SHELL in the generated gcc/Makefile is not 
enough since it's defined by the higher level Makefile/config.status. 
Indeed rebuilding from scratch with CONFIG_SHELL=/bin/dash fails with my 
patch.

We have lived with that behavior for years, so it's not that bad anyway :-)

Thanks,

Christophe

[PATCH v2] genmultilib: Add sanity check

[Christophe Lyon]

When a list of dirnames is provided to genmultilib, its length is
expected to match the number of options.  If this is not the case, the
build fails later for reasons not obviously related to this mistake.
This patch adds a sanity check to help diagnose such cases.

Tested by adding an option to t-aarch64 and no corresponding dirname,
with both bash and dash.

v2: do not use arrays (bash feature).

OK for trunk?

gcc/ChangeLog:

	* genmultilib: Add sanity check.
---
 gcc/genmultilib | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gcc/genmultilib b/gcc/genmultilib
index 1e387fb1589..b5f372c8358 100644
--- a/gcc/genmultilib
+++ b/gcc/genmultilib
@@ -141,6 +141,20 @@ multiarch=$9
 multilib_reuse=${10}
 enable_multilib=${11}
 
+# Sanity check: make sure we have as many dirnames as options
+if [ -n "${dirnames}" ]; then
+    set x $options
+    nboptions=$#
+    set x $dirnames
+    nbdirnames=$#
+    if [ $nbdirnames -ne $nboptions ]; then
+	echo 1>&2 "Error calling $0: Number of dirnames ($nbdirnames) does not match number of options ($nboptions)"
+	echo 1>&2 "options: ${options}"
+	echo 1>&2 "dirnames: ${dirnames}"
+	exit 1
+    fi
+fi
+
 echo "static const char *const multilib_raw[] = {"
 
 mkdir tmpmultilib.$$ || exit 1
-- 
2.34.1

Re: [PATCH v2] genmultilib: Add sanity check

[Jeff Law]

On 11/3/22 03:52, Christophe Lyon via Gcc-patches wrote:
> When a list of dirnames is provided to genmultilib, its length is
> expected to match the number of options.  If this is not the case, the
> build fails later for reasons not obviously related to this mistake.
> This patch adds a sanity check to help diagnose such cases.
>
> Tested by adding an option to t-aarch64 and no corresponding dirname,
> with both bash and dash.
>
> v2: do not use arrays (bash feature).
>
> OK for trunk?
>
> gcc/ChangeLog:
>
> 	* genmultilib: Add sanity check.

OK.  It should be interesting to see if it trips.


jeff

Re: [PATCH v2] genmultilib: Add sanity check

[Mark Wielaard]

Hi,

On Fri, Nov 18, 2022 at 12:42:10PM -0700, Jeff Law wrote:
> > 	* genmultilib: Add sanity check.
> 
> OK.  It should be interesting to see if it trips.

It trips up various buildbot setups:
https://builder.sourceware.org/buildbot/#/changes/13720

Error calling ../../gcc/gcc/genmultilib: Number of dirnames (3) does not match number of options (2)
options: m64/m31
dirnames: 64 32
make[2]: *** [Makefile:2240: s-mlib] Error 1

Mark

Re: [PATCH v2] genmultilib: Add sanity check

[Christophe Lyon]

On 11/21/22 11:16, Mark Wielaard wrote:
> Hi,
> 
> On Fri, Nov 18, 2022 at 12:42:10PM -0700, Jeff Law wrote:
>>> 	* genmultilib: Add sanity check.
>>
>> OK.� It should be interesting to see if it trips.
> 
> It trips up various buildbot setups:
> https://builder.sourceware.org/buildbot/#/changes/13720
> 
> Error calling ../../gcc/gcc/genmultilib: Number of dirnames (3) does not match number of options (2)
> options: m64/m31
> dirnames: 64 32
> make[2]: *** [Makefile:2240: s-mlib] Error 1
> 
> Mark

Indeed, sorry for that.

I've just sent a fix:
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606887.html

Hopefully that one is right

Christophe

Re: [PATCH v2] genmultilib: Add sanity check

[Mark Wielaard]

Hi Christophe,

On Mon, 2022-11-21 at 13:12 +0100, Christophe Lyon wrote:
> On 11/21/22 11:16, Mark Wielaard wrote:
> > On Fri, Nov 18, 2022 at 12:42:10PM -0700, Jeff Law wrote:
> > > > 	* genmultilib: Add sanity check.
> > > 
> > > OK. It should be interesting to see if it trips.
> > 
> > It trips up various buildbot setups:
> > https://builder.sourceware.org/buildbot/#/changes/13720
> > 
> > Error calling ../../gcc/gcc/genmultilib: Number of dirnames (3)
> > does not match number of options (2)
> > options: m64/m31
> > dirnames: 64 32
> > make[2]: *** [Makefile:2240: s-mlib] Error 1
> 
> Indeed, sorry for that.
> 
> I've just sent a fix:
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606887.html
> 
> Hopefully that one is right

The buildbot gcc builds are turning green again!
https://builder.sourceware.org/buildbot/#/changes/13736

Thanks,

Mark

Re: [PATCH v2] genmultilib: Add sanity check

[Christophe Lyon]

On 11/21/22 13:32, Mark Wielaard wrote:
> Hi Christophe,
> 
> On Mon, 2022-11-21 at 13:12 +0100, Christophe Lyon wrote:
>> On 11/21/22 11:16, Mark Wielaard wrote:
>>> On Fri, Nov 18, 2022 at 12:42:10PM -0700, Jeff Law wrote:
>>>>> 	* genmultilib: Add sanity check.
>>>>
>>>> OK. It should be interesting to see if it trips.
>>>
>>> It trips up various buildbot setups:
>>> https://builder.sourceware.org/buildbot/#/changes/13720
>>>
>>> Error calling ../../gcc/gcc/genmultilib: Number of dirnames (3)
>>> does not match number of options (2)
>>> options: m64/m31
>>> dirnames: 64 32
>>> make[2]: *** [Makefile:2240: s-mlib] Error 1
>>
>> Indeed, sorry for that.
>>
>> I've just sent a fix:
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606887.html
>>
>> Hopefully that one is right
> 
> The buildbot gcc builds are turning green again!
> https://builder.sourceware.org/buildbot/#/changes/13736
> 

Good! I didn't imagine the feedback would be so fast :-)

BTW, I don't remember receiving an email from the buildbot after the 
breakage, does it send such emails for binutils/gdb only?

> Thanks,
> 
> Mark

Activate gcc builder problem emails (Was: [PATCH v2] genmultilib: Add sanity check)

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

Hi Christophe,

On Mon, Nov 21, 2022 at 01:35:34PM +0100, Christophe Lyon wrote:
> On 11/21/22 13:32, Mark Wielaard wrote:
> > > I've just sent a fix:
> > > https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606887.html
> > > 
> > > Hopefully that one is right
> > 
> > The buildbot gcc builds are turning green again!
> > https://builder.sourceware.org/buildbot/#/changes/13736
> 
> Good! I didn't imagine the feedback would be so fast :-)
> 
> BTW, I don't remember receiving an email from the buildbot after the
> breakage, does it send such emails for binutils/gdb only?

You are right, we didn't seem to have any problem report emails for
the gcc builds. The attached (pushed) patch activates email reports
for the quick gcc builders. Whenever they fail the patch author should
get an email now. Also it will now sent email to gcc-testresults if a
build starts failing or becomes successful again.

Note that we don't have good reporting for the full gcc builders
yet. We don't have regression detection yet, so we can only see that
the full testsuite passes or fails. All results are put into the
bunsen database though.

Cheers,

Mark

[-- Attachment #2: 0001-Add-mail-notifiers-for-gcc-builds.patch --]
[-- Type: text/x-diff, Size: 2355 bytes --]

From adbdb81530f97880facb942afe14781c1006e283 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Mon, 21 Nov 2022 23:47:53 +0100
Subject: [PATCH] Add mail notifiers for gcc builds

Change gcc_fedrawhide_x86_64_builder tag from "gcc" to "gcc-full"
like other "full" builders (debian-amd64, ubuntu-armhf and ubuntu-arm64).

Add mail notifiers for builders tagged "gcc". One to sent email to
patch author if a new problem (failed build) appears. Another to
sent email to gcc-testresults whenever a build changes from success
to failed or the other way around.
---
 builder/master.cfg | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/builder/master.cfg b/builder/master.cfg
index 61bf4f1..b25731d 100644
--- a/builder/master.cfg
+++ b/builder/master.cfg
@@ -2888,7 +2888,7 @@ gcc_fedrawhide_x86_64_builder = util.BuilderConfig(
         name="gcc-fedrawhide-x86_64",
         collapseRequests=True,
         workernames=["fedrawhide-x86_64"],
-        tags=["gcc", "fedora", "x86_64"],
+        tags=["gcc-full", "fedora", "x86_64"],
         factory=gcc_factory)
 c['builders'].append(gcc_fedrawhide_x86_64_builder)
 
@@ -3857,6 +3857,27 @@ mn_elfutils_try = reporters.MailNotifier(
         generators=[generator_elfutils_try])
 c['services'].append(mn_elfutils_try)
 
+# Problem report for the whole gcc tagged builder set
+# Goes to patch author if a new problem appears
+generator_gcc = reporters.BuildSetStatusGenerator(
+        mode=('problem',), tags=['gcc'])
+mn_gcc = reporters.MailNotifier(
+        fromaddr="builder@sourceware.org",
+        sendToInterestedUsers=True,
+        generators=[generator_gcc])
+c['services'].append(mn_gcc)
+
+# Change report for the whole gcc tagged builder set
+# Goes to the mailinglist if a builder result changes
+generator_gcc_change = reporters.BuildSetStatusGenerator(
+        mode=('change',), tags=['gcc'])
+mn_gcc_change = reporters.MailNotifier(
+        fromaddr="builder@sourceware.org",
+        sendToInterestedUsers=False,
+        extraRecipients=['gcc-testresults@gcc.gnu.org'],
+        generators=[generator_gcc_change])
+c['services'].append(mn_gcc_change)
+
 # Problem report for the whole gccrust tagged builder set
 generator_gccrust = reporters.BuildSetStatusGenerator(
         mode=('problem',), tags=['gccrust'])
-- 
2.30.2