Skip to content

scripts: add support for setting ICE_CPPDEFS in set_env.* options#448

Merged
apcraig merged 5 commits intoCICE-Consortium:masterfrom
phil-blain:cppdefs-in-settings
May 26, 2020
Merged

scripts: add support for setting ICE_CPPDEFS in set_env.* options#448
apcraig merged 5 commits intoCICE-Consortium:masterfrom
phil-blain:cppdefs-in-settings

Conversation

@phil-blain
Copy link
Copy Markdown
Member

@phil-blain phil-blain commented May 21, 2020

PR checklist

  • Short (1 sentence) summary of your PR:
    add support for setting ICE_CPPDEFS in set_env.* options
  • Developer(s):
    P. Blain
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    I tested that the feature works by adding a set_env.lapack file with content setenv ICE_CPPDEFS CICE_USE_LAPACK
    and checking that the CPP is indeed used at compile time.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

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 #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.

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.
Copy link
Copy Markdown
Contributor

@apcraig apcraig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why ${0:t} was removed in favor of a hardwired filename here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained in the commit message: e6b3f6f

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can always change it to echo "running ${0:t}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@phil-blain
Copy link
Copy Markdown
Member Author

OK. I'll update the doc. thanks.

@phil-blain phil-blain force-pushed the cppdefs-in-settings branch from 006248b to e291c90 Compare May 26, 2020 14:01
@phil-blain
Copy link
Copy Markdown
Member Author

@apcraig I've updated the documentation. I'll uniformize the wording with CICE-Consortium/Icepack#319 once we converge there.

@apcraig
Copy link
Copy Markdown
Contributor

apcraig commented May 26, 2020

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
@phil-blain phil-blain force-pushed the cppdefs-in-settings branch from e291c90 to 4f5be78 Compare May 26, 2020 18:36
@phil-blain
Copy link
Copy Markdown
Member Author

@apcraig I've also added the "using the syntax -DCICE_MACRO" bit here. This should be ready to merge.

@apcraig
Copy link
Copy Markdown
Contributor

apcraig commented May 26, 2020

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.

@apcraig apcraig merged commit c1eef87 into CICE-Consortium:master May 26, 2020
@phil-blain phil-blain deleted the cppdefs-in-settings branch June 3, 2020 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants