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

CMake Policy 0077 #269

Merged
merged 2 commits into from
Mar 19, 2020
Merged

CMake Policy 0077 #269

merged 2 commits into from
Mar 19, 2020

Conversation

grantfirl
Copy link
Collaborator

@grantfirl grantfirl commented Mar 18, 2020

set a value for cmake policy 0077 in order to not reset the STATIC variable within CMakeLists.txt when building with the SCM

needed for NCAR/ccpp-scm#176

…riable within CMakeLists.txt when building with the SCM
Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Could you describe briefly what the issue is, please? I was reading https://cmake.org/cmake/help/git-stage/policy/CMP0077.html but I am getting confused with the abstract terms "cached variable" versus "normal variable" ...

Oh, and since this feature will not be needed any more in the near future (when the dynamic build is removed entirely), could you leave a comment in the newly added policy so that a grep for "STATIC" will find it? Thanks!

@grantfirl
Copy link
Collaborator Author

Could you describe briefly what the issue is, please? I was reading https://cmake.org/cmake/help/git-stage/policy/CMP0077.html but I am getting confused with the abstract terms "cached variable" versus "normal variable" ...

Oh, and since this feature will not be needed any more in the near future (when the dynamic build is removed entirely), could you leave a comment in the newly added policy so that a grep for "STATIC" will find it? Thanks!

Sure thing. It seems as though when we hardcoded the STATIC variable to be set in the SCM CMakeLists.txt (see NCAR/ccpp-scm@2cd86f6), that when cmake reaches the CMakeLists.txt for ccpp-framework, it wants to clear the STATIC variable coming in. This is what is produced when running cmake:

CMake Warning (dev) at /volumes/d1/grantf/code/gmtb-scm/ccpp/framework/CMakeLists.txt:45 (option):
Policy CMP0077 is not set: option() honors normal variables. Run "cmake
--help-policy CMP0077" for policy details. Use the cmake_policy command to
set the policy and suppress this warning.

For compatibility with older versions of CMake, option is clearing the
normal variable 'STATIC'.

It of course, doesn't immediately cause a failure, but the ccpp-framework CMakeLists.txt then assumes a dynamic build. Thus, when I go to compile the code, it is trying to compile the CCPP framework tests, and that compilation fails with messages like:
Error: Symbol 'ccpp_physics_init' referenced at (1) not found in module 'ccpp_api' due to the dynamic/static mixup.

Adding the CMP0077 policy prevents STATIC from being cleared. You're right that this should go away once we remove the dynamic build. I will add a comment with the word "STATIC" in it so that we can remove this when the dynamic build goes completely.

@climbfuji
Copy link
Collaborator

Thanks! We will need to test this with the UFS for both dynamic and static builds, I am afraid.

@climbfuji
Copy link
Collaborator

So, I don't understand how this worked for the public release code of SCM? And the UFS?

@grantfirl
Copy link
Collaborator Author

So, I don't understand how this worked for the public release code of SCM? And the UFS?

Yes, I was confused by this too. I think it is because the changes in 55e861f#diff-af3b638bc2a3e6c650974192a53c7291 is not in the released code.

@climbfuji
Copy link
Collaborator

This makes total sense, thanks for clarifying.

@grantfirl
Copy link
Collaborator Author

Thanks! We will need to test this with the UFS for both dynamic and static builds, I am afraid.

@climbfuji For purposes of testing, would it be sufficient to, say, run the rt_ccpp_gsd.conf through rt.sh and just keep the compile steps? Any reason to actually run?

@climbfuji
Copy link
Collaborator

climbfuji commented Mar 18, 2020 via email

@grantfirl
Copy link
Collaborator Author

I think the best would be to first compile the model in three ways: - compile.sh with dynamic CCPP - compile.sh with static CCPP - compile_cmake.sh with static CCPP (compile_cmake.sh doesn't work with dynamic CCPP) Then run a one static CCPP test of rt.conf against the official baseline. Then create a baseline with static CCPP for one test in rt_ccpp_dtc.conf, then verify against it using rt_ccpp_dtc.conf in both static and dynamic mode. Or, even easier: wait until I merge Dustin's RRTMGP PRs, because I can pull this change into the PR and test it with all the bells and whistles turned on. Should be happening this week. Dom

On Mar 18, 2020, at 2:32 PM, grantfirl @.***> wrote: Thanks! We will need to test this with the UFS for both dynamic and static builds, I am afraid. @climbfuji https://github.com/climbfuji For purposes of testing, would it be sufficient to, say, run the rt_ccpp_gsd.conf through rt.sh and just keep the compile steps? Any reason to actually run? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#269 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5C2RMPW4INJVCN7HEN7O3RIEVXJANCNFSM4LODA2GA.

OK, that is way more testing for this change than I thought was necessary, but I will try to implement your plan. I would love to just let you test it with the RRTMGP stuff, but I honestly get rusty with running FV3 RTs and it would probably be good for me to run through this as a refresher for testing other stuff that I need to soon.

@grantfirl
Copy link
Collaborator Author

Testing procedure so far:

git clone --recursive -b dtc/develop http://github.com/NCAR/ufs-weather-model
check out grantfirl:add_policy_0077 branch of ccpp-framework
./compile.sh $PWD/../FV3 hera.intel 'CCPP=Y' | tee compile_dynamic_ccpp.log
./compile.sh $PWD/../FV3 hera.intel 'CCPP=Y STATIC=Y SUITES=FV3_GFS_v15p2' | tee compile_static_ccpp.log
./compile_cmake.sh $PWD/.. hera.intel 'CCPP=Y STATIC=Y SUITES=FV3_GFS_v15p2' | tee compile_cmake_static_ccpp.log

All OK so far:
compile_dynamic_ccpp.log
compile_static_ccpp.log
compile_cmake_static_ccpp.log

@climbfuji
Copy link
Collaborator

Great! Just that you know, when running the tests on Cheyenne, you need to use the NCAR dtc/develop version of ufs-weather-model (which you did), and then pick regression tests in rt_intel.conf (this is a Cheyenne-only version), because this was used to generate the baseline.

@climbfuji
Copy link
Collaborator

And, I am still creating the baseline corresponding to the latest version of dtc/develop (date tag 20200317 for RTPWD in rt.sh). Sorry, this is usually done post-commit on cheyenne.

@grantfirl
Copy link
Collaborator Author

I only used hera.intel. All RTs were successful. All tests used FV3_GFS_v15p2.

Using the test branch:
./rt.sh -l rt_cmp77.conf 2>&1 | tee rt_cmp77.log

Switched to dtc/develop branch to create:
./rt.sh -l rt_ccpp_dtc_cmp77.conf -c fv3 2>&1 | tee rt_ccpp_dtc_cmp77.log

Switched back to test branch to verify:
./rt.sh -l rt_ccpp_dtc_cmp77.conf -m 2>&1 | tee rt_ccpp_dtc_cmp77_verify_static.log
./rt.sh -l rt_ccpp_dtc_cmp77.conf -m 2>&1 | tee rt_ccpp_dtc_cmp77_verify_dynamic.log
rt_cmp77.log
rt_ccpp_dtc_cmp77_create.log
rt_ccpp_dtc_cmp77_verify_static.log
rt_ccpp_dtc_cmp77_verify_dynamic.log

@grantfirl grantfirl merged commit 4ffb637 into NCAR:dtc/develop Mar 19, 2020
@climbfuji
Copy link
Collaborator

Thanks! I already pulled this into the RRTMGP PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants