search

NOAA-EMC / NCEPLIBS-g2

Utilities for coding/decoding GRIB2 messages.
Other
7 stars 14 forks source link

Combining grib_util with this repo #706

Closed AlysonStahl-NOAA closed 1 month ago

AlysonStahl-NOAA commented 1 month ago

Fixes #296 Fixes NOAA-EMC/NCEPLIBS-grib_util#332

edwardhartnett commented 1 month ago

The tests for utils should be in their own directory, test_utils. It should have it's own data subdirectory.

But we don't want duplicate data files. If the utils test needs to get data from tests/data that''s fine too.

AlysonStahl-NOAA commented 1 month ago

@edwardhartnett @AlexanderRichert-NOAA

I've done pretty much all I can to get most of the checks to pass.

The tests for the utilities fail the memory checks in the develop workflow. Is this something we can work around for now and fix down the line or should we make sure the tests pass those checks before merging?

One of the Spack checks keep failing. I did make changes to the python script myself and I'm fairly new to Spack, so someone should probably double check my work there.

I know the grib_util workflow can probably be deleted, but I want to double check before I do that.

Also, I just need to update the docs, but I'm not sure how we want to structure it. Any thoughts?

AlexanderRichert-NOAA commented 1 month ago

The Spack workflow failure is because it's compiling w3emc without some extra functions that are disabled by default. In the grib-util recipe, notice it depends on w3emc +extradeps, so we'll need the same thing here.

edwardhartnett commented 1 month ago

The grib_utils CI run was the one that build grib_utils and ran its tests in order to check g2. Since we will now be building and testing them together, it's no longer needed and I will remove it...

edwardhartnett commented 1 month ago

The grib_utils will fail memory tests. That's a known problem. For the purposes of this PR, just cause the developer build to not build the utilities at all. We will address this with some CI cleanup after @AlexanderRichert-NOAA merges his CI simplifying changes which will make it easier.

I will see if I can do that edit to the developer workflow...

edwardhartnett commented 1 month ago

OK, I turned off utils in the developer memory checking build. Also I removed the linux version expected builds for two versions of w3emc, but why?

@AlexanderRichert-NOAA can you help with the spack stuff? I tried to figure it out but ended up confused...

Once we get the CI problems resolved, I think we can merge...

AlexanderRichert-NOAA commented 1 month ago

Sure. I think I see the issue. I'll see if I can work out a fix and I'll submit it as a PR to @AlysonStahl-NOAA's branch.

AlexanderRichert-NOAA commented 1 month ago

See https://github.com/AlysonStahl-NOAA/NCEPLIBS-g2/pull/1. It might be worth (at some point) adding BUILD_4/BUILD_D to the non-Spack unit tests under Linux_options.

AlysonStahl-NOAA commented 1 month ago

See AlysonStahl-NOAA#1. It might be worth (at some point) adding BUILD_4/BUILD_D to the non-Spack unit tests under Linux_options.

Good catch, thanks for fixing that.

edwardhartnett commented 1 month ago

@AlysonStahl-NOAA do you know what is happening with the failing build in CI?

Did we change the acceptable version of w3emc?

AlysonStahl-NOAA commented 1 month ago

@AlysonStahl-NOAA do you know what is happening with the failing build in CI?

Did we change the acceptable version of w3emc?

Yes I did change that. I changed it back to the previous requirement (2.9.0) if BUILD_UTILS is off. It will require 2.10.0 and higher if it's on.

AlexanderRichert-NOAA commented 1 month ago

FWIW this looks pretty good to me-- the only thing I would add is you'll want to update ./cmake/PackageConfig.cmake.in to reflect dependencies, including adding OpenMP if enabled.

AlysonStahl-NOAA commented 1 month ago

FWIW this looks pretty good to me-- the only thing I would add is you'll want to update ./cmake/PackageConfig.cmake.in to reflect dependencies, including adding OpenMP if enabled.

Which other dependencies should be included? It looks like the only dependency it checks for currently is PNG and bacio was removed recently. It doesn't seem to check for Jasper or w3emc either.

AlexanderRichert-NOAA commented 1 month ago

Are any of the changes as far as added find_package's, OpenMP, etc. related to the library itself, or is that all just for the utilities? If it's just for the utilities then I guess nothing needs to change there.

AlysonStahl-NOAA commented 1 month ago

The find_package changes should be for the utilities only. In fact, I'll go ahead and only look for those packages if the utilities are enabled.

edwardhartnett commented 1 month ago

Is this ready to merge?

AlysonStahl-NOAA commented 1 month ago

Is this ready to merge?

Yep.