scripts: add support for setting ICE_CPPDEFS in set_env.* options#448
scripts: add support for setting ICE_CPPDEFS in set_env.* options#448apcraig merged 5 commits intoCICE-Consortium:masterfrom
ICE_CPPDEFS in set_env.* options#448Conversation
Let's not keep these commented variables in cice.settings.mods, they've been in the namelist for a while now.
Other scripts called by cice.setup output "running <script name>", but this convention was not respected by cice.batch.csh. Uniformize the output to respect the convention.
Some `sed` implementations (notably, on macOS) do not handle using
the `-i` option without a backup file. As such, the following argument
('-e') is used as an extension for the backup file, resulting in a
file named ${filename}.check-e which does not get
deleted and stays in the case directory.
Add a backup extension and remove the backup file at the end of the
script.
apcraig
left a comment
There was a problem hiding this comment.
I think documentation needs a minor update. In "Table of CICE Settings". Also maybe in the build section.
| echo "running cice.batch.csh (creating ${1})" | ||
| else | ||
| echo ${0:t} | ||
| echo "running cice.batch.csh" |
There was a problem hiding this comment.
I'm curious why ${0:t} was removed in favor of a hardwired filename here.
There was a problem hiding this comment.
I can always change it to echo "running ${0:t}"
There was a problem hiding this comment.
I see now and find it curious. I wonder why we're not using $0 in all the scripts. I might look into this at some point, but for now, I agree this change is a good idea to make it consistent. Thanks.
|
OK. I'll update the doc. thanks. |
006248b to
e291c90
Compare
|
@apcraig I've updated the documentation. I'll uniformize the wording with CICE-Consortium/Icepack#319 once we converge there. |
|
Looks good. I have updated the Icepack CPPDEFS documentation and think they're largely consistent. If you agree, I'll approve the PR and merge it. |
Allow using the present case/test configuration mechanism (set_env.* in configuration/scripts/options/) to add preprocessor macros. cice.setup already allows for modifying cice.setttings using set_env.* options, so the only change needed is to add a default value for `ICE_CPPDEFS` in the default cice.settings file, and not resetting this variable in cice.build. Note that contrary to what was proposed in CICE-Consortium#445, we do not implement a similar ICE_SLIBS variable, that could be used to add linker flags (such as optional libraries), because the way in which libraries are made accessible and linked with an executable vary depending on the machine in use, so such modifications are best made in the Macros.<machine>_<compiler> and env.<machine>_<compiler> files. Closes CICE-Consortium#445
e291c90 to
4f5be78
Compare
|
@apcraig I've also added the "using the syntax |
|
Great, I'll merge as soon as the checks are complete. Looks like readthedocs is struggling today, we may need to retrigger the build in a while. I am having the same problems with readthedocs with another PR. |
PR checklist
add support for setting
ICE_CPPDEFSin set_env.* optionsP. Blain
I tested that the feature works by adding a
set_env.lapackfile with contentsetenv ICE_CPPDEFS CICE_USE_LAPACKand checking that the CPP is indeed used at compile time.
Allow using the present case/test configuration mechanism
(set_env.* in configuration/scripts/options/) to add preprocessor macros.
cice.setup already allows for modifying cice.setttings using set_env.*
options, so the only change needed is to add a default value for
ICE_CPPDEFSin the default cice.settings file, and not resettingthis variable in cice.build.
Note that contrary to what was proposed in #445, we
do not implement a similar ICE_SLIBS variable, that could be used to add
linker flags (such as optional libraries), because the way in which
libraries are made accessible and linked with an executable vary
depending on the machine in use, so such modifications are best made in
the
Macros.<machine>_<compiler>andenv.<machine>_<compiler>files.