Skip to content

Commit

Permalink
Initialize variable so as not to keep appending. (#66)
Browse files Browse the repository at this point in the history
  • Loading branch information
@DJDavies2
DJDavies2 authored Jun 30, 2023
1 parent cabd775 commit 3bd7769
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion Modules/FindNetCDF.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ if(NetCDF_PARALLEL)
endif()

## Find libraries for each component
set( NetCDF_LIBRARIES )
set( NetCDF_LIBRARIES "" )
foreach( _comp IN LISTS _search_components )
string( TOUPPER "${_comp}" _COMP )

Expand Down

12 comments on commit 3bd7769

@drnimbusrain
Copy link

Choose a reason for hiding this comment

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

@DJDavies2

I have a question on this. I have updated to use this change in our fork of the UWM submodule CMakeModules, and it breaks the build with the following error:

-- Setting build type to 'Release'.
CMake Error at /apps/cmake-3.22.1/share/cmake-3.22/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find NetCDF (missing: NetCDF_LIBRARIES) (found version "4.7.4")
Call Stack (most recent call first):
  /apps/cmake-3.22.1/share/cmake-3.22/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
  CMakeModules/Modules/FindNetCDF.cmake:312 (find_package_handle_standard_args)
  FV3/atmos_cubed_sphere/CMakeLists.txt:42 (find_package)

When I revert the change here, it builds successfully. Thank you!

@climbfuji
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are you trying to find netcdf in cmake? Where is netcdf coming from?

@drnimbusrain
Copy link

@drnimbusrain drnimbusrain commented on 3bd7769 Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Assuming this question is directed to me. Across numerous different NOAA systems using the UFS, I understand it finds NetCDF using the system-installed NetCDF modules (using hpc-stack). Example of successful find on Hera or Orion, when this change is removed:

-- Setting build type to 'Release'.
-- Found NetCDF: /work/noaa/epic-ps/role-epic-ps/hpc-stack/libs/intel-2022.1.2/intel-2022.1.2/impi-2022.1.2/netcdf/4.7.4/include (found version "4.7.4") found components: C Fortran

Note that this change makes the build fail across all NOAA systems tested on, WCOSS2, Hera, Orion (with included system hpc-stack builds/modules). Once I remove the change (delete the "" on line 203), the system builds OK across NOAA systems. I notice this change has yet to be included in the [develop] branch of the ufs-community weather model, as its current hash of the EMC CMakeModules sub-module codes, points to an older version, without this change:

CMakeModules/Modules/FindNetCDF.cmake

Line 203 in cabd775

set( NetCDF_LIBRARIES )

@climbfuji
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DJDavies2 may be on leave at the moment. Adding @srherbener here for his awareness. I don't understand how initializing something to empty can cause a problem, but I am heading out for vacation as well. If there is no good fix for it, I suggest to revert. Duplicates are better than errors.

@DJDavies2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am back now. We use a self-built netcdf stack including C, C++ and Fortran bits (4.9.2, 4.3.1, 4.6.1).

If it can't be fixed then I guess it should be reverted because I don't have access to your system so I can't debug it.

@drnimbusrain
Copy link

@drnimbusrain drnimbusrain commented on 3bd7769 Aug 2, 2023 via email

Choose a reason for hiding this comment

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

@DJDavies2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Who is responsible for testing those systems? I don't have access.

@drnimbusrain
Copy link

@drnimbusrain drnimbusrain commented on 3bd7769 Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good question David. I don't know at EMC who is responsible for overall testing of the UFS-Community's UWM on NOAA systems, but for the air quality modeling (AQM) component aspect, it should be @chan-hoo . This is what I was testing on NOAA systems when I encountered the error.

@climbfuji
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was on leave for the last three weeks. Let's make sure this problem gets fixed in the next weeks

@climbfuji
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we revert the change, because duplicates being appended can be fixed in other ways, and even if they don't get fixed they don't cause any harm. @drnimbusrain @DJDavies2 @aerorahul ?

@DJDavies2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to revert for the time being.

@climbfuji
Copy link
Collaborator

Choose a reason for hiding this comment

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

I created the PR to revert this, waiting for reviews.

Please sign in to comment.