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

Migrate hdf5 version #36

Merged
merged 3 commits into from
Dec 6, 2021
Merged

Migrate hdf5 version #36

merged 3 commits into from
Dec 6, 2021

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Nov 22, 2021

Blocked on: #35

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@xylar
Copy link
Contributor

xylar commented Nov 30, 2021

A new version will be needed for #34, #35 and then this, and there's no indication of when that might happen:
Unidata/netcdf-cxx4#106

@ocefpaf ocefpaf marked this pull request as draft December 1, 2021 16:46
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 6, 2021

@conda-forge-admin please rerender

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 6, 2021

With enough patching, anything is possible!

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 6, 2021

@xylar for some reason osx find HDF5 but then fails at defining the variables.

Maybe we can help CMake find HDF5 somehow?

@xylar
Copy link
Contributor

xylar commented Dec 6, 2021

@hmaarrfk, sorry, I really don't have much expertise at all in Cmake. I'm not sure I'm going to be much help here. Seems pretty frustrating!

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 6, 2021

The "not enough" argument error is a red herring. The real error is that FindHDF5 does not set the variables correctly

@xylar
Copy link
Contributor

xylar commented Dec 6, 2021

@hmaarrfk, you got it! At least it looks that way!

@hmaarrfk hmaarrfk marked this pull request as ready for review December 6, 2021 16:12
@xylar
Copy link
Contributor

xylar commented Dec 6, 2021

@hmaarrfk, thanks a lot for adding yourself as a maintainer. That's very much appreciated.

@kmuehlbauer
Copy link
Contributor

@hmaarrfk Great sleuthing!

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 6, 2021

Merging this will basically close out the HDF5 migration. It doesn't seem like there is much else that needs to be moved over.

Copy link
Contributor

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@xylar
Copy link
Contributor

xylar commented Dec 6, 2021

@ocefpaf and @kmuehlbauer, let me know if you have any concerns. I'll merge this unless I hear something soon (by tomorrow).

@@ -69,3 +86,4 @@ extra:
- kmuehlbauer
- ocefpaf
- xylar
- hmaarrfk
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ocefpaf
Copy link
Member

ocefpaf commented Dec 6, 2021

@ocefpaf and @kmuehlbauer, let me know if you have any concerns. I'll merge this unless I hear something soon (by tomorrow).

LGTM! Merge away!!

Thanks @hmaarrfk!

@xylar xylar merged commit eddfb12 into conda-forge:master Dec 6, 2021
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.

5 participants