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

Bump netcdf source #114

Merged
merged 4 commits into from
Oct 11, 2023
Merged

Conversation

weiznich
Copy link
Contributor

@weiznich weiznich commented Oct 6, 2023

This pulls in Unidata/netcdf-c#2756 which fixes an issue with building netcdf-src in a directory containing a whitespace somewhere in the path.

@lnicola
Copy link
Member

lnicola commented Oct 6, 2023

Looks like Unidata/netcdf-c#2623 has indeed been merged.

@weiznich
Copy link
Contributor Author

weiznich commented Oct 6, 2023

I'm not sure about the CI errors: Are they likely caused by this change or are they happening from time to time?

@lnicola
Copy link
Member

lnicola commented Oct 6, 2023

No idea, it passed three weeks ago. But hdf5-sys hasn't changed in two years.

  /usr/bin/ld: cannot find -lhdf5_hl_debug: No such file or directory
  /usr/bin/ld: cannot find -lhdf5_debug: No such file or directory

@magnusuMET
Copy link
Member

Unfortunately the fix in 2756 has not landed in an official release, which I would like to see before packaging it.

2623 has been merged and we could bump the source to the offical release (v4.92) instead of a personal branch.

CI is not happy because the hdf5 library was not found properly by cmake. We need to change the build script somewhat to get the names/paths on the expected format.

@magnusuMET
Copy link
Member

Nothing wrong with CI, reran it just now

@magnusuMET
Copy link
Member

Found the issue and made a PR in Unidata/netcdf-c#2761

@weiznich
Copy link
Contributor Author

weiznich commented Oct 6, 2023

Thanks for looking into this 👍

Do you want to keep the PR open till the next netcdf release? Or is it better to close it?

Also: Would you be open to accept a backport of the linked fix to "private" fork that is based on the latest release?

@magnusuMET
Copy link
Member

I would be willing to accept a backport with the fix applied to 4.9.2 since this seems to be an important issue in your case. Do specify a rev in the submodule (which I forgot on my fork...)

@weiznich
Copy link
Contributor Author

I would be willing to accept a backport with the fix applied to 4.9.2 since this seems to be an important issue in your case. Do specify a rev in the submodule (which I forgot on my fork...)

I've updated the PR to reference a version of the netcdf source that is the v4.9.2 tag + the fix cherry-picked on top of it. I'm not sure if you are OK with accepting the forked repo or if you would like to have the ownership of the branch. In the later case you would need to pull in the branch into your fork.

@magnusuMET
Copy link
Member

I am afraid the branch is not merged on top of 4.9.2 from Unidata, but instead from my fork. You need to rebase against rev 9328ba17cb53f13a63707547c94f4715243dafdf from https://github.com/Unidata/netcdf-c

@weiznich
Copy link
Contributor Author

It seems like git submodule does not update the rev in the .gitmodules file automatically, so that what was pushed did not correspond with my local version.
It should be fixed now.

@magnusuMET magnusuMET merged commit aa1f6c8 into georust:master Oct 11, 2023
13 checks passed
@magnusuMET
Copy link
Member

Thanks for the PR, this update should be included in 0.3.2 which was just pushed

@weiznich
Copy link
Contributor Author

Thanks for the fast release ❤️

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.

3 participants