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

CMake build no longer uses generator expressions in defines #2056

Merged

Conversation

islas
Copy link
Collaborator

@islas islas commented May 31, 2024

TYPE: bug fix

KEYWORDS: cmake, compilation

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:
The use of generator expressions in the defines compacts the logic neatly but removes the ability to evaluate these conditionals at configuration time. As such, assumptions must either be made or defines wholly dropped when adding configure-time commands like C preprocessing, both of which are wrong.

Solution:
Switch the logic to a more verbose if()-style that guarantees defines that can be known at configure time are resolved.

LIST OF MODIFIED FILES:
M CMakeLists.txt
M cmake/c_preproc.cmake

RELEASE NOTE: CMake build no longer uses generator expressions in defines

@amstokely
Copy link
Collaborator

These changes look good to me. My only suggestion is to use target_compile_definitions where possible and to group related compile definitions into a list before passing them to add_compile_definitions or target_compile_definitions. However, I see that you've already implemented this approach in PR 2053

@islas islas requested a review from amstokely September 13, 2024 20:32
Copy link
Collaborator

@amstokely amstokely left a comment

Choose a reason for hiding this comment

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

All of these changes look good to me.

@mgduda mgduda self-requested a review October 11, 2024 22:55
@islas islas merged commit b1ec964 into wrf-model:release-v4.6.1 Oct 12, 2024
0 of 4 checks passed
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.

3 participants