Re: [PATCH] Another board file for remote host

[Doug Evans <xdje42@gmail.com>]

Yao Qi <yao@codesourcery.com> writes:
> Doug Evans <xdje42@gmail.com> writes:
>
>> +global GDB
>> +set GDB [file join [pwd] "../gdb"]
>>
>> Check if GDB has been set first, allow the user to
>> pass this in from the command line.
>
> GDB is always set in lib/gdb.exp and it is loaded earlier than the
> board file.  In lib/gdb.exp:
>
> if ![info exists GDB] {
>     if ![is_remote host] {
> 	set GDB [findfile $base_dir/../../gdb/gdb "$base_dir/../../gdb/gdb" [transform gdb]]
>     } else {
> 	set GDB [transform gdb]
>     }
> }
>
> so we can't do the check here.

I deleted the setting of GDB in this file and was able to
run tests by passing GDB=/usr/bin/gdb on the command line.

I then tried not passing GDB= on the command line and I see /usr/bin/gdb
still being run (it's invoked as "gdb" without any path).  Bleah.

It would really be nice to make this work, but it's not critical
for this first checkin.
OTOH, I think this is way too subtle an issue and requires a comment.
Something like:

+# We have to explicitly specify GDB with the path to the copy in
+# in the build directory because otherwise it will be set to the
+# result of "transform GDB" since the harness thinks we're using
+# a remote host.  See lib/gdb.exp.
+set GDB [file join [pwd] "../gdb"]
+verbose -log "Overriding setting of GDB to $GDB"

There's no need for "global GDB" here.

>> +# The directory to copy files to.  In default, we choose ./remote-host, to
>> +# avoiding messing up your HOME.  You can choose other directory as
>> +# you like.
>> +set host_dir [file join [pwd] "remote-host"]
>>
>> How about allowing the user to pass this in from the command line?
>> By convention such variables are then uppercase.
>>
>
> That is fine to me.  In the updated patch, user can specify the
> directory in command line via variable HOST_DIR.
>
>> +
>> +proc ${board}_download { board src dest } {
>> +    global env board_type
>> +    global host_dir
>> +
>> +    if { [llength $dest] > 0 } {
>> +	set destfile [lindex $dest 0]
>> +    } else {
>> +	set destfile [file tail $src]
>> +    }
>>
>> This doesn't feel right.
>> I realize /usr/share/dejagnu/remote.exp:remote_download
>> checks for whether the third parameter can be a list, but
>> remote_download is a varargs function (the name of the last
>> variable is "args") and is not a "board" function.
>> Other ${foo}_download functions that are actual "board" functions,
>> e.g., /usr/share/dejagnu/remote.exp:standard_download,
>> or gdb/testsuite/boards/remote-stdio-gdbserver.exp,
>> don't treat the third parameter as if it can be a list.
>
> Yes, remote_download acts as a wrapper or an adapter to "board"
> functions which are defined in board files.  In tcl, simple string is
> also list, see,
>
> % llength "foo"
> 1
> % lindex "foo" 0     
> foo
>
> The code above is right to me but unnecessary.  Use dest as a string in
> the updated patch.

Besides the issue of setting GDB I have one more nit.
I found this useful while diagnosing failures:

 proc ${board}_download { board src dest } {
     global HOST_DIR
 
     if { ![file exists $HOST_DIR] } {
 	file mkdir $HOST_DIR
     }
 
     set destfile [file join $HOST_DIR $dest]
+    verbose -log "${board}_download: file copy -force $src $destfile"
     file copy -force $src $destfile
 
     return $destfile
 }

With those two issues addressed I'm happy.
[I'm sure more issues will arise as I'm seeing failures
I wouldn't have expected.  I didn't spend much time on them
and at any rate I think we can get this checked in so that
we can start addressing them collectively.]