Skip to content

Re-developed makedep in python #158

Merged
marshallward merged 5 commits into
NOAA-GFDL:dev/gfdlfrom
adcroft:makedep
Jul 8, 2022
Merged

Re-developed makedep in python #158
marshallward merged 5 commits into
NOAA-GFDL:dev/gfdlfrom
adcroft:makedep

Conversation

@adcroft
Copy link
Copy Markdown
Member

@adcroft adcroft commented Jul 6, 2022

  • Works with system python (2.7) and later (tested with 3.9)
  • Processes #include without invoking cpp
    • Currently naive regex without honoring/interpretting CPP macros
  • Calculates link dependencies directly rather than invoking "make"
    to infer the link list

Changes relative to the bash version of makedep:

  • No longer adding/using a macro "SRC_DIRS" since it was only used to record the arguments for regenerating the makefile. Now we record the command line to achieve the same re-run ability.
  • Adds new option "-e" to link all externals.
    • This was the default before but I think in time we can make this approach more intelligent and figure out which functions/subroutines are used elsewhere.
  • Processes F90 include to build dependencies
    • Currently not tested with nested includes

Comments:

  • As usual, the bizarre self-referencing in drifters.F90 needed some special handling. The self-referencing is associated with code in CPP block to generate local test programs. Now, we filter out circular self-references rather than complain like make does. A large-separation circular dependence will not be caught and lead to unpredictable behavior. Such circular dependence is not something that makes sense to support or allow.
  • Using only native python featues, no packages, to avoid the "package dependence" we are trying to avoid with makedep.
  • This python version appears to be of orders of magnitude faster than the bash version. Should have started here ...

Todo:
[ ] Improve some list/dictionary comprehensions. We use list comprehensions quite a lot but a few "clunky" functions remain when the first attempt at a comprehension failed. I never figured out a working dictionary comprehension.
[ ] Add a solution for the need to always link "externals".

adcroft and others added 4 commits July 4, 2022 14:38
- Works with system python (2.7) and later (tested with 3.9)
- Processes #include without invoking cpp
  - Currently naive regex without honoring/interpretting CPP macros
- Calculates link dependencies directly rather than invoking "make"
  to infer the link list

Changes relative to the bash version of makedep:
- No longer adding/using a macro "SRC_DIRS" since it was only used to
  record the arguments for regenerating the makefile. Now we record the
  command line to achieve the same re-run ability.
- Adds new option "-e" to link all externals.
  - This was the default before but I think in time we can make this
    approach more intelligent and figure out which functions/subroutines
    are used elsewhere.
- Processes F90 `include` to build dependencies
  - Currently not tested with nested `include`s

Comments:
- As usual, the bizarre self-referencing in drifters.F90 needed some
  special handling. The self-referencing is associated with code in CPP
  block to generate local test programs. Now, we filter out circular
  self-references rather than complain like make does. A large-separation
  circular dependence will not be caught and lead to unpreditable
  behavior. Such circular dependence is not something that makes sense to
  support or allow.
- Using only native python featues, no packages, to avoid the "package
  dependence" we are trying to avoid with makedep.
- This python version appears to be of orders of magnitude faster than
  the bash version. Should have started here ...

Todo:
[ ] Improve some list/dictionary comprehensions. We use list
    comprehensions quite a lot but a few "clunky" functions remain when
    the first attempt at a comprehension failed. I never figrued out a
    working dictionary comprehension.
[ ] Add a solution for the need to always link "externals".
- @marshallward suggested this fix to recursively follow F90 includes
  when building the list of dependencies.
- Renamed the function from nested_h() to nested_inc() to be better
  describe function

Co-authored-by: Marshall Ward <marshall.ward@gmail.com>
- The function includes_in_path() had many unused arguments and was
  left over from development. It turned out to be easy and clean to
  replace with a list comprehension.
- @marshallward reported that FMS2 has multiple versions of the same
  file int he search path. To avoid fatal errors, we are now allowing
  an ambiguous outcome and simple throwing out a warning that two
  files of the same name were encountered.

Co-authored-by: Marshall Ward <marshall.ward@gmail.com>
@adcroft adcroft requested a review from marshallward July 6, 2022 19:22
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 6, 2022

Codecov Report

Merging #158 (f49a0c7) into dev/gfdl (5cadb72) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           dev/gfdl     #158   +/-   ##
=========================================
  Coverage     34.02%   34.02%           
=========================================
  Files           259      259           
  Lines         70204    70204           
  Branches      13013    13012    -1     
=========================================
  Hits          23884    23884           
  Misses        41822    41822           
  Partials       4498     4498           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cadb72...f49a0c7. Read the comment docs.

Copy link
Copy Markdown
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

This worked for my MacOS patch (with FMS1) and with FMS2 (on Linux), and solves the "bash 3.x" problem on default MacOS systems.

I didn't test MacOS with FMS2 but hopefully we can infer that it will work as well. (The issues with each respective target appear independent.)

@marshallward
Copy link
Copy Markdown
Member

marshallward commented Jul 8, 2022

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16068 ✔️

@marshallward marshallward merged commit 9ecf1a6 into NOAA-GFDL:dev/gfdl Jul 8, 2022
@adcroft adcroft deleted the makedep branch June 26, 2023 17:46
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