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

Fix buildlib arg order when called as a subprocess #136

Merged
merged 3 commits into from
Dec 8, 2021

Conversation

billsacks
Copy link
Member

Description of changes

The argument order in the buildlib function was wrong when it was called as a subprocess in the course of building the CDEPS shared library.

As noted in ESMCI/cime#4034, this was using argument parsing that is appropriate for components, which (confusingly!) differs from the order of arguments in calls to libraries' buildlibs. The CDEPS shared library is built using the library code, not the component code.

However, this was made trickier by the fact that the same buildlib was being used for both the CDEPS shared library build and the individual data model builds. The latter need the original argument order. It seemed like there was little value gained by using the same buildlib for both, so I have split this into two different files in this PR.

I have confirmed that this works by introducing the following temporary diffs to force the CDEPS buildlibs to be called as subprocesses:

diff --git a/cime_config/buildlib b/cime_config/buildlib
index 27e3776..f92591d 100755
--- a/cime_config/buildlib
+++ b/cime_config/buildlib
@@ -56,7 +56,7 @@ formatter_class=argparse.ArgumentDefaultsHelpFormatter
     return args.buildroot, args.installpath, args.caseroot
 
 
-def buildlib(bldroot, libroot, case):
+def buildlib2(bldroot, libroot, case):
     expect(not bldroot.endswith("obj"),
            "It appears that the main CDEPS buildlib is being called for a specific component\n"
            "(specific data model components should use buildlib_comps)")
@@ -155,7 +155,7 @@ def all_files_under(path, ignoredirs=[]):
 def _main_func(args):
     bldroot, installpath, caseroot = parse_command_line(args, __doc__)
     with Case(caseroot) as case:
-        buildlib(bldroot, installpath, case)
+        buildlib2(bldroot, installpath, case)
 
 if __name__ == "__main__":
     _main_func(sys.argv)
diff --git a/cime_config/buildlib_comps b/cime_config/buildlib_comps
index 7e640eb..34ecad0 100755
--- a/cime_config/buildlib_comps
+++ b/cime_config/buildlib_comps
@@ -21,7 +21,7 @@ from CIME.utils import run_cmd, symlink_force, expect
 
 logger = logging.getLogger(__name__)
 
-def buildlib(bldroot, libroot, case, compname=None):
+def buildlib2(bldroot, libroot, case, compname=None):
     if not compname:
         expect(bldroot.endswith("obj"),
                "It appears that buildlib_comps is being called for the main CDEPS build\n"
@@ -40,7 +40,7 @@ def buildlib(bldroot, libroot, case, compname=None):
 def _main_func(args):
     caseroot, libroot, bldroot = parse_input(args)
     with Case(caseroot) as case:
-        buildlib(bldroot, libroot, case)
+        buildlib2(bldroot, libroot, case)
 
 if __name__ == "__main__":
     _main_func(sys.argv)

Specific notes

Contributors other than yourself, if any:

CMEPS Issues Fixed (include github issue #):
Resolves #104

Are there dependencies on other component PRs

  • CIME (list)
  • CMEPS (list)

Are changes expected to change answers?

  • bit for bit
  • different at roundoff level
  • more substantial

Any User Interface Changes (namelist or namelist defaults changes)?

  • Yes
  • No

Testing performed:

  • (required) aux_cdeps

    • machines and compilers: cheyenne_intel
    • details (e.g. failed tests): All tests passed and were bit-for-bit except for these two that also failed from the baseline (the PEND test is still running after about 4 hours in both; I'm killing it):
    PEND SMS_Ln9_P1.T42_T42.2000_DATM%QIA_SLND_DICE%SSMI_DOCN%DOM_SROF_SGLC_SWAV.cheyenne_intel.datm-scam RUN
    FAIL SMS_Vnuopc_Ld5.TL319_t061.2000_DATM%ERA5_SLND_SICE_SOCN_SROF_SGLC_SWAV_SESP.cheyenne_intel RUN time=9
    
  • (optional) CESM prealpha test

    • machines and compilers
    • details (e.g. failed tests):

Hashes used for testing:

The argument order differs for the cdeps library vs. the individual
components. Thus, we need a different function to parse the command line
for the library. The new parse_command_line function is a direct copy of
the same function from buildlib.csm_share.

To handle this difference in command-line arguments in the two
situations, I have renamed the old file to buildlib_comps (which is
identical to the old buildlib); the individual data model components now
point to that file, leaving buildlib just being used for the cdeps
shared library.

In a following commit I will remove unnecessary code from each of these
files.

Resolves ESCOMP#104
It turns out that, for the most part, each line of code was just needed
for one of these, not both. So, now that the unnecessary code is removed
from each, there is essentially no duplication between buildlib and
buildlib_comps.
@billsacks
Copy link
Member Author

@jedwards4b - I'm pretty sure I did the right thing in removing certain code from buildlib and buildlib_comps, but you should double-check it: My understanding is that, prior to this PR, the shared buildlib script was called for cdeps overall in a scenario that led to the else block, then was called for each data model component in a scenario that led to the if block. So, in this PR, the else block is maintained in buildlib and the if block is now in buildlib_comps.

I recommend viewing the diffs without whitespace diffs: that will make it easier to see what changed in buildlib, since the main change was to out-dent most of the body of the buildlib function due to removing the if/else structure.

@jedwards4b jedwards4b merged commit 37aff69 into ESCOMP:master Dec 8, 2021
@billsacks billsacks deleted the buildlib_arg_order branch December 9, 2021 00:21
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.

Wrong argument order in cdeps buildlib
2 participants