Some bugfixes for the tools/test-avrdude script

[ndim]

While working on #1888, I have found and fixed a few issues with tools/test-avrdude:

• Since commit cf0822b, the avrdude -v version line looks different, and therefore the test-avrdude code which deals with the version does not work any more. Impact: The output looks like

  Testing avrdudePrepare "-cdryrun -pm2560" and press 'enter' or 'space' to continue. Press any other key to skip
  Cannot detect flash; check that "-cdryrun -pm2560" are valid avrdude options; skipping this test
  

  instead of

  Testing avrdude version 1.2.3
  Prepare "-cdryrun -pm2560" and press 'enter' or 'space' to continue. Press any other key to skip
  Cannot detect flash; check that "-cdryrun -pm2560" are valid avrdude options; skipping this test
  

  The fix is easy and just make grep look for both "version" and "Version".

• If avrdude -v does not run, the remaining tests in tools/test-avrdude will not run either, so aborting with a useful error message showing the complete output of avrdude -v makes sense.

• As it is not clear yet which avrdude executable tools/test-avrdude is actually running, it makes sense to... well not so sure about that any more. But having the complete output of type $avrdude_bin has certainly been helpful when trying to run the wrong avrdude executable, and that is what that commit does.

• tools/test-avrdude uses bash associative arrays. Associative arrays are a feature introduced with bash 4.0, so the bash version shipped with MacOS (3.x) does not support associative arrays. I have replaced the associative array with a shell function.

@stefanrueger Not sure whether you want to merge any of this before 8.0. I would suggest at least cherrypicking 29133dc.

[ndim]

Ah. Just noticed that properly failing if avrdude -v cannot even run will fail the linux-x86_64-autotools build dry-run stage. Give me a few minutes to fix that.

[stefanrueger]

@ndim Great catch!

I ever so slightly prefer the following variant:

diff --git a/tools/test-avrdude b/tools/test-avrdude
index 233f5c86..8b030058 100755
--- a/tools/test-avrdude
+++ b/tools/test-avrdude
@@ -158,8 +158,8 @@ tmpfile=$(mktemp "$tmp/$progname.tmp.XXXXXX")
 resfile=$(mktemp "$tmp/$progname.res.XXXXXX")
 trap "rm -f $status $logfile $outfile $tmpfile $resfile" EXIT
 
-echo -n "Testing '$avrdude_bin -v'..."
-$avrdude_bin -v 2>&1 | grep '[vV]ersion' | cut -f2- -d: | sed s/Version/version/ > "$outfile"
+echo -n "Testing $avrdude_bin "
+$avrdude_bin -v 2>&1 | grep ersion | sed s/"^.* [Vv]"ersion/version/ | head -1 > "$outfile"
 if grep version "$outfile" > /dev/null 2>&1; then
     cat "$outfile"
 else

[askn37]

There's something bothering me about test-avrdude.

For example, when I try to pass -xrtsdtr=high along with -cserialupdi, dry-run doesn't recognize it, throws an error and doesn't allow me to continue. Is it possible to devise additional option statements that I want to include in my tests but can't pass to dry-run?

Or would it be better if dry-run didn't exit with an error for unknown -x?

[ndim]

Done, AFAICT.

[stefanrueger]

@ndim Thanks for fixing the [vV]ersion bug and for making the script run on older bash versions. Looks great!

Just checking whether you need the first line of the output

$ tools/test-avrdude -p "-c dryrun -p m328p"
Testing executable type for 'avrdude'... avrdude is /usr/local/bin/avrdude
Testing avrdude version... 7.3-20240815 (e230d889)
Prepare "-c dryrun -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip

My preference would be a slightly crisper output:

$ tools/test-avrdude -p "-c dryrun -p m328p"
Testing /usr/local/bin/avrdude 7.3-20240815 (e230d889)
Prepare "-c dryrun -p m328p" and press 'enter' or 'space' to continue. Press any other key to skip

@askn37 @MCUdude Yes, currently cannot pass -x arguments. I had a look and it turns out it is easy to remedy.

@ndim Both points would be solved by the following patch

