#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: review Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: | Keywords: -------------------------+-------------------------------------------------- Changes (by rhaas):

* status: new => review

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: closed Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: fixed | Keywords: -------------------------+-------------------------------------------------- Changes (by rhaas):

* status: reviewed_ok => closed * resolution: => fixed

Comment:

applied as rev 2101 as of ET_2013_05 (already in trunk in rev 2100).

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: | Keywords: -------------------------+--------------------------------------------------

Comment (by rhaas):

The fix is designed to cure precisely this problem. Are you still seeing this?

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: | Keywords: -------------------------+--------------------------------------------------

Comment (by knarf):

We should find a solution that works everywhere, as soon as possible, and backport it after testing.

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: | Keywords: -------------------------+--------------------------------------------------

Comment (by hinder):

I was trying to run the tests on the release branch before the release, and found that there was a regression. I didn't have time to revert the commit to see if this was responsible, but I was previously able to run the tests on the release (ET_2013_05) branch.

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: | Keywords: -------------------------+--------------------------------------------------

Comment (by hinder):

I was referring to reverting the commit locally for testing purposes. I assume that the commit is required, and just needs to be tweaked somehow. It's also possible that I made an error myself when running the tests.

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: | Keywords: -------------------------+--------------------------------------------------

Comment (by hinder):

I can see r2101 on the release branch, and r2100 on the trunk, both of which have the subject line "be more careful extraction $MAKE in RunTestSuite", and both of which seem to be the same patch to RunTestSuite. These were both committed (by Roland) on 20th May, but several hours apart. So the fix seems to have been applied both to the trunk and the release branch.

I just tried to run with the version from the release branch again, and get the same error.

RunTestSuite contains

{{{ MAKE=$(@SOURCEDIR@/simfactory/bin/sim print-mdb-entry @MACHINE@ | grep '^make' | sed -e 's/^make *= *//;s/ *#.*$//') }}}

Adding set -x to the script gives:

{{{ + command=/lustre/datura/ianhin/simulations/ettests_1proc/output-0000/SIMFACTORY/RunScript + export 'CCTK_TESTSUITE_RUN_COMMAND=ln -fns . output-0000-active && mkdir -p SIMFACTORY && TESTSUITE_PARFILE=$parfile /lustre/datura/ianhin/simulations/ettests_1proc/output-0000/SIMFACTORY/RunScript' + CCTK_TESTSUITE_RUN_COMMAND='ln -fns . output-0000-active && mkdir -p SIMFACTORY && TESTSUITE_PARFILE=$parfile /lustre/datura/ianhin/simulations/ettests_1proc/output-0000/SIMFACTORY/RunScript' + export CCTK_TESTSUITE_RUN_PROCESSORS=1 + CCTK_TESTSUITE_RUN_PROCESSORS=1 + ulimit -c 0 ++ /home/ianhin/Cactus/EinsteinToolkit/simfactory/bin/sim print-mdb-entry datura ++ grep '^make' ++ sed -e 's/^make *= *//;s/ *#.*$//' + MAKE='INTEL_LICENSE_FILE=/cluster/intel/licenses make -j32' + INTEL_LICENSE_FILE=/cluster/intel/licenses make -j32 ET_2013_06-testsuite PROMPT=no /lustre/datura/ianhin/simulations/ettests_1proc/output-0000/RunTestSuite: line 23: INTEL_LICENSE_FILE=/cluster/intel/licenses: No such file or directory }}}

It seems to be getting the correct value for the MAKE variable, but some interplay between substitution and word-splitting is causing an error. If I do

{{{ [ianhin@login-damiana EinsteinToolkit]$ cmd='ls -l' [ianhin@login-damiana EinsteinToolkit]$ $cmd -a total 156 drwxr-xr-x+ 15 ianhin users 4096 Nov 7 2012 . drwxr-xr-x+ 14 ianhin users 4096 May 9 11:14 .. drwxr-xr-x+ 29 ianhin users 4096 Apr 25 17:20 arrangements }}} then cmd is inserted into the '$cmd -a' string and ls and -l are treated as separate words, just like -a, and the shell then correctly executes the command. However, if I do {{{ [ianhin@login-damiana EinsteinToolkit]$ cmd='var=val ls -l' [ianhin@login-damiana EinsteinToolkit]$ $cmd -a -bash: var=val: command not found }}} this does not happen. I thought that unquoted parameter expansions like $cmd here would be expanded before word-splitting, so the result would be split. But it looks like environment variable assignments are performed before even parameter substitution, meaning that you cannot include these variable assignments in expanded variables. See http://www.gnu.org/software/bash/manual/bashref.html#Executing-Commands:

When a simple command is executed, the shell performs the following

expansions, assignments, and redirections, from left to right.

The words that the parser has marked as variable assignments (those

preceding the command name) and redirections are saved for later processing.

The words that are not variable assignments or redirections are expanded

(see Shell Expansions). If any words remain after expansion, the first word is taken to be the name of the command and the remaining words are the arguments.

Redirections are performed as described above (see Redirections). The text after the ‘=’ in each variable assignment undergoes tilde

expansion, parameter expansion, command substitution, arithmetic expansion, and quote removal before being assigned to the variable.

This suggests that we need to first expand the parameter, then use eval to execute the command as a whole:

{{{ cmd="${MAKE} @CONFIGURATION@-testsuite PROMPT=no" eval $cmd }}}

This works for me on Datura. Is this a good solution?

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: reopened Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: | Keywords: -------------------------+--------------------------------------------------

Comment (by hinder):

(after a discussion with Erik)

var=val cmd arg... is standard bash, and this is a bash script. However, due to the issue with parameter expansion being applied after the var=val items are read, it is not good to use it here. A better approach would be to use env. In this case though, this variable should be set in an envsetup entry in the machine definition. I think I added it to the make command before envsetup existed. I am testing this solution now.

#1364: be mor careful extracting $MAKE in RunTestSuite -------------------------+-------------------------------------------------- Reporter: rhaas | Owner: eschnett Type: defect | Status: closed Priority: critical | Milestone: ET_2013_05 Component: SimFactory | Version: Resolution: fixed | Keywords: -------------------------+-------------------------------------------------- Changes (by hinder):

* status: reopened => closed * resolution: => fixed

Comment:

The datura machine file has been updated to use envsetup. We should use only <command> <args>... in the make command, not arbitrary bash fragments. If you want to use environment variables, you can use the "env" command as above.