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

Fix: CMAKE_MODULE_PATH contents is being overriden with -D contents, not merged with #2946

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Fix: CMAKE_MODULE_PATH contents is being overriden with -D contents, not merged with #2946

merged 1 commit into from
Jul 10, 2024

Conversation

gorloffslava
Copy link
Contributor

Current code defines the CMAKE_MODULE_PATH as a cache variable[1]. As a result, if one passes the -DCMAKE_MODULE_PATH=<something> to the cmake invocation, its contents override the paths to netcdf-bundled cmake modules. Based on code and attempt to use Shell-PATH-like expansion, it looks as unintended behavior. Moreover, it's quite common in CI/CD systems to pass extra cmake module paths (e.g., we have to do so for AWS SDKs, on which netcdf depends optionally). This tiny fix replaces that with list(APPEND) which adds paths to netcdf-provided modules to the user-passed -DCMAKE_MODULE_PATH. At the same time, if user doesn't passes this variable - the previous behavior is preserved, as netcdf paths will be added to that CMAKE_MODULE_PATH currently holds (if netcdf is built as standalone project and not as a subproject of our project - it's an empty list).

References:
[1]

set(CMAKE_MODULE_PATH "${CMAKE_MODULE_PATH};${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules/;${PROJECT_SOURCE_DIR}/cmake"

@CLAassistant
Copy link

CLAassistant commented Jul 2, 2024

CLA assistant check
All committers have signed the CLA.

@WardF
Copy link
Member

WardF commented Jul 9, 2024

The failures are unrelated to the PR, and are part of an issue we're fixing on the Unidata side. This looks good and will be merged shortly.

@BwL1289
Copy link

BwL1289 commented Jul 9, 2024

@WardF thank you!

1 similar comment
@gorloffslava
Copy link
Contributor Author

@WardF thank you!

@WardF WardF merged commit c0c2910 into Unidata:main Jul 10, 2024
109 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.

4 participants