Fix errors in cice.setup script and add travis tests to catch this sooner #406
Conversation
|
This is ready to merge. I fixed the error and also added some tests to travis to check that cice.setup --case and cice.setup --test complete successfully (but I don't build or run those case, it's just a scripts check). I also verified that travis would have failed with the new tests. Since this is critical, I will merge this now. If there are still problems, we can execute another PR later. |
| else if (-e ${ICE_SCRIPTS}/tests/${tarray}.ts) then | ||
| cat ${ICE_SCRIPTS}/tests/${tarray}.ts >> $tsfile | ||
| else | ||
| echo "${0}: ERROR, cannot find testsuite file ${tarray}, also checked ${ICE_SCRIPTS}/tests/${tarray}.ts" | ||
| exit -1 |
There was a problem hiding this comment.
this just simplifies the logic, right ?
There was a problem hiding this comment.
correct. noticed this as I was debugging.
| @@ -422,6 +420,7 @@ set dorun = false | |||
| set dosubmit = true | |||
| if (\$?SUITE_BUILD) then | |||
| set dobuild = "\${SUITE_BUILD}" | |||
| endif | |||
There was a problem hiding this comment.
so from what I understand and tested this forgotten endif was the cause of the
else: endif not found.
error. I am pretty surprised, since this is inside the heredoc, so it is just some text written to a file, still the shell running cice.setup dies with a syntax error... the mysteries of the C shell!
There was a problem hiding this comment.
exactly. this is pretty shocking. The endif is missing in the job.submit script yet that script never complained. This is exactly what I had been testing all along. Then when we run a non-suite with cice.setup, it failed due to the missing endif in the part of the script that was generating the job.submit suite script. I really cannot believe how this bug was manifested in the end.
There was a problem hiding this comment.
This kind of weird behavior usually means there's some other bug, doesn't it? Is it possible to step through the script to find out why it hit that block of code? Or is this a "compiler" bug?
There was a problem hiding this comment.
I think this weird behavior is due to the peculiarities of the C shell. As far as I know it is not possible to step a shell script...
So yes I think this is could be classified as a bug in the C shell.
|
FWIW, I had to manually fix this in the basalstress tests I just ran (I did not commit this for the pull request). Otherwise cice.setup crashed for me. How it worked before is a good question.
David
David A. Hebert
Ocean Sciences Division, Code 7322
Naval Research Laboratory
Stennis Space Center, MS 39529
(228)688-5846; DSN 828-5846
http://www.nrl.navy.mil <http://www.nrl.navy.mil/>
From: Philippe Blain <notifications@github.com>
Sent: Tuesday, February 18, 2020 10:38
To: CICE-Consortium/CICE <CICE@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Subject: Re: [CICE-Consortium/CICE] Fix errors in cice.setup script and add travis tests to catch this sooner (#406)
@phil-blain commented on this pull request.
_____
In cice.setup <#406 (comment)> :
@@ -422,6 +420,7 @@ set dorun = false
set dosubmit = true
if (\$?SUITE_BUILD) then
set dobuild = "\${SUITE_BUILD}"
+endif
I think this weird behavior is due to the peculiarities of the C shell. As far as I know it is not possible to step a shell script...
So yes I think this is could be classified as a bug in the C shell.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#406?email_source=notifications&email_token=AE52VPFCV5OWTWNUHMWVRL3RDQFH7A5CNFSM4KW22E2KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCV6LDGY#discussion_r380792948> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE52VPHNPZKZIBQQIRQXG2TRDQFH7ANCNFSM4KW22E2A> . <https://github.com/notifications/beacon/AE52VPDZ5WWNXN2FXJVPMBTRDQFH7A5CNFSM4KW22E2KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCV6LDGY.gif>
|
fix error in cice.setup script introduced in update suite.submit script and add new controls and cice.setup option… #395 and documented in cice.setup broken for cases and tests since PR #395 #405
apcraig
Tested case and test on izumi. Also added test to travisCI. Ran a quick suite on izumi,
https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#b2ca7858f35d20b2ba00a1e73d3ddfe2299c9f6f
I am first testing if the new travis tests will catch this then I am going to implement the fix.
I verified that the new travis tests would have failed in this case.
I have also fixed the error.