[OB] gdb/testsuite: Wrap `param_integer_error' in gdb.guile/scm-parameter.exp

Message ID alpine.DEB.2.20.2210261710250.19931@tpp.orcam.me.uk
State Committed
Commit 4404bce9a723489e0fa2bee8a41bbf56eb3f3d72
Headers
Series [OB] gdb/testsuite: Wrap `param_integer_error' in gdb.guile/scm-parameter.exp |

Commit Message

Maciej W. Rozycki Oct. 29, 2022, 1:56 p.m. UTC
  Wrap an overlong line in the definition of `param_integer_error' in 
gdb.guile/scm-parameter.exp.  No functional change.
---
On Wed, 26 Oct 2022, Simon Marchi wrote:

> >>>   FTR I'm still looking into it and like you I have hesitated to just paper
> >>> the issue over by allowing both outputs without first understanding what
> >>> is really going on here.  I cannot rule out a distribution-specific patch
> >>> causing a discrepancy here, but I feel like tracking it down.
> >>>
> >>>   NB guile 2.0.13 here, reporting as:
> >>>
> >>> guile (GNU Guile) 2.0.13
> >>> Packaged by Debian (2.0.13-deb+1-5.4)
> >>
> >> According to that version number, it looks like Ubuntu 20.04?
> >>
> >>    https://packages.ubuntu.com/focal/guile-2.0
> >>
> >> I tried building on Ubuntu 20.04 against guile-2.0, and I see the same
> >> result as you.  And when I try guile2.0 on Arch Linux (this package
> >> [1]), I also see the same result as you.  So I must have tested it wrong
> >> previously.
> >>
> >> You can dig further if you want, but I'd be fine just accepting both
> >> outputs and saying that guile-2.0 outputs the additional ERROR: while
> >> subsequent versions do not.
> >>
> > 
> > FWIW, I did the same here in commit 6bbe1a929c6 ("[gdb/testsuite] Fix gdb.guile/scm-breakpoint.exp with guile 3.0").
> 
> Thanks, then I just went ahead and pushed this:

 Thanks, though why such a rush to fix a benign test failure while the 
review took months in the first place?

 FWIW I have been struggling to get multiple versions of Guile compiled 
and installed locally (easier) and then GDB built against each of them 
(tougher).  As it turns out our documentation is misleading.  We have:

`--with-guile[=GUILE]'
     Build GDB with GNU Guile scripting support.  (Done by default if
     libguile is present and found at configure time.)  If your host
     does not have Guile installed, you can find it at
     `https://www.gnu.org/software/guile/'.  The optional argument
     GUILE can be a version number, which will cause `configure' to
     try to use that version of Guile; or the file name of a
     `pkg-config' executable, which will be queried to find the
     information needed to compile and link against Guile.

(and similarly in `./configure --help'), so one could have thought 
`--with-guile=2.0' will work.  Well, not.  You need to specify both the 
name and the version, e.g.: `--with-guile=guile-2.0', which I find kind of 
awkward: why would one want to have a Guile package under a name that is 
not "guile"?  I think documentation is sensible and it's implementation 
that ought to be fixed.

 But that is not enough.  Still I got:

checking whether to use guile... guile-2.0
checking for pkg-config... /usr/bin/pkg-config
checking for usable guile from /usr/bin/pkg-config... checking for scm_set_automatic_finalization_enabled... no
configure: error: in `.../gdb':
configure: error: linking guile version guile-2.0 test program failed
See `config.log' for more details
make[1]: *** [Makefile:12496: configure-gdb] Error 1

This is because I have built local Guile as static libraries only and 
dependencies are not pulled with the (incorrect) `pkg-config' invocation 
we have in our configury.

 I got these issues sorted now and will be posting fixes soon.  With these 
in place I have figured out that the message changed between Guile 2.0 and 
2.2.

 Last but not least here's a fix I committed as obvious to correct an 
overlong line you have introduced with your change.  While we do have an 
exemption for TCL code, there's no need to make use of it here and these 
long lines hit clarity of code badly.  I have verified this test case to 
still pass after my change.

  Maciej
---
 gdb/testsuite/gdb.guile/scm-parameter.exp |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

gdb-test-scm-parameter-error-wrap.diff
  

Comments

Simon Marchi Oct. 31, 2022, 12:57 p.m. UTC | #1
On 10/29/22 09:56, Maciej W. Rozycki wrote:
> Wrap an overlong line in the definition of `param_integer_error' in 
> gdb.guile/scm-parameter.exp.  No functional change.
> ---
> On Wed, 26 Oct 2022, Simon Marchi wrote:
> 
>>>>>   FTR I'm still looking into it and like you I have hesitated to just paper
>>>>> the issue over by allowing both outputs without first understanding what
>>>>> is really going on here.  I cannot rule out a distribution-specific patch
>>>>> causing a discrepancy here, but I feel like tracking it down.
>>>>>
>>>>>   NB guile 2.0.13 here, reporting as:
>>>>>
>>>>> guile (GNU Guile) 2.0.13
>>>>> Packaged by Debian (2.0.13-deb+1-5.4)
>>>>
>>>> According to that version number, it looks like Ubuntu 20.04?
>>>>
>>>>    https://packages.ubuntu.com/focal/guile-2.0
>>>>
>>>> I tried building on Ubuntu 20.04 against guile-2.0, and I see the same
>>>> result as you.  And when I try guile2.0 on Arch Linux (this package
>>>> [1]), I also see the same result as you.  So I must have tested it wrong
>>>> previously.
>>>>
>>>> You can dig further if you want, but I'd be fine just accepting both
>>>> outputs and saying that guile-2.0 outputs the additional ERROR: while
>>>> subsequent versions do not.
>>>>
>>>
>>> FWIW, I did the same here in commit 6bbe1a929c6 ("[gdb/testsuite] Fix gdb.guile/scm-breakpoint.exp with guile 3.0").
>>
>> Thanks, then I just went ahead and pushed this:
> 
>  Thanks, though why such a rush to fix a benign test failure while the 
> review took months in the first place?

The fix seemed relatively obvious, given we already had one instance of
this.

>  FWIW I have been struggling to get multiple versions of Guile compiled 
> and installed locally (easier) and then GDB built against each of them 
> (tougher).  As it turns out our documentation is misleading.  We have:
> 
> `--with-guile[=GUILE]'
>      Build GDB with GNU Guile scripting support.  (Done by default if
>      libguile is present and found at configure time.)  If your host
>      does not have Guile installed, you can find it at
>      `https://www.gnu.org/software/guile/'.  The optional argument
>      GUILE can be a version number, which will cause `configure' to
>      try to use that version of Guile; or the file name of a
>      `pkg-config' executable, which will be queried to find the
>      information needed to compile and link against Guile.
> 
> (and similarly in `./configure --help'), so one could have thought 
> `--with-guile=2.0' will work.  Well, not.  You need to specify both the 
> name and the version, e.g.: `--with-guile=guile-2.0', which I find kind of 
> awkward: why would one want to have a Guile package under a name that is 
> not "guile"?  I think documentation is sensible and it's implementation 
> that ought to be fixed.

Ack.

> 
>  But that is not enough.  Still I got:
> 
> checking whether to use guile... guile-2.0
> checking for pkg-config... /usr/bin/pkg-config
> checking for usable guile from /usr/bin/pkg-config... checking for scm_set_automatic_finalization_enabled... no
> configure: error: in `.../gdb':
> configure: error: linking guile version guile-2.0 test program failed
> See `config.log' for more details
> make[1]: *** [Makefile:12496: configure-gdb] Error 1
> 
> This is because I have built local Guile as static libraries only and 
> dependencies are not pulled with the (incorrect) `pkg-config' invocation 
> we have in our configury.
> 
>  I got these issues sorted now and will be posting fixes soon.  With these 
> in place I have figured out that the message changed between Guile 2.0 and 
> 2.2.

Thanks.

>  Last but not least here's a fix I committed as obvious to correct an 
> overlong line you have introduced with your change.  While we do have an 
> exemption for TCL code, there's no need to make use of it here and these 
> long lines hit clarity of code badly.  I have verified this test case to 
> still pass after my change.

This is subjective.  For expected output patterns, I prefer to format it
as one expected line per source line.  But how you formatted it is fine
with me as well.

Simon
  

Patch

Index: src/gdb/testsuite/gdb.guile/scm-parameter.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.guile/scm-parameter.exp
+++ src/gdb/testsuite/gdb.guile/scm-parameter.exp
@@ -185,7 +185,9 @@  foreach_with_prefix kind {
     set param_integer_error \
 	[multi_line \
 	    "ERROR: In procedure set-parameter-value!:" \
-	    "(ERROR: )?In procedure gdbscm_set_parameter_value_x: Wrong type argument in position 2 \\(expecting integer\\): #:unlimited" \
+	    "(ERROR: )?In procedure gdbscm_set_parameter_value_x:\
+	     Wrong type argument in position 2 \\(expecting integer\\):\
+	     #:unlimited" \
 	    "Error while executing Scheme code\\."]
     set param_minus_one_error "integer -1 out of range"
     set param_minus_two_range "integer -2 out of range"