Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move icepack to a submodule, and enable icepack in CI #1116

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

travissluka
Copy link
Collaborator

@travissluka travissluka commented Jan 8, 2025

Description

Icepack is now a submodule of soca, instead of using a custom fork of the repo in the bundle, the same as what was done for mom6. I used version 1.2.5 of icepack, which let me compile without changing the soca code. Updating to a newer version (1.5.0 is the latest) is outside the scope of this PR

Using git submodules for this is ok assuming the following conditions remain true:

  • SOCA is the only repo in JEDI using icepack. This will probably remain true, unless we want to move some functionality into vader.
  • no changes are needed to icepack other than cmake wrappers (now true since we are not using private interfaces anymore)
  • we don't intend to update icepack often (every now and then when there is a new release, but we can't automatically keep up with develop)

If any of those are ever false, we should revert back to a fork of the repo.


Other things that were changed:

  • tests using icepack are now enabled in CI!
  • mom6 and icepack were both moved to the external/ directory, to tidy things up
  • all of the if(icepack_FOUND) cmake statements were removed. ice is important, so we now assume that icepack is always available! 🧊 🌊 🐧 ❄️
  • the icepack library built here is named icepack_limited because only the files needed by soca are compiled. Keep this in mind if you ever need to use other icepack routines, you might have to enable other files in icepack_files.cmake

After this PR is merged, separate PRs will be issued to remove icepack from the CI and the jedi-bundle, and our icepack fork can be archived. <- nevermind, I forgot icepack was not being used by the CI until now!

@travissluka travissluka changed the title Feature/icepack submodule move icepack to a submodule Jan 8, 2025
@travissluka travissluka self-assigned this Jan 8, 2025
@travissluka
Copy link
Collaborator Author

@eap there is something weird going on with the CI here. There are 4 instances now of the unit test "running", but the integration test is skipped because "prior step failed". The first CI unit test says it has been "running" for 17.5 hours now.

I'm not sure what's going on since I cant look at any of the build output

@travissluka travissluka changed the title move icepack to a submodule move icepack to a submodule, and enable icepack in CI Jan 9, 2025
Copy link
Contributor

@shlyaeva shlyaeva left a comment

Choose a reason for hiding this comment

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

@travissluka 🎉 you rock! looks good to my non-expert eye. I'm going to build and also check what needs to be done to adapt in gdasapp, I'll be back to approve once ready.

Copy link
Contributor

@shlyaeva shlyaeva left a comment

Choose a reason for hiding this comment

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

Looks good to me, and I've prepared our workflow for this change. Thank you @travissluka 🎉

@travissluka travissluka requested a review from Dooruk January 14, 2025 22:21
@travissluka
Copy link
Collaborator Author

pinging @Dooruk in case anything needs to be changed in your build scripts

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.

CI is not using icepack
2 participants