Allow lfric_atm to compile for oasis coupled models#486
Allow lfric_atm to compile for oasis coupled models#486Paul Earnshaw (pdearnshaw) wants to merge 6 commits into
Conversation
…ch actually turned out to be main.
Tagging vn3.1.1 @ 624bfb8
Mike Hobson (mike-hobson)
left a comment
There was a problem hiding this comment.
This is a straightforward change that will more easily allow lfric_atm jobs to run as coupled systems. I just have one comment about the use of #ifdefs.
Mike Hobson (mike-hobson)
left a comment
There was a problem hiding this comment.
Thanks for looking at the ifdefs I'm happy that they stay in. That means the the PR is ready for code review with Alistair Pirrie (@mo-alistairp).
|
Can I check whether this is ready for Code Review, because it seems to have been moved out? |
|
This has passed sci/tech review and is ready for code review - so you're free to start looking at it. I don't know why the tracker script isn't moving the state on - I'll have a word with the script's maintainer. |
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Code Owner (Build System) Review: Just a quick question.
| ifdef COUPLED | ||
| ifeq ("$(COUPLED)","True") |
There was a problem hiding this comment.
Normally we would just test for the existence of the macro. What is the reason for requiring it to have a particular value?
There was a problem hiding this comment.
The COUPLED here is not a macro, it is an environment variable. What we want in this test is to make sure firstly that it has been set, and then that it has been set to True. The idea being that if it is not set, or set to False then the coupled model infrastructure is not applied.
As I understand it the ifdef statement can test for environment variables or keyword-like arguments in the make command, it cannot act on PRE_PROCESS_MACRO entries. So the choice in how to identify if we wanted to compile as a coupled model was to use an optional environment variable.
Happy to consider other ways of doing this.
There was a problem hiding this comment.
The PRE_PROCESS_MACROS are converted into a set of -D<key>=<value> arguments to the compiler. So they will appear to the preprocessor as macros. In which case you can just test on their presence. Unless you think there will be cases where people are setting that value to False.
There's no correctness reason not to do it the way you have, it just runs the risk of people getting unexpected behaviour if they say COUPLED=YES.
I don't mind which you do so feel free to resolve this conversation when you have made a decision.
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Code owner (Build system) further commentary.
| ifdef COUPLED | ||
| ifeq ("$(COUPLED)","True") |
There was a problem hiding this comment.
The PRE_PROCESS_MACROS are converted into a set of -D<key>=<value> arguments to the compiler. So they will appear to the preprocessor as macros. In which case you can just test on their presence. Unless you think there will be cases where people are setting that value to False.
There's no correctness reason not to do it the way you have, it just runs the risk of people getting unexpected behaviour if they say COUPLED=YES.
I don't mind which you do so feel free to resolve this conversation when you have made a decision.
PR Summary
Sci/Tech Reviewer: Mike Hobson (@mike-hobson)
Code Reviewer: Alistair Pirrie (@mo-alistairp)
This PR will allow the lfric_atm app to be used for oasis coupled models. The changes add a conditional statement to the lfric_atm makefile controlled by the environment variable
COUPLED. If set and true then the internal dependency$(CORE_ROOT_DIR)/components/couplingis added to the list ofINTERNAL_DEPENDENCIESwhile also adding to the list ofPRE_PROCESS_MACROSthe valuesMCTandCOUPLED. If not set or not true then nothing happens.The
lfric_atm.f90program file is modified to aF90file in order to allow of pre-processing, and under the macroMCTextra statements are added to the code that allow for the initialistion of the coupled context.No new tests have been added to cover this functionality as these will added a later date to cover all GC6 requirements.
fixes #92 (auto-closes the issue)
Code Quality Checklist
Testing
This branch (and previous versions of) has been used extensively in GC6 testing over a number of releases and has worked fine.
trac.log
Test Suite Results - lfric_apps - lfric_apps_92_coupled_compilation_fix/run1
Suite Information
Task Information
✅ succeeded tasks - 1164
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review