diff --git a/tools/test-avrdude b/tools/test-avrdude
index 192bee63..5e9e6f42 100755
--- a/tools/test-avrdude
+++ b/tools/test-avrdude
@@ -149,12 +149,7 @@ fi
 
 arraylength=${#pgm_and_target[@]}
 
-echo -n "Testing executable type for '$avrdude_bin'..."
-if type "$avrdude_bin" >/dev/null 2>&1; then
-    echo -n " "
-    type "$avrdude_bin"
-else
-    echo
+if ! type "$avrdude_bin" >/dev/null 2>&1; then
     echo "$progname: cannot execute $avrdude_bin"
     exit 1
 fi
@@ -167,13 +162,12 @@ tmpfile=$(mktemp "$tmp/$progname.tmp.XXXXXX")
 resfile=$(mktemp "$tmp/$progname.res.XXXXXX")
 trap "rm -f $status $logfile $outfile $tmpfile $resfile" EXIT
 
-echo -n "Testing $avrdude_bin version..."
-$avrdude_bin -v 2>&1 | grep '[vV]ersion' | sed 's/^.* [Vv]ersion //' | head -n1 > "$outfile"
+echo -n "Testing "$(type -p "$avrdude_bin")
+$avrdude_bin -v 2>&1 | grep -i version | sed 's/^.* version//i' | head -n1 > "$outfile"
 if test -s "$outfile"; then
-    echo -n " "
     cat "$outfile"
 else
-    echo " error"
+    echo ": error obtaining version using $avrdude_bin -v"
     $avrdude_bin -v
     exit 1
 fi
@@ -273,9 +267,9 @@ for (( p=0; p<$arraylength; p++ )); do
     avrdude=($avrdude_bin -l $logfile $avrdude_conf -qq ${pgm_and_target[$p]})
 
     # Get flash and EEPROM size in bytes and make sure the numbers are in dec form
-    flash_size=$(${avrdude[@]} -cdryrun -T 'part -m' 2>/dev/null | grep flash | awk '{print $2}')
+    flash_size=$("$avrdude_bin" -c dryrun -p $part -T 'part -m' 2>/dev/null | grep flash | awk '{print $2}')
     bench_flwr_size=$((flash_size/6)) # Approximate(!) size of file holes_rjmp_loops_${flash_size}B.hex
-    ee_size=$(${avrdude[@]} -cdryrun -T 'part -m' 2>/dev/null | grep eeprom | awk '{print $2}')
+    ee_size=$("$avrdude_bin" -c dryrun -p $part -T 'part -m' 2>/dev/null | grep eeprom | awk '{print $2}')
     bench_eewr_size=$((ee_size/6)) # Approximate(!) size of file holes_pack_my_box_${ee_size}B.hex
 
     if [[ -z "$flash_size" ]]; then

How should we do this? Do you want to apply this patch? Should I push a commit onto your branch?

[ndim]

@stefanrueger I have taken up that patch and reworked it a bit, splitting it into two commits.

The first commit condenses two output lines into one output line:

• Your quoting is weird. I have fixed that.

• I have added a normal type $avrdude_bin command to print potentially helpful information in the error case.

• Completely ignoring case in grep and sed does not make sense IMHO. We do not want to match "vErsIOn", do we? I have therefore kept the "[Vv]ersion" regexps.

The second commit deals with flash_size and ee_size. I do not understand your motiviation for the changes, but I have found something to change nevertheless:

• When you are replacing ${avrdude[@]} (which should be "${avrdude[@]}" presuming proper quoting when composing the avrdude array) by "$avrdude_bin" (which should not be quoted, just like all the other uses of $avrdude_bin, allowing avrdude_bin to be something like "env FOO=bar /path/to/avrdude"), shouldn't that line also contain $avrdude_conf? I have removed the quotes and added $avrdude_conf.

  Reasoning: When just running "avrdude -v" to obtain the version number, the config file is not needed. But to do anything with parts and programmers (like determine flash or eeprom sizes), avrdude needs a config file, right?

How should we do this? Do you want to apply this patch? Should I push a commit onto your branch?

Applied, with the changes described above.

[stefanrueger]

@ndim Well spotted re the $avrdude_conf addition. Thanks for carefully looking at test-avrdue! And yes, quoting is always sth of a black art in Unix. Huge design mistake to have allowed white space, even newline, in file names some 55 years ago.

This should now be ready for merging.