Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow --compiler and --mpilib #1804

Merged
merged 3 commits into from
Aug 11, 2017
Merged

Conversation

jedwards4b
Copy link
Contributor

@jedwards4b jedwards4b commented Aug 10, 2017

This PR allows ctest to use the new --compiler and --mpilib options in scripts_regression_tests.py
by using env variables CIME_COMPILER and CIME_MPILIB

Test suite: CIME_COMPILER=nag ./cime.ctest.hobart
Test baseline:
Test namelist changes:
Test status: bit for bit
Fixes #1770
Fixes #1281

User interface changes?:

Update gh-pages html (Y/N)?:

Code review:

Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this, and for adding cron jobs with some alternative compilers!

My one comment relates to how CIME_COMPILER and CIME_MPILIB are passed in to cmake. I find it confusing when things are passed in via environment variables, and this can cause hard-to-track-down problems when someone happens to have something set in their environment for some other reason.

I feel like the better way to pass things in to cmake is to use something like:

cmake -D CIME_COMPILER=something

Would it be feasible to use that approach here? If there is some large difficulty in doing this, I can live with the current implementation, but I'd prefer to avoid environment variables if possible.

@jedwards4b
Copy link
Contributor Author

I gave them CIME_ prefix because that way it's unlikely to conflict with anything else. I don't think that this is worth the effort it would take to change, but you are welcome if you want to.

@billsacks
Copy link
Member

I gave them CIME_ prefix because that way it's unlikely to conflict with anything else. I don't think that this is worth the effort it would take to change, but you are welcome if you want to.

Okay, I can live with this, but can you please add documentation at the top of the two cmake files documenting the usage and the possible environment variables that can affect operation of these scripts? Once you have done that, I'll accept this PR.

@jedwards4b
Copy link
Contributor Author

added comments

@billsacks billsacks merged commit 7f91804 into ESMCI:master Aug 11, 2017
@jedwards4b jedwards4b deleted the allow_new_opts_ctest branch August 11, 2017 17:03
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.

3 participants