Skip to content

Adds "makedep" script to replace mkmf#135

Merged
marshallward merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
adcroft:makedep
Jun 17, 2022
Merged

Adds "makedep" script to replace mkmf#135
marshallward merged 2 commits into
NOAA-GFDL:dev/gfdlfrom
adcroft:makedep

Conversation

@adcroft
Copy link
Copy Markdown
Member

@adcroft adcroft commented May 31, 2022

Adds makedep script that constructs the link stage for as many programs as found, links only the necessary object files*, and is coded solely in bash thus removing an external dependency (namely mkmf). The addition of this script is the bulk of this commit.

Code changes:

  • when more than one program is encountered the executable is given the name following "program". Since all the programs were named "MOM_main" I have changed them to provide unique names:
    • MOM_sum_driver.F90, "program MOM_main" has been changed to "MOM_sum_driver"
    • solo_driver/MOM_driver.F90, "program MOM_main" has been changed to "MOM6"

Script changes:

  • Makefile.in now has a target "depend" to generate dependencies using makedep
  • configure.ac no longer checks for list_paths and mkmf
  • configure.ac now invokes "make depend" in place of mkmf
  • Added target "unit" to .testing/Makefile to build all programs in config_src/drivers/unit_drivers

Ugliness:

  • I had to add a -f option to makedep to handle FMS non-standard macros
    • To compile FMS, the dependencies Makefile is passed CPPDEFS in addition to CPPFLAGS.
    • The first version of makedep was consistent with the standard gmake rules which were sufficient to build MOM6. Adding -f "rule command" allows FMS to be built:
      makemake -f '$(FC) $(FFLAGS) $(CPPFLAGS) $(CPPDEFS) -c $<' -x libFMS.a ../src
  • .inc suffix is included when searching for include directories
    • FMS has includes of .inc files which modify the search path passed to /lib/cpp .
  • Handling of badly formatted comments when searching for modules
    • FMS fm_util.F90, that generates fm_util_mod.mod, has some odd strings in a comment on the module declaration line. This was causing wierdness in the script.
  • Not just Fortran dependencies
    • makedep needs to also generate rules for C files in order to build FMS

Todo:
[ ] A work around is used for TEOS10 (gsw_) functions that are in separate object files even though accessed via a module (WTFortran!!!)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 31, 2022

Codecov Report

Merging #135 (cff0224) into dev/gfdl (e5580e3) will increase coverage by 0.81%.
The diff coverage is 100.00%.

❗ Current head cff0224 differs from pull request most recent head f7d0f9c. Consider uploading reports for the commit f7d0f9c to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #135      +/-   ##
============================================
+ Coverage     33.45%   34.26%   +0.81%     
============================================
  Files           262      257       -5     
  Lines         71434    69742    -1692     
  Branches      13339    12913     -426     
============================================
  Hits          23896    23896              
+ Misses        43064    41372    -1692     
  Partials       4474     4474              
Impacted Files Coverage Δ
config_src/drivers/solo_driver/MOM_driver.F90 50.65% <100.00%> (ø)
src/user/MOM_controlled_forcing.F90
config_src/external/ODA_hooks/write_ocean_obs.F90
...g_src/external/GFDL_ocean_BGC/FMS_coupler_util.F90
...external/stochastic_physics/get_stochy_pattern.F90
src/ice_shelf/MOM_marine_ice.F90
src/user/MOM_wave_interface.F90 0.57% <0.00%> (+<0.01%) ⬆️
src/core/MOM_open_boundary.F90 23.01% <0.00%> (+0.01%) ⬆️
src/equation_of_state/MOM_EOS.F90 23.57% <0.00%> (+0.04%) ⬆️
src/tracer/MOM_neutral_diffusion.F90 62.92% <0.00%> (+0.05%) ⬆️
... and 56 more

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 e5580e3...f7d0f9c. Read the comment docs.

@adcroft
Copy link
Copy Markdown
Member Author

adcroft commented Jun 1, 2022

FWIW, regression and performance tests are failing because they depend on mkmf being installed which is no longer the case. These are expected fails.

Comment thread ac/Makefile.in Outdated

.PHONY: depend
depend:
../../../ac/makedep -o Makefile.dep $(SRC_DIRS)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this depend rule? Should it just be Makefile.dep?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This target allows a single target to be used to generate the dependencies either when autoconf is making the Makefile or after the fact. make depend is the "old-timer" conventional way to do this. I think it better that autoconf use make depend than requiring we re-do the autoconf when changing module dependencies.

Copy link
Copy Markdown
Member

@marshallward marshallward Jun 7, 2022

Choose a reason for hiding this comment

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

OK, I see what you are going for now. I agree with it.

But maybe could do something like this (if not too pedantic):

.PHONY: depend
depend: Makefile.dep

Makefile.dep: $(SRC_DIRS)
	../../../ac/makedep -o $@ $^

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I like it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The ../../.. is wrong. It needs to be something like ${srcdir}/ac

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@srcdir@/ac/makedep is working for me

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have a fix for this and a few other things, will send it to your branch later today.

https://github.com/marshallward/MOM6/tree/makedep

@marshallward
Copy link
Copy Markdown
Member

Currently, Makefile.in acts as an autoconf->mkmf translator, in order to account for the mkmf Makefiles. But since we are replacing mkmf, we can just use the autoconf variables directly.

So I think there might be some opportunity to unwind some of the "non-standard macros" that you refer.

