CMake: Improve dependency management for mct and csm_share #5964

jgfouca · 2023-09-27T22:35:24Z

Neither of these packages are cmake packages, so we have to roll our own Find$package.cmake modules. These can be pretty simple for e3sm since we build them ourselves (no worries about version or not found etc).

Also, remove a lot of unused for special handling of clm and stuff for pio building.

[BFB]

bartgol

Mostly good. I have some suggestions, you can decide what's worth adding.

Btw, are you thinking of changing share/util/CMakeLists.txt to actually be a cmake project, rather than whatever it is now?

bartgol · 2023-09-27T22:47:32Z

components/cmake/modules/FindMCT.cmake

+add_library(mct INTERFACE)
+target_link_libraries(mct INTERFACE ${MCT_LIB} ${MPEU_LIB})
+target_include_directories(mct INTERFACE ${INSTALL_SHAREDPATH}/include)
+target_compile_definitions(mct INTERFACE "MCT_INTERFACE")


I grepped for MCT_INTERFACE in the whole repo+submodules, and I can't see where it is used. I see it set in the CPPDEFS var, but never used. Maybe we can get rid of this?

Also not sure where this is use but there is code in CIME to allow MCT or MOAB to be used as the coupler. We need to keep that.

You mean we need to keep ability to use MOAB, not "keep MCT_INTERFACE in CPPDEFS", right?

Also, is there an example in e3sm of a compset using moab instead of mct?

Yes that's what I mean.

Its not in master yet but you can run a moab case on this branch (on Chrysalis only for now) https://github.com/sarich/E3SM/tree/sarich/moab/update-moab-driver
e.g. ./create_test SMS_Vmoab.ne4pg2_oQU480.WCYCL1850NS

MCT_INTERFACE does not appear to be used in CIME either, but there's not much harm in keeping it.

Imho, if something is unused (and pointless), it should be removed. Pointless code tends to pile up, until we have no idea of what is actually needed/used. I vote for pruning it.

I think you are right.

@rljacob , I looked at Sarich's Moab branch and he's not using this cppdef there either.

components/cmake/modules/FindCsmShare.cmake

jgfouca · 2023-09-28T18:35:20Z

Hmm, it looks like the CI build fails with a link error for the case it tries to build. The same case builds fine on mappy so I may have some digging to do.

bartgol · 2023-09-28T20:04:25Z

components/cmake/modules/FindCsmShare.cmake

@@ -29,7 +29,7 @@ endif()

 # Create the interface library, and set target properties
 add_library (csm_share INTERFACE)
-target_link_libraries (csm_share INTERFACE ${CSM_SHARE_LIB};mct;${PIOLIBS})
+target_link_libraries (csm_share INTERFACE ${CSM_SHARE_LIB};mct;spio)


Should we maybe to the same thing we did for MCT also for spio? That is, adding

find_package(PIO)

?

Yep, did exactly that.

I meant adding it inside the FindCsmShare.cmake module.

Oh, I see. Yes let me do that.

bartgol · 2023-09-28T20:05:35Z

components/cmake/common_setup.cmake

+# Have to do this here for now because the macros must be loaded in order for
+# the netcdf cmake variables to be set. Once scorpio has a good package-config,
+# this won't be necessary
+if (NOT TARGET netcdf)


I don't think we need this if guard. subsequent calls to find_package will do nothing if the package was already found...

I think that depends on the implementation, but I'm not sure. I wanted to keep the findX modules as simple as possible, so they don't set or check XX_FOUND

I think CMake states it clearly

Once one of the calls succeeds the result variable will be set and stored in the cache so that no call will search again.

That could be for find_package when it does things the "normal" way, IE loading the package-config.cmake. When you are rolling your own find module, I don't think this is true. The example FindXXX.cmake guidance in the documentation has all that stuff explicit.

Ok, no big deal. I think the logic for skipping second calls works regardless of search mode (provided that FindXXX.cmake sets <PackageName>_FOUND=TRUE). But I am not entirely sure, so let's not stop progress based on this detail.

components/cmake/modules/FindNETCDF.cmake

components/cmake/modules/FindPIO.cmake

components/cmake/modules/FindNETCDF.cmake

components/cmake/modules/FindPIO.cmake

Fix mpas netcdf dep, should be public

This ensures correct link order

CMake: Improve dependency management for mct and csm_share Neither of these packages are cmake packages, so we have to roll our own Find$package.cmake modules. These can be pretty simple for e3sm since we build them ourselves (no worries about version or not found etc). Also, remove a lot of unused for special handling of clm and stuff for pio building. [BFB]

Second merge to next for this PR. [BFB]

cime_config/machines/scripts/cmake3ify-package

components/eamxx/src/share/io/CMakeLists.txt

Third merge to next for this PR. [BFB]

bartgol · 2023-10-02T23:34:41Z

components/cmake/modules/FindNETCDF.cmake

+
+  # Create the interface library, and set target properties
+  add_library(netcdf INTERFACE)
+  target_link_libraries(netcdf INTERFACE ${pnetcdf_lib};${netcdf_c_lib};${netcdf_f_lib})


Suggested change

target_link_libraries(netcdf INTERFACE ${pnetcdf_lib};${netcdf_c_lib};${netcdf_f_lib})

target_link_libraries(netcdf INTERFACE ${pnetcdf_lib};${netcdf_f_lib};${netcdf_c_lib})

Fourth merge to next for this PR. [BFB]

This will ensure correct link order

Fifth merge to next for this PR. [BFB]

rljacob · 2023-10-05T18:49:55Z

This branch looks like it did a lot of work on the netcdf/pnetcdf dependency but that's not mentioned in the title or description.

jgfouca added 3 commits September 27, 2023 15:40

CMake: improve dep on mct and csm_share

cf3c1a8

Add modules

f2786de

Fixes

fbdfc20

jgfouca added Land BFB PR leaves answers BFB CMake build system labels Sep 27, 2023

jgfouca requested review from bartgol and bishtgautam September 27, 2023 22:35

jgfouca self-assigned this Sep 27, 2023

jgfouca mentioned this pull request Sep 27, 2023

E3SM CMake Build System V2! #5943

Open

49 tasks

bartgol reviewed Sep 27, 2023

View reviewed changes

jgfouca added 2 commits September 28, 2023 10:24

Some minor fixes. Add find MCT to find module for csm_share

ac40a2c

Remove unused MCT_INTERFACE cppdef

360bf8a

bartgol approved these changes Sep 28, 2023

View reviewed changes

Ensure netcdf dep is in the right order

3a78f4d

bartgol reviewed Sep 28, 2023

View reviewed changes

components/cmake/modules/FindPIO.cmake Show resolved Hide resolved

jgfouca added 4 commits September 28, 2023 14:28

Fix for rrtmgp netcdf dep

ab75f35

More netcdf fixes. Mpas was using old approach

89f28a0

Remove pnetcdf cppdef

1de6606

Guard all Find modules from repetition so users don't have to

3fcd94e

Fix mpas netcdf dep, should be public

jgfouca requested a review from bartgol September 28, 2023 22:13

bartgol approved these changes Sep 28, 2023

View reviewed changes

jgfouca added 2 commits September 29, 2023 10:16

Better way of handling the spio->netcdf dependency

293a797

Make mpas depend on spio instead of just netcdf

8ab1f1d

This ensures correct link order

bartgol approved these changes Sep 29, 2023

View reviewed changes

jgfouca added 5 commits October 2, 2023 10:55

Better handling of adios2

7075e9a

Fix adios stuff in FindPIO

a5177a0

Only find adios if ADIOS2_ROOT is set

c92837c

Fix paren

a514f98

Another syntax fix

db34761

jgfouca added a commit that referenced this pull request Oct 2, 2023

Merge branch 'jgfouca/cmake_mct_csm_share' into next (PR #5964)

232be8a

Second merge to next for this PR. [BFB]

bartgol reviewed Oct 2, 2023

View reviewed changes

cime_config/machines/scripts/cmake3ify-package Show resolved Hide resolved

Fix a couple more adios things

34f2eec

bartgol reviewed Oct 2, 2023

View reviewed changes

components/eamxx/src/share/io/CMakeLists.txt Show resolved Hide resolved

Workaround adios2 find cmake problem

fe64ea0

jgfouca added a commit that referenced this pull request Oct 2, 2023

Merge branch 'jgfouca/cmake_mct_csm_share' into next (PR #5964)

945e86c

Third merge to next for this PR. [BFB]

bartgol reviewed Oct 2, 2023

View reviewed changes

jgfouca added 2 commits October 3, 2023 11:59

Attempting to improve netcdf situation

eddad77

Progress on refactored netcdf handling

b7a78cb

jgfouca added a commit that referenced this pull request Oct 3, 2023

Merge branch 'jgfouca/cmake_mct_csm_share' into next (PR #5964)

90e4edb

Fourth merge to next for this PR. [BFB]

jgfouca added 2 commits October 4, 2023 11:38

For now, make rrtmgp depend on spio instead of netcdf

50b3694

This will ensure correct link order

Fixes frontier netcdf handling

653b97b

jgfouca added a commit that referenced this pull request Oct 4, 2023

Merge branch 'jgfouca/cmake_mct_csm_share' into next (PR #5964)

b90b908

Fifth merge to next for this PR. [BFB]

dqwu mentioned this pull request Oct 5, 2023

mpas-framework linking errors with updated Fortran interface of SCORPIO E3SM-Project/scorpio#543

Closed

jgfouca merged commit 2ff3742 into master Oct 5, 2023
2 checks passed

jgfouca deleted the jgfouca/cmake_mct_csm_share branch October 5, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: Improve dependency management for mct and csm_share #5964

CMake: Improve dependency management for mct and csm_share #5964

jgfouca commented Sep 27, 2023

bartgol left a comment

bartgol Sep 27, 2023

rljacob Sep 28, 2023

bartgol Sep 28, 2023

rljacob Sep 28, 2023

jgfouca Sep 28, 2023

bartgol Sep 28, 2023

jgfouca Sep 28, 2023

jgfouca Sep 28, 2023

jgfouca commented Sep 28, 2023

bartgol Sep 28, 2023

jgfouca Sep 28, 2023

bartgol Sep 28, 2023

jgfouca Sep 28, 2023

bartgol Sep 28, 2023

jgfouca Sep 28, 2023

bartgol Sep 28, 2023

jgfouca Sep 28, 2023

bartgol Sep 28, 2023

bartgol Oct 2, 2023

rljacob commented Oct 5, 2023

	target_link_libraries(netcdf INTERFACE ${pnetcdf_lib};${netcdf_c_lib};${netcdf_f_lib})
	target_link_libraries(netcdf INTERFACE ${pnetcdf_lib};${netcdf_f_lib};${netcdf_c_lib})

CMake: Improve dependency management for mct and csm_share #5964

CMake: Improve dependency management for mct and csm_share #5964

Conversation

jgfouca commented Sep 27, 2023

bartgol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgfouca commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rljacob commented Oct 5, 2023