Change eddy cuda assumptions

[mharms]

This PR should make the automatic detection of the GPU-enabled version of eddy a bit more robust.

[mharms]

Before we merge this, I thought of a potential problem. Namely, when I diff the two files, the intended outcome is for them to differ -- i.e., return a non-zero status code. But is that going to cause a problem with the error trapping in debug.shlib? Should I just wrap that section with debug_disable_trap, followed by debug_enable_trap to turn it back on?

[coalsont]

As written, it would be a problem if we made the trap work in functions, yes - you should do if ! diff -q <whatever>; then instead. A 0 return code is treated as true, and diff returns 0 (true) when the files are identical, so it reads a bit strangely.

[mharms]

Why not just wrap that section with debug_disable_trap and then debug_enable_trap to turn it back on?

[coalsont]

Because doing it correctly (making the conditional actually use the command that does the test) is cleaner than the current code, let alone adding disable/enable around the current code?

[coalsont]

Suggested change

	diff -q ${g_stdEddy} ${g_gpuEnabledEddy} > /dev/null
	return_code=$?
	if [ "${return_code}" -eq "0" ]; then
	# diff returns true when files are the same, which we want to treat as an error
	if diff -q "${g_stdEddy}" "${g_gpuEnabledEddy}" > /dev/null; then

[coalsont]

I had it backwards, we don't want a negation, because we want to take the conditional when the files are the same. I have suggested the edit.

[mharms]

So, simply by putting the command within an if statement it will no longer trigger the error trapping in the (desired) case where the two files differ?

[coalsont]

Correct, conditional tests do not trigger trap ERR - the [ in sh-style scripting is treated as an executable (in bash, it is actually a builtin), but /usr/bin/[ generally exists and works the same way - I suspect the true, ancient sh just called /usr/bin/[ from $PATH for an if [ construct.

bash manpage:

       trap [-lp] [[arg] sigspec ...]
...
              If  a  sigspec  is  ERR,  the command arg is executed whenever a
              pipeline (which may consist of a single simple command), a list,
              or a compound command returns a non-zero exit status, subject to
              the following conditions.  The ERR trap is not executed  if  the
              failed command is part of the command list immediately following
              a while or until keyword, part of the test in an  if  statement,
              part of a command executed in a && or || list except the command
              following the final && or ||, any command in a pipeline but  the
              last,  or  if the command's return value is being inverted using
              !.  These are the same conditions obeyed  by  the  errexit  (-e)
              option.

This is slightly misleading, as set -eu will exit on undefined variable, but trap ERR will not execute under set -u with the same problem. I believe there is also something that trap ERR does treat as a problem, that set -e doesn't, but I don't recall exactly what it is - maybe something with subshells...

[mharms]

Ok, should be robust against error trapping now.

[coalsont]

FYI, () specifies a subshell (which does not behave like $(), which additionally captures stdout and returns it as a string), which in this case should do the same thing (same exit status as the last command it executed). (()) specifies a mathematical expression, with special syntax, [] uses a builtin [ command that treats its arguments as a conditional expression (similar to the test builtin), and [[]] is bash syntax that behaves mostly like [, but with some enhancements (playing nicely with && and || inside the expression).

if doesn't have any special syntax rules (it treats its arguments like any other bash command), those [], etc constructs can be used anywhere, it's just that if and while are usually the only things that need to use [] or similar (because everywhere else in a script, you generally don't want things returning nonzero exit codes, which is how [ and similar signal "false").

[glasserm]

Should I not have merged this?

[coalsont]

The commits are fine, I was commenting on a comment, for explanatory purposes, not suggesting a change.

[mharms]

I can remove the last line of the comment if you want (and just push the change directly to master), but are you saying that using (), $(), or [] would all survive the error trapping as well, in the case when the files differ?

[coalsont]

No. Using [] would change the expected syntax (because [ is particular about what its arguments are), and would throw an error. Using $() would be wrong, as if would then try to execute whatever string came from stdout of the diff (but it is redirected currently, so if would always try to execute the empty string).

I am not suggesting a change, just trying to explain why [] would fail. if itself doesn't have special syntax, but whatever you put after if sets the syntax for anything that comes after it ([ accepts arguments like -e somefile ], (()) takes integer arithmetic, etc).

[coalsont]

Maybe this is a less roundabout explanation: every if [ statement was already using a nonzero exit status, whenever the condition was false, because a nonzero exit status was always the only way to tell if to skip the then section and do the else section if it exists - [ was just the command that was giving the exit status:

tim@blackbox:~$ [ -e foo ]
tim@blackbox:~$ echo $?
1

Thus, nonzero exit status in an if or while, etc. is expected, and therefore these are specially exempted from the trap ERR and set -e rules.