Fix PackageConfig.cmake.in to include mgbf #797

AlexanderRichert-NOAA · 2024-10-15T23:26:08Z

This PR adds a (proposed) solution for #796. This block should be encapsulated with if(@BUILD_MGBF@) or something to that effect as part of resolving #765.

Description

Including the mgbf config/target files in gsi's will allow users to call find_package(GSI) from their own CMake-based packages without the build failing due to the mbgf::mbgf target being undefined/not found.

Fixes #796

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

In GSI repo (head of develop):

mkdir build && cd build
cmake .. -DCMAKE_INSTALL_DIR=$PWD/install -DCMAKE_C_COMPILER=mpicc -DCMAKE_Fortran_COMPILER=mpif90
make -j6
make install

Then download https://github.com/NOAA-GSL/rrfs_utl/ (or just create a dummy CMakeLists.txt and call find_package(GSI REQUIRED):

mkdir build && cd build
cmake .. -DCMAKE_PREFIX_PATH=/path/to/gsi/build/install

The cmake invocation will fail due to missing mgbf::mgbf target.

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
New and existing tests pass with my changes
Any dependent changes have been merged and published

RussTreadon-NOAA · 2024-10-18T12:44:13Z

@TingLei-NOAA , would you please review this PR since it involves MGBF

@ShunLiu-NOAA and @TingLei-NOAA : who else do you recommend as the second peer reviewer?

TingLei-NOAA · 2024-10-18T13:23:33Z

Russ Sure I will do the reviewing as soon as possible. I would recommend Gang to the reviewer too. Ting

…

______________________________ Ting Lei Physical Scientist, Contractor with Lynker in support of EMC/NCEP/NWS/NOAA 5830 University Research Ct., Cubicle 2765 College Park, MD 20740 ***@***.*** 301-683-3624

On Fri, Oct 18, 2024 at 8:44 AM RussTreadon-NOAA ***@***.***> wrote: @TingLei-NOAA <https://github.com/TingLei-NOAA> , would you please review this PR since it involves MGBF @ShunLiu-NOAA <https://github.com/ShunLiu-NOAA> and @TingLei-NOAA <https://github.com/TingLei-NOAA> : who else do you recommend as the second peer reviewer? — Reply to this email directly, view it on GitHub <#797 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APEFS7FDBTSCRNB54SMXIVLZ4D7DJAVCNFSM6AAAAABQAGRUUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRSGM4TGMZRGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

RussTreadon-NOAA · 2024-10-18T13:28:38Z

@GangZhao-NOAA , do you have time to review this PR?

GangZhao-NOAA · 2024-10-18T13:43:00Z

Hi Russ, Sure. I can review it. Thank you! -Gang

…

On Fri, Oct 18, 2024 at 9:29 AM RussTreadon-NOAA ***@***.***> wrote: @GangZhao-NOAA <https://github.com/GangZhao-NOAA> , do you have time to review this PR? — Reply to this email directly, view it on GitHub <#797 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMWMXU2RIDYYMYJGG7DP3LTZ4EEJ3AVCNFSM6AAAAABQAGRUUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRSGQ4DANJRHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

GangZhao-NOAA · 2024-10-18T13:43:21Z

@RussTreadon-NOAA
Sure. I can review this PR.
Thank you!
-Gang

RussTreadon-NOAA · 2024-10-18T13:44:17Z

Thank you @TingLei-NOAA and @GangZhao-NOAA for reviewing @AlexanderRichert-NOAA ' s PR

GangZhao-NOAA

@RussTreadon-NOAA @AlexanderRichert-NOAA
Hi Russ and Alexander,

The added two lines in src/gsi/cmake/PacakgeConfig.cmake.in seem good to me. I approved this PR.

By the way, I noticed that in the 4 automatic checks by github, 2 checks (Intel Linux Build/setup and GCC Linux Build/gsi) failed. These checks are trivial, so these failures are neglectable, right?

Thanks,
-Gang

I just noticed that

GangZhao-NOAA · 2024-10-18T14:52:33Z

@AlexanderRichert-NOAA @RussTreadon-NOAA
Hi Alexander and Russ,

I am not familiar with cmake. So I do have a question for you about how cmake generates the targets.cmake and config-version.cmake.

