Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

Fix building with hdf5 and migrate everything #115

Merged
merged 10 commits into from
Mar 29, 2022

Conversation

fulminemizzega
Copy link
Contributor

@fulminemizzega fulminemizzega commented Mar 26, 2022

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.

This fixes (at least on linux) #100, #90, maybe it helps with #114.

See commit messages for more info, with these changes pynio builds fine at least with linux_64_numpy1.19python3.9.____cpython and linux_64_numpy1.21python3.10.____cpython
There are 101 passing tests during the build, with 506 warnings.

PyNIO does not build with the new HDF5 1.12 API, using the
API compatibility macros for version 1.10 fixes it.
GCC 10 changes how to handle tentative definitions, with this
codebase this generates errors like (simplifying a bit):
[... libnio.a(NclNewHDF5.o):(.bss+0x0): multiple definition of `possibleDimNames';
[...] libnio.a(NclHDF5.o):(.bss+0x0): first defined here

A workaround is available to use the old behaviour, fixing this issue,
see also https://gcc.gnu.org/gcc-10/porting_to.html (par. Default to
-fnoc-common). Probably this is not compatible with clang.
MNT: Re-rendered with conda-build 3.21.8, conda-smithy 3.19.0, and conda-forge-pinning 2022.03.25.20.30.42
@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.

@fulminemizzega
Copy link
Contributor Author

Tests are failing because http://test.opendap.org/opendap/data/nc/bears.nc does not work anymore. A few hours ago it was working fine, I hope it's not because of my test runs...

@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.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Mar 27, 2022

Try:

pytest -k "not test_opendap2"

The server used to download a file needed for this test just went offline.
Maybe revist this in the future?
It was added by mistake in commit 45398aa
@hmaarrfk
Copy link
Contributor

@conda-forge-admin please rerender

@fulminemizzega
Copy link
Contributor Author

fulminemizzega commented Mar 27, 2022

I think I do not understand how to do migrations correctly, the bot rerendered with older versions. I have not committed migration files to my fork.

This commmit is related to c59c7c1, where migrations file were not
included and caused the bot to create commit 8184cbe with old package
versions.
@hmaarrfk
Copy link
Contributor

I think it is good to keep the rerender commits completely isolated from your changes. You can then rebase and cleanup your history with more ease

@hmaarrfk
Copy link
Contributor

Are you interested in becoming a maintainer?

@fulminemizzega
Copy link
Contributor Author

fulminemizzega commented Mar 28, 2022

Are you interested in becoming a maintainer?

First, thank you for your proposal. I do not know if it is a good idea, I've just improved a bit what I did back in January and sent a similar PR, I do not use any of this stuff and neither understand what it is used for (well, I understand it reads netCDF files, that's all). Back then I was playing with conda for the first time trying to reproduce an environment with newer packages and the first thing that failed was pynio, requiring an old hdf5 version. By luck I found about the compatibility macro, I tested it and for once instead of throwing everything away and filing an issue I decided to make a PR.
I know my way around a bit with git, python (dabbled with numpy and matplotlib) and C, but I'm not a python developer. I did this just for fun, being a maintainer implies that I have to be able to make time for this.. how strict are the requirements for being a maintainer?

I think it is good to keep the rerender commits completely isolated from your changes. You can then rebase and cleanup your history with more ease

I use git in my personal projects and never had to rebase anything, I can try. I do now know if I understand completely what you said, I've made commits with changes and a re-render like 45398aa and 5488686... do you mean that also these commits should be changed?

@hmaarrfk
Copy link
Contributor

I feel like the requirements are quite loose.

Ultimately, if you need this even a little bit, it is in your interest to keep it alive.

Otherwise, do you use this package at all? I'm somewhat interested in archiving this package since it hasn't been refreshed in a while.

But... Maybe we can merge this and this about this problem later.

@fulminemizzega
Copy link
Contributor Author

Otherwise, do you use this package at all? I'm somewhat interested in archiving this package since it hasn't been refreshed in a while.

A research friend showed me this ugly setup with a hand-made VM with many (many) dependencies, I tried to reproduce it with conda, after trying for 2 days I gave up, but at least I got pynio updated. I think the current solution is to ship back and forth a VM that keeps growing in size (I did not know before how hopeless it was).
So, no, I do not use at all.

But... Maybe we can merge this and this about this problem later.

Now at least it can be refreshed, maybe having found these fixes will make it easier in the future.
Was your suggestion about isolating rerendering from other changes a future advice or is it better if I rebase before merging this PR?

@hmaarrfk
Copy link
Contributor

@conda-forge-admin please rerender

@github-actions
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/pynio-feedstock/actions/runs/2056049142.

@hmaarrfk
Copy link
Contributor

See conda-forge/av-feedstock#53

  • One commit where I touched every line of code.
  • The second commit where the rerender happened.

You can then delete the rerenders when you want to cleanup, or just sqaush them to better understnad what your modifications were

@hmaarrfk hmaarrfk merged commit d0b6b56 into conda-forge:main Mar 29, 2022
@fulminemizzega
Copy link
Contributor Author

Try:

pytest -k "not test_opendap2"

Link for the file used in this test is now up again, I reverted this change. I also added another patch that fixes a subset of the warnings from numpy, does the build number need to be incremented?. Can new commits still be added to this PR?

@hmaarrfk
Copy link
Contributor

Can new commits still be added to this PR?

No. please open a new PR.

At this stage you are doing the maintainers job. so please add yourself as one.

@hmaarrfk
Copy link
Contributor

"a maintainer's job"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants