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

Refactor NetCDF Find Module #727

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

program--
Copy link
Contributor

@program-- program-- commented Feb 9, 2024

This PR refactors the NetCDF find module to correctly use components. The current module does not set component found vars.

Now, when configuring NGen, we should correctly see:

...

-- Found NetCDF: /usr/include (found version "4.9.2") found components: CXX

...

Changes

  • Overhaul of cmake/FindNetCDF.cmake.

Todos

  • Add module documentation

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux
  • macOS

@program-- program-- added bug Something isn't working build Issues related to CMake and building ngen labels Feb 9, 2024
@program-- program-- self-assigned this Feb 9, 2024
@program-- program-- marked this pull request as ready for review February 9, 2024 21:47
@PhilMiller
Copy link
Contributor

This may be mooted or simplified by #729 if that's embraced.

Copy link
Contributor

@donaldwj donaldwj left a comment

Choose a reason for hiding this comment

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

The main issue I see is this seems to loose parallel netcdf detection. If possible I would like that capability kept, If not simple I will approve as is.

set(NETCDF_F77 "NO")
set(NETCDF_F90 "NO")
find_package(NetCDF REQUIRED)
find_package(NetCDF REQUIRED COMPONENTS CXX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably require the C and CXX components at a minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The C library isn't a component, it's the base module, so if you call find_package(NetCDF) that only gets the C library.

function(FindNetCDF_get_is_parallel_aware include_dir)
file(STRINGS "${include_dir}/netcdf_meta.h" _netcdf_lines
REGEX "#define[ \t]+NC_HAS_PARALLEL[ \t]")
string(REGEX REPLACE ".*NC_HAS_PARALLEL[ \t]*([0-1]+).*" "\\1" _netcdf_has_parallel "${_netcdf_lines}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we want to loose parallel netcdf detection. I was planning on using parallel output for the netcdf writer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parallel detection is on lines 52-60.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# Check if NetCDF was built with ParallelIO support
file(STRINGS "${NetCDF_INCLUDE_DIR}/netcdf_meta.h" _netcdf_lines
REGEX "#define[ \t]+NC_HAS_PARALLEL[ \t]")
string(REGEX REPLACE ".*NC_HAS_PARALLEL[ \t]*([0-1]+).*" "\\1" _netcdf_has_parallel "${_netcdf_lines}")
if (_netcdf_has_parallel)
set(NetCDF_HAS_PARALLEL TRUE)
else()
set(NetCDF_HAS_PARALLEL FALSE)
endif()
unset(_netcdf_version_lines)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed git was showing your entire file as a change and I did not find the string when I searched.

@donaldwj donaldwj merged commit 7200322 into NOAA-OWP:master Feb 12, 2024
19 checks passed
@program-- program-- deleted the jsm-refactor-netcdf-cmake branch February 12, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build Issues related to CMake and building ngen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants