Skip to content

Add CMake build system and Pkgconf pc file#17

Merged
harshula merged 6 commits intomainfrom
development
Jul 23, 2025
Merged

Add CMake build system and Pkgconf pc file#17
harshula merged 6 commits intomainfrom
development

Conversation

@harshula
Copy link
Copy Markdown
Collaborator

@harshula harshula commented Nov 19, 2024

[Update: 22/07/2025]

  • New: option(BUILD_SHARED_LIBS "Build shared/dynamic libraries" OFF)

@harshula harshula self-assigned this Nov 19, 2024
@harshula harshula marked this pull request as draft November 19, 2024 02:46
@harshula harshula changed the title Add CMake build system and Add CMake build system and Pkgconf pc file Nov 19, 2024
Copy link
Copy Markdown
Member

@micaeljtoliveira micaeljtoliveira left a comment

Choose a reason for hiding this comment

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

@harshula Here are some comments plus a couple of questions.

@harshula harshula force-pushed the development branch 2 times, most recently from b2bb8df to ac2765e Compare November 28, 2024 13:19
@harshula harshula force-pushed the development branch 6 times, most recently from 4a79dc7 to 271db1e Compare December 17, 2024 00:19
@harshula
Copy link
Copy Markdown
Collaborator Author

Update

The Spack interaction with upstream netcdf-fortran (autotools), ACCESS-NRI's FMS (CMake) and ACCESS-NRI's GFDL-generic-tracers (CMake) is complicated if we don't use pkgconf and use CMake's native find_package().

Spack's netcdf-fortran SPR builds via autotools. i.e. No CMake exports. Spack's FMS SPR builds via CMake and uses the FindNetCDF.cmake file that resides within FMS. I've modified ACCESS-NRI's FMS to do the same instead of using pkgconf. When I made the changes, that were recommended above, and switched ACCESS-NRI's GFDL-generic-tracers from using pkgconf to CMake's find_package to find FMS, CMake looks for a FindNetCDF.cmake in GFDL-generic-tracers too. Adding a file to accommodate a dependency's dependency is bad practice.

I had a chat with @micaeljtoliveira about this on Friday. He suggested trying to export the FindNetCDF.cmake in FMS. So this PR will be paused till ACCESS-NRI's FMS is updated.

@dougiesquire
Copy link
Copy Markdown
Collaborator

This has been tested fairly extensively in ACCESS models. Results here: ACCESS-NRI/access-spack-packages#238 (comment)

dougiesquire
dougiesquire previously approved these changes May 13, 2025
Copy link
Copy Markdown
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

One change below and then this is good to go from my perspective (noting that I haven't tested the PkgConfig).

@dougiesquire
Copy link
Copy Markdown
Collaborator

@harshula, once we get @JorgeG94's update for the mocsy dependency into this branch are you okay for me to (squash and) merge?

This has now been pretty well tested. If there's additional testing you want to do, could it continue after merging?

@harshula
Copy link
Copy Markdown
Collaborator Author

harshula commented Jul 8, 2025

Hi @dougiesquire , I'll try to finish my testing this week and I'll merge it.

21/07/2025 - Added support to build the library as shared/dynamic.

@harshula harshula force-pushed the development branch 2 times, most recently from 1010640 to 343ac9f Compare July 21, 2025 05:58
@dougiesquire
Copy link
Copy Markdown
Collaborator

@harshula is your plan to "Squash and merge" this?

@harshula
Copy link
Copy Markdown
Collaborator Author

Hi @dougiesquire , No, there'll be a merge commit. These commits will all be independent.

@dougiesquire
Copy link
Copy Markdown
Collaborator

My preference is to "squash and merge" to minimize the number of commits we carry ahead of upstream. Are you okay with that?

@harshula
Copy link
Copy Markdown
Collaborator Author

Hi @dougiesquire , A merge commit is different from a 'rebase and merge', where all the commits appear linearly in the git commit history and hence difficult to isolate to a single PR/merge. The merge commit provides isolation of a series of commits. e.g. git revert of the merge commit reverts the entire series of commits. I'll be doing a merge commit to keep the functional changes in separate commits but the series will isolated and grouped together via the merge commit. i.e. If you view the log via a graphical/tree diff, you'll clearly see this series of commits grouped together.

@dougiesquire
Copy link
Copy Markdown
Collaborator

Yup, I understand that difference thanks. But I said "squash and merge", not "rebase and merge". As I mentioned, we're trying to keep clean and minimize the commits on main that are ahead of upstream. You can include the PR number in the squashed commit message (the Github UI defaults to this) so that the individual commits in this PR can still be easily found.

@harshula
Copy link
Copy Markdown
Collaborator Author

Hi @dougiesquire , Your reasoning sounded like you are confusing a merge commit with 'rebase and merge'. Try using git log --graph. The merge commit achieves the isolation you want and additionally keeps the functional changes in separate commits.
If you want to properly track an upstream repo, try using a strategy like we do for our Spack fork. I'm happy to help with that. I'm afraid the current GFDL-generic-tracers main does not look like it is tracking https://github.com/NOAA-GFDL/ocean_BGC/commits/main/ .

@dougiesquire
Copy link
Copy Markdown
Collaborator

Your reasoning sounded like you are confusing a merge commit with 'rebase and merge'. Try using git log --graph. The merge commit achieves the isolation you want and additionally keeps the functional changes in separate commits.

Nah I'm not confusing them. I'm just trying to keep the commits on main clean/minimal and as linear as possible.

@harshula
Copy link
Copy Markdown
Collaborator Author

Notes
Waiting on the test runs. If they are successful, then I'll rebase all the commits to ensure they all have the same last modified date and then do a merge commit.

@dougiesquire
Copy link
Copy Markdown
Collaborator

Apologies, testing was held up by a dependency bug in model-config-tests and Payu

Copy link
Copy Markdown
Collaborator

@dougiesquire dougiesquire left a comment

Choose a reason for hiding this comment

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

Summary of results of testing with dev-1deg_jra55_ryf+wombatlite config:

  • Adding support for shared fms, mocsy, gtracers libs does not change answers (expected)
  • Building ACCESS-OM2 with shared fms, mocsy, gtracers libs does change answers (by a tiny amount)

Thanks for this long slog @harshula. I'm happy to merge. Don't forget to rebase :)

@harshula harshula merged commit 5ba87f8 into main Jul 23, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Done in ACCESS-ESM1.6 Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project
Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants