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 sanitize leading define flag #2130

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

islas
Copy link
Collaborator

@islas islas commented Nov 13, 2024

TYPE: bug fix

KEYWORDS: compilation, cmake

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:
With the addition of #2088, ARCH_LOCAL from the stanza is now being fed into compilation with potential -D defines. On versions of CMake <3.26 leading -D on defines is not removed from certain function calls like add_compile_definitions(). This will pass the configuration stage but will fail to compile when using the defined minimum CMake version of the project.

Solution:
To simplify the logic, all defines fed into the wrf_config.cmake file for the configuration step will be sanitized of leading -D. This follows the original design intent where stanza sanitization happens before being fed into CMake, thus allowing the CMake code to focus on configuration of options rather than translation of stanza into usable values.

TESTS CONDUCTED:

  1. Tested configuration and compilation on CMake version v3.20.6

RELEASE NOTE:
Remove leading -D on defines during stanza reading to allow older versions of CMake to configure properly.

@islas islas requested a review from a team as a code owner November 13, 2024 00:43
@weiwangncar
Copy link
Collaborator

The regression test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

@islas islas merged commit c2e121f into wrf-model:develop Dec 20, 2024
6 of 9 checks passed
islas added a commit that referenced this pull request Dec 20, 2024
TYPE: bug fix

KEYWORDS: autogeneration, compilation, cmake

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:
Currently the CMake build system minimum version is limited by the
automated scoping of generated property on certain source files. This in
combination of `CMP0118` requires usage of CMake >3.20

Solution:
Set the generated property at the correct scope for source file usage.
Lower the minimum required version to reflect this change. Note that
CMake 3.18.6 is not used as there are features from 3.19 in use that,
while could be implemented by hand, would take more effort to remove
than the gained benefit.

TESTS CONDUCTED: 
1. Combined with #2125, #2130, and #2131 this was tested on the latest
patch of each unique minor release of CMake since v3.19.8. The full list
of tested versions is as follows:
v3.19.8
v3.20.6
v3.21.7
v3.22.6
v3.23.5
v3.24.4
v3.25.3
v3.26.6
v3.27.9
v3.28.6
v3.29.8
v3.30.5
v3.31.0

RELEASE NOTE: 
Set the generated property on autogen source code explicitly in CMake
and allow for lowered minimum CMake version of v3.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants