Re: PATCH: add search path for tool init file (part of fix for Automake misintegration)

[Jacob Bachmeyer]

Andrew Burgess wrote:
> * Jacob Bachmeyer <address@hidden> [2018-11-14 21:50:20 -0600]:
>   
> > This patch fixes the problem of the first "make check" after running
> > configure complaining about being unable to find tool init files that I have
> > previously described.
> >
> > ChangeLog entry
> > --
> >         * runtest.exp (load_lib): Clean up whitespace.
> >         (load_tool_init): Search for tool init file instead of assuming
> >         exactly one location.  Backwards compatibility is maintained.
> >     
>
> This patch, combined with a recent change in GDB, means that GDB
> testing no longer works.

Er... oops?  ...maybe?

> I can see what the problem is, but I was
> hoping to get your guidance for what the best fix is.
>
> I've explained more inline....
>   

[response inline with your explanation]

> [...]
>   
> > diff --git a/runtest.exp b/runtest.exp
> > index 84e3e6b..5ced8ab 100644
> > --- a/runtest.exp
> > +++ b/runtest.exp
> > [...]
> > @@ -929,25 +929,21 @@ proc load_tool_init { file } {
> >      global srcdir
> >      global loaded_libs
> >  
> > -    if {[info exists loaded_libs($file)]} {
> > +    if {[info exists loaded_libs(tool/$file)]} {
> >         return
> >      }
> >  
> > -    set loaded_libs($file) ""
> > -
> > -    verbose "Looking for tool init file $srcdir/lib/$file"
> > +    set loaded_libs(tool/$file) ""
> >     
>
> This change is the one that causes problems.  Previously the call to
> 'load_tool_init' resulted in the tool init file being recorded in
> 'loaded_libs' just like any other library file - using just the name
> of the file, so for GDB this was 'gdb.exp'.
>
> After this patch the tool init file is recorded as 'tool/gdb.exp'.
>   

The patch also adds a new directory at the beginning of the search path 
for tool init files: testsuite/lib/tool.  This allows a testsuite to 
have both a library module for using a tool as part of other tests and a 
separate tool init file for testing the tool itself.  There was 
previously no elegant solution for this case.

> GDB (probably wrongly) includes, from its target init files (for
> example gdb/testsuite/config/unix.exp), the tool init file using
> 'load_lib gdb.exp'.
>   

I concur that this is incorrect; the tool init file is something that 
the DejaGnu core is supposed to handle, is firmly on the "testsuite" 
side of the testsuite/testing lab distinction, and is the first piece 
from the testsuite that DejaGnu loads.  I argue that attempting to load 
the tool init file from any other init file is a bug in the testsuite 
and/or site configuration.

> The problem is that gdb.exp now ends up getting included twice, first
> time it is included and recorded as 'tool/gdb.exp' and the second time
> as just 'gdb.exp'.  A recent change means that this file can _only_ be
> parsed once (a use of rename is the blocker).
>   

As an alternative, could the rename be made conditional on the result of 
[info commands NEW_NAME]?

I have had similar problems with another project, where I used Tcl 
namespaces in the test drivers, then switched to multipass testing.  I 
had to add "namespace forget *" just ahead of each file's "namespace 
import" block, along with other bits here and there to make sure that 
various other internal states are reset before being loaded.

> Prior to this patch the second attempt to include 'gdb.exp' would be
> ignored as 'gdb.exp' was already in loaded_libs.
>
> The obvious fix is probably just, remove the 'load_lib gdb.exp' calls
> from the GDB testsuite,

Did the GDB testsuite ever actually need those calls?  Presumably, 
DejaGnu has always correctly loaded gdb.exp as the tool init file, well 
before loading the target init file from setup_target_hook.

(Side note:  I am currently working on moving all actual code out of the 
{setup,cleanup}_{build,host,target}_hook procedures and establishing 
those as pure customization points for additional processing that a 
testing lab may want to insert.  If the GDB testsuite is depending on 
overriding any of those, now would be a good time to fix those bugs.)

> [...]
>
> It's not clear from this patch why this change was necessary, could we
> revert to the old behaviour?
>   

The patch also allows tool init files to appear in testsuite/lib/tool, a 
new feature that allows both a library and tool init file to exist under 
the same name.  For this to work, the tool init file must be recorded 
separately from a same-named library module.

> There's many calls to 'search_and_load_file' in dejagnu (the core of
> things like load_lib and load_tool_init), but only some of them are
> guarded with a check of loaded_libs.  Would there be an advantage of
> moving the loaded_libs check into 'search_and_load_file', and placing
> the absolute path to the file being loaded into loaded_libs?
>   

That might be a good idea, but I have been told 
(<URL:http://lists.gnu.org/archive/html/dejagnu/2019-01/msg00032.html>) 
that the "*load*" procedures should be considered stable API and that 
refactoring them is ill-advised without great care due to their use in 
board description files, only some of which are shipped with DejaGnu.

> Thanks for your time,
>
> Andrew
>   

-- Jacob