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

Update CMakeLists.txt: set minimum cmake version to 3.18 #611

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

climbfuji
Copy link
Collaborator

@climbfuji climbfuji commented Nov 9, 2024

All in the title. Resolves #610

User interface changes?: No

Fixes: #610

Testing: only CI testing needed

Related to NCAR/ccpp-physics#1101

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@mkavulich mkavulich left a comment

Choose a reason for hiding this comment

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

@climbfuji Can you direct this commit to the develop branch instead of the main branch please?

@climbfuji
Copy link
Collaborator Author

@climbfuji Can you direct this commit to the develop branch instead of the main branch please?

May I ask why? This is intended to go in asap instead of waiting for the lengthy process of validating the capgen changes. It's a simple fix needed for UFS (and other models will benefit from it). I can easily cherry-pick the commit and create a parallel PR for develop if you think that is helpful/more in line with the (not-yet-finalized) development workflow.

Copy link
Collaborator

@mkavulich mkavulich left a comment

Choose a reason for hiding this comment

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

@climbfuji Sorry, I had assumed the target of main was unintentional, since this didn't seem to be an urgent bug fix. I'll add an item to today's agenda to discuss PRs directly to main, since we don't currently cover that in our development rules.

@mkavulich
Copy link
Collaborator

@climbfuji at today's meeting we decided to go ahead and merge this PR as-is, but we'll need to have a discussion about the protocol for making commits directly to main at a future meeting.

@mkavulich mkavulich merged commit 9e1c3ab into main Nov 21, 2024
19 checks passed
@mkavulich mkavulich deleted the feature/update_cmake_min_version branch November 21, 2024 20:22
@climbfuji
Copy link
Collaborator Author

@climbfuji at today's meeting we decided to go ahead and merge this PR as-is, but we'll need to have a discussion about the protocol for making commits directly to main at a future meeting.

Technically speaking we should have tested this with the UFS (combine with their next PR), but ok. @DusanJovic-NOAA Can you update the ccpp-framework submodule pointer in your next PR, please? Or in any other PR that involves fv3atm that goes in next. Thanks very much.

Looking forward to the "merge to main" discussion. This is going to happen more often, because develop is de-facto dev-capgen at the moment (and will become dev-ncarsima or something like that in the future). Consolidated updates from NRL's internal develop branch will all come to main directly.

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.

Update cmake version requirement to fix warnings that _ROOT variables are ignored
6 participants