Usually in PackageConfig.cmake.in, the two files -- "@PROJECT_NAME@-targets.cmake" and "@PROJECT_NAME@-config-version.cmake" are assumed to be created under and then imported from the same directory. But sometimes I noticed that these two files are not stored under the same directory, for example in GSI,
the "mgbf-config-version.cmake" is saved under the top building directory -- build/mgbf-config-version.cmake,
the "mgbf-targets.cmake" is saved under sub-directory of top building directory -- build/src/mgbf/mgbf-targets.cmake.

Anyway, this inconsistence seems to have no influence on the building process, the GSI package still can be built successfully. So I guess it is not a problem. I just feel curious about it. Do you have any idea or explanation about it?

Thank you!

Best regards
-Gang

TingLei-NOAA · 2024-10-18T16:10:22Z

@AlexanderRichert-NOAA I had approved this PR and thanks for sorting out this issue.
Just a question, are there other ways following the current style of GSI cmake to fix this issue?
I have limited expertise on cmake and wonder if, in src/gsi/CMakelists.txt , changing

if(USE_MGBF)
  if(NOT TARGET mgbf)
    find_package(mgbf REQUIRED)
  endif()
endif()

to

if (USE_MGBF) 
find_package(mgbf REQUIRED)
endif()

would make the export files to be automatically included in the @PROJECT_NAME@-targets.cmake and @PROJECT_NAME@-config-version.cmake.

AlexanderRichert-NOAA · 2024-10-18T19:59:43Z

I'm not sure if it would, but I think at most that would only work is mgbf was already compiled and could be found in CMAKE_PREFIX_PATH, so it wouldn't work for building gsi & mgbf simultaneously because there's no mgbf package to be found (there's no mgbf-config.cmake before we've run make install). I agree it seems like there must be a cleaner way to include it automatically, I'm just not sure what it is.

AlexanderRichert-NOAA · 2024-10-18T20:19:13Z

The only other pitfall I can think of here would be if mgbf was built separately and used as an external package-- Is that ever the case? If so, this could be wrapped with something to the effect of if(@BUILD_MGBF@), or if(@BUILD_MGBF@ AND @USE_MGBF@) in the future case where gsi doesn't always depend on mgbf. Then, theoretically, an external/separate mgbf installation should work fine as long as it's in CMAKE_PREFIX_PATH (though I have not tested this).

RussTreadon-NOAA · 2024-10-22T10:25:30Z

@TingLei-NOAA , above you say

@AlexanderRichert-NOAA I had approved this PR and thanks for sorting out this issue.

but I see no approval from you in this PR. At present only @GangZhao-NOAA has approved this PR.

If you are ready to approve this PR, please do so. If not, what else would you like @AlexanderRichert-NOAA to do?

TingLei-NOAA · 2024-10-22T13:23:23Z

@RussTreadon-NOAA Thanks for pointing out my oversight. I thought I had already clicked the approval button.
Then, I will take this chance to test whether my suggested change behave as I expected before, maybe, some further discussions .

RussTreadon-NOAA · 2024-10-23T16:16:10Z

Cactus and Hera ctests

Install AlexanderRichert-NOAA:patch-1 at 4936050 on Cactus and Hera. Run ctests with the following results

Cactus

Test project /lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/pr797/build
    Start 1: global_4denvar
    Start 2: rtma
    Start 3: rrfs_3denvar_rdasens
    Start 4: hafs_4denvar_glbens
    Start 5: hafs_3denvar_hybens
    Start 6: global_enkf
1/6 Test #3: rrfs_3denvar_rdasens .............   Passed  979.10 sec
2/6 Test #5: hafs_3denvar_hybens ..............   Passed  1398.01 sec
3/6 Test #6: global_enkf ......................   Passed  1460.37 sec
4/6 Test #4: hafs_4denvar_glbens ..............   Passed  1698.58 sec
5/6 Test #2: rtma .............................   Passed  2713.01 sec
6/6 Test #1: global_4denvar ...................   Passed  2948.19 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) = 2948.20 sec

Hera

Test project /scratch1/NCEPDEV/da/Russ.Treadon/git/gsi/pr797/build
    Start 1: global_4denvar
    Start 2: rtma
    Start 3: rrfs_3denvar_rdasens
    Start 4: hafs_4denvar_glbens
    Start 5: hafs_3denvar_hybens
    Start 6: global_enkf
