be mor careful extracting $MAKE in RunTestSuite

Issue #1364 closed

Roland Haas created an issue 2013-05-20

currently sim --testsuite fails on kraken since the extracted $MAKE command is incorrect since it splits at the second "=" in the make definition:

make = env LD_LIBRARY_PATH="${LD_LIBRARY_PATH}:/opt/gcc/4.5.1/snos/lib64:/opt/gcc/mpc/0.8.1/lib" make -j2

the attached patch rectifies this. Tested on kraken and lonestar.

Keyword:

runtestsuite.patch

Comments (18)

Roland Haas reporter
- changed status to open
- removed comment
- 2013-05-20T12:45:02+00:00
Erik Schnetter
- changed status to open
- removed comment
Approved.
- 2013-05-20T13:38:04+00:00
Roland Haas reporter
- changed status to resolved
- removed comment
applied as rev 2101 as of ET_2013_05 (already in trunk in rev 2100).
- 2013-05-20T15:14:18+00:00

Ian Hinder

changed status to open
removed comment

SimFactory can no longer run the tests on datura. The error is

 /lustre/datura/ianhin/simulations/ettests_1proc/output-0000/RunTestSuite: line 21: /cluster/intel/licenses: is a directory

The runscript has

MAKE=$(/home/ianhin/Cactus/EinsteinToolkit/simfactory/bin/sim print-mdb-entry datura | grep '^make' | sed -e 's/^.*= *//;s/ *#.*$//')

${MAKE} ET_2013_06-testsuite PROMPT=no

The make command is

make = INTEL_LICENSE_FILE=/cluster/intel/licenses make -j12

Could this be related to this change?

2013-05-22T04:27:26+00:00

Roland Haas reporter
- removed comment
The fix is designed to cure precisely this problem. Are you still seeing this?

Executing the dature line on my worstation (2013_05, release canditate) I get:
```
rhaas@horizon:.../gr/ET_2013_05$ simfactory/bin/sim print-mdb-entry datura | grep '^make' | sed -e 's/^make *= *//;s/ *#.*$//'
INTEL_LICENSE_FILE=/cluster/intel/licenses make -j12
```
which looks like the correct answer as far as I understand it.
- 2013-05-22T04:35:35+00:00
Frank Löffler
- removed comment
We need to test these changes on all machines, which takes time. When we did this, we can update the release, if necessary.
- 2013-05-22T13:51:04+00:00
Frank Löffler
- removed comment
We should find a solution that works everywhere, as soon as possible, and backport it after testing.
- 2013-05-26T11:44:40+00:00
Roland Haas reporter
- removed comment
I am confused by Ian's comment, and don't know if he says that it fails only with the sed change to include 'sed /^make/' or only without that change. Ie. is this currently with the code in trunk failing on datura?
- 2013-05-26T11:52:26+00:00
Ian Hinder
- removed comment
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.
- 2013-05-26T13:20:06+00:00
Erik Schnetter
- removed comment
I think we should wait with reverting until we have a full bug report that lets us see what the problem is, and how to revert it. From an abstract point of view, the current code seems correct, while the previous version was obviously incorrect.
- 2013-05-26T13:48:53+00:00
Ian Hinder
- removed comment
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.
- 2013-05-26T14:51:40+00:00
Roland Haas reporter
- removed comment
To be very explicit. There are two commits that are involved, one is rev 2065 'Use MDB entry for "make" when running test suite' would give you a file simfactory/bin/RunTestSuite with a make line like the one Ian quotes. This revision fails with the error Ian describes also on Kraken. Therefore I committed rev 2101 "be more careful extraction $MAKE in RunTestSuite" which gives a file simfactory/bin/RunTestSuite where the MAKE= line reads:
```
MAKE=$(@SOURCEDIR@/simfactory/bin/sim print-mdb-entry @MACHINE@ | grep '^make' | sed -e 's/^make *= *//;s/ *#.*$//')
```
which I hoped would avoid the error. Please note that 2101 is after the creation of the release branch.
- 2013-05-26T15:30:29+00:00
Ian Hinder
- removed comment
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?
- 2013-05-26T16:53:55+00:00
Erik Schnetter
- removed comment
Datura's make command is "make = INTEL_LICENSE_FILE=/cluster/intel/licenses make -j12". This assumes that "var=val" means that var is set to val while executing the following command. I don't know whether this is standard; apparently it does not work in all cases.

Can you try using the "env" command for this? This would then look like
```
make = env INTEL_LICENSE_FILE=/cluster/intel/licenses make -j12
```
which we use everywhere else for setting environment variables in the MDB.

Adding on additional "eval" here is dangerous, because this means that the command will be evaluated twice. This would make quoting difficult.
- 2013-05-26T17:06:56+00:00
Ian Hinder
- removed comment
(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.
- 2013-05-26T17:29:18+00:00
Roland Haas reporter
- removed comment
The kraken entry for make (which is the machine I needed the fix for) uses "env" (as well as an envsetup to load the intel compiler module).
- 2013-05-26T18:02:28+00:00
Ian Hinder
- changed status to resolved
- removed 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.
- 2013-10-28T10:34:55+00:00
Roland Haas reporter
- edited description
- changed status to closed
- 2019-02-21T20:06:25+00:00
Log in to comment

Assignee: Erik Schnetter

Type: bug

Priority: minor

Status: closed

Component: SimFactory

Milestone: ET_2013_05

Version: –

Votes: 0

Watchers: 0