For example:

  • The mkmf CPPDEFS currently uses the autoconf DEFS, which provides some specific management of -D type flags. So I think you could just use DEFS in the script.
  • Autoconf reserves FFLAGS for Fortran 77-style code, and FCFLAGS for Fortran90+, whereas mkmf still uses FFLAGS. We could just replace FFLAGS with FCFLAGS
  • mkmf incorrectly bundles the LDFLAGS and LIBS flags into a single variable, which is actually rather problematic because sometimes you want, e.g. -Lmy_dir to precede the output and -lmylib to follow the output. Since the autoconf output already provides sensible LDFLAGS and LIBS, we can actually use them correctly in makedep output.

Those three things came quickly to mind, there may be others. In general, I'd suggest purging any mkmf-y variables and using native autoconf ones.

@adcroft
Copy link
Copy Markdown
Member Author

adcroft commented Jun 7, 2022

Currently, Makefile.in acts as an autoconf->mkmf translator, in order to account for the mkmf Makefiles. But since we are replacing mkmf, we can just use the autoconf variables directly.

So I think there might be some opportunity to unwind some of the "non-standard macros" that you refer.

Good idea.

adcroft and others added 2 commits June 16, 2022 10:25
`mkmf` is an external dependency that that uses a multi-stage
approach to building a Makefile with dependencies but fails to yield
an optimal link stage (it links everything, including unused modules).

`makedep` is a bash script that constructs the link stage for as many
programs as found, links only the necessary object files*, and is coded
solely in bash thus removing an external dependency. The addition of this
script is the bulk of this commit.

Code changes:
- when more than one program is encountered the executable is given
  the name following "program". Since all the programs were named
  "MOM_main" I have had to change them to provide unique names:
  - MOM_sum_driver.F90, "program MOM_main" has been changed to
    "MOM_sum_driver"
  - solo_driver/MOM_driver.F90, "program MOM_main" has been
    changed to "MOM6"

Script changes:
- Makefile.in now has a target "depend" to generate dependencies
  using makedep
- configure.ac no longer checks for list_paths and mkmf
- configure.ac now invokes "make depend" in place of mkmf
- Added target "unit" to .testing/Makefile to build all programs
  in config_src/drivers/unit_drivers

Ugliness:
- I had to add a -f option to makedep to handle FMS non-standard macros
  - To compile FMS, the dependencies Makefile is passed CPPDEFS in addition
    to CPPFLAGS.
  - The first version of makedep was consistent with the standard gmake
    rules which were sufficient to build MOM6. Adding -f "rule command"
    allows FMS to be built:
      makemake -f '$(FC) $(FFLAGS) $(CPPFLAGS) $(CPPDEFS) -c $<' -x libFMS.a ../src
- .inc suffix is included when searching for include directories
  - FMS has includes of .inc files which modify the search path passed
    to /lib/cpp .
- Handling of badly formatted comments when searching for modules
  - FMS fm_util.F90, that generates fm_util_mod.mod, has some odd strings
    in a comment on the module declaration line. This was causing wierdness
    in the script.
- Not just Fortran dependencies
  - makedep needs to also generate rules for C files in order to
    build FMS

Todo:
[ ] *A work around is used for TEOS10 (gsw_*) functions that are in separate
    object files even though accessed via a module (WTFortran!!!)
* Autoconf: Fix makedep path

The current path of makedep in the autoconf build assumes a directory tree as in .testing.  This patch uses the 
generalized @srcdir@ to support more general autoconf builds.

Other minor changes:
* The `depend` rule was split into an explicit Makefile.dep rule and a phony rule to support `make depend`

* SRC_DIRS shell assignment is replaced with an Autoconf macro.

* A "self-generate" rule was added to `Makefile.in`, so that changes to `Makefile.in` do not trigger a full `./configure` run and regeneration of `.config.status`.

  This could possibly be extended to support `make depend` but let's first see how this one goes.

* Autoconf: makedep uses autoconf var conventions

This patch changes the makedep script to use autoconf environment variable conventions rather than mkmf ones:

* FCFLAGS in place of FFLAGS

* DEFS in place of CPPDEFS

* LDFLAGS and LIBS rather than just LDFLAGS
  (NOTE: This differs from Makefile's LDLFLAGS/LDLIBS)

This also allowed us to remove the custom build rule in the FMS build. Note that we now use an autoconf-friendly rule, rather than the default Makefile rule (which was arguably for fixed-format Fortran anyway).

The description of autoconf->mkmf translation from the Makefile templates has also been removed, since they're no longer relevant.

Some other minor changes in this build:

* The `make depend` rule was added to the FMS Makefile template.

* @srcdir@ is directly passed to FMS makedep, rather than identically re-defining it in a variable.

* Testing: Resolve makedep paths

This patch resolves some issues relating to finding the path to makedep in both .testing and more generalized autoconf builds.

An explicit autoconf test for makedep has been added, with a default path which includes the `ac` directory relative to `deps`.

The .testing directory, which does not lie within `ac`, instead modifies the PATH to allow autoconf to find makedep.

The absolute path is determined and substituted into the appropriate Makefile.in template.

Some redundant operations in .testing/Makefile have been removed, but I suspect there are even more.  Much of the structure required to support mkmf and list_paths is probably no longer needed.
@marshallward
Copy link
Copy Markdown
Member

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

Good bye, mkmf.

@marshallward marshallward merged commit 4bcc849 into NOAA-GFDL:dev/gfdl Jun 17, 2022
@adcroft adcroft deleted the makedep branch July 4, 2022 18:35
marshallward pushed a commit that referenced this pull request Sep 26, 2024
initialize cpl_scalar field when created
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