1/6 Test #3: rrfs_3denvar_rdasens .............   Passed  503.31 sec
2/6 Test #6: global_enkf ......................   Passed  1489.26 sec
3/6 Test #5: hafs_3denvar_hybens ..............   Passed  1897.30 sec
4/6 Test #4: hafs_4denvar_glbens ..............   Passed  2018.07 sec
5/6 Test #2: rtma .............................   Passed  2295.33 sec
6/6 Test #1: global_4denvar ...................   Passed  3069.08 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) = 3069.29 sec

The Passed results are expected. This PR does not change source code or fix files.

Hercules and Orion are down today, 10/23/2024, for monthly PM.

RussTreadon-NOAA · 2024-10-23T16:17:57Z

@TingLei-NOAA , we would like to merge this PR no later than next Wednesday, 10/30/2024.

RussTreadon-NOAA

ctests pass on Cactus and Hera.

Approve.

TingLei-NOAA

I haven't successfully created a dummy CMakeList.txt to reproduce the issue and verify if a different change I referred to in previous discussion would actually work. Since this is a question about the style and could be further improved in future if needed, I would approve this PR.

Fix PackageConfig.cmake.in to include mgbf

4936050

RussTreadon-NOAA assigned AlexanderRichert-NOAA Oct 18, 2024

RussTreadon-NOAA requested a review from TingLei-NOAA October 18, 2024 13:28

RussTreadon-NOAA requested a review from GangZhao-NOAA October 18, 2024 13:43

GangZhao-NOAA approved these changes Oct 18, 2024

View reviewed changes

RussTreadon-NOAA self-requested a review October 23, 2024 16:18

RussTreadon-NOAA self-assigned this Oct 23, 2024

RussTreadon-NOAA approved these changes Oct 23, 2024

View reviewed changes

TingLei-NOAA approved these changes Oct 23, 2024

View reviewed changes

RussTreadon-NOAA merged commit 9c47f2a into NOAA-EMC:develop Oct 23, 2024
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PackageConfig.cmake.in to include mgbf #797

Fix PackageConfig.cmake.in to include mgbf #797

AlexanderRichert-NOAA commented Oct 15, 2024 •

edited by RussTreadon-NOAA

Loading

RussTreadon-NOAA commented Oct 18, 2024

TingLei-NOAA commented Oct 18, 2024 via email

RussTreadon-NOAA commented Oct 18, 2024

GangZhao-NOAA commented Oct 18, 2024 via email

GangZhao-NOAA commented Oct 18, 2024

RussTreadon-NOAA commented Oct 18, 2024

GangZhao-NOAA left a comment

GangZhao-NOAA commented Oct 18, 2024

TingLei-NOAA commented Oct 18, 2024

AlexanderRichert-NOAA commented Oct 18, 2024

AlexanderRichert-NOAA commented Oct 18, 2024

RussTreadon-NOAA commented Oct 22, 2024

TingLei-NOAA commented Oct 22, 2024

RussTreadon-NOAA commented Oct 23, 2024

RussTreadon-NOAA commented Oct 23, 2024

RussTreadon-NOAA left a comment

TingLei-NOAA left a comment

Fix PackageConfig.cmake.in to include mgbf #797

Fix PackageConfig.cmake.in to include mgbf #797

Conversation

AlexanderRichert-NOAA commented Oct 15, 2024 • edited by RussTreadon-NOAA Loading

RussTreadon-NOAA commented Oct 18, 2024

TingLei-NOAA commented Oct 18, 2024 via email

RussTreadon-NOAA commented Oct 18, 2024

GangZhao-NOAA commented Oct 18, 2024 via email

GangZhao-NOAA commented Oct 18, 2024

RussTreadon-NOAA commented Oct 18, 2024

GangZhao-NOAA left a comment

Choose a reason for hiding this comment

GangZhao-NOAA commented Oct 18, 2024

TingLei-NOAA commented Oct 18, 2024

AlexanderRichert-NOAA commented Oct 18, 2024

AlexanderRichert-NOAA commented Oct 18, 2024

RussTreadon-NOAA commented Oct 22, 2024

TingLei-NOAA commented Oct 22, 2024

RussTreadon-NOAA commented Oct 23, 2024

RussTreadon-NOAA commented Oct 23, 2024

RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

TingLei-NOAA left a comment

Choose a reason for hiding this comment

AlexanderRichert-NOAA commented Oct 15, 2024 •

edited by RussTreadon-NOAA

Loading