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

1.14.0 and remove static builds #188

Merged
merged 3 commits into from
Feb 5, 2023

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Jan 14, 2023

I really don't think we should be dragging along the static builds. The build process isn't really nice toward building both the static and the dynamic library. I think we are going to be better off without this for 1.14.0

Let me know what you all think

Upstream changes

TODO:

  • Create a branch for 1.12.X maintenance (this will be necessary since HDF5 is typically a hard transition)

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-webservices
Copy link
Contributor

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 Author

The really heavy handed patching is to remove all the statically linked binaries from windows:
image

@hmaarrfk
Copy link
Contributor Author

The executables that link to the shared libraries take 10s of kB, those that statically link take MBs
image

@hmaarrfk hmaarrfk marked this pull request as ready for review January 15, 2023 03:13
@hmaarrfk hmaarrfk changed the title 1.14.0 configure - remove static 1.14.0 and remove static builds Jan 15, 2023
@hmaarrfk
Copy link
Contributor Author

@djhoese i'm not sure if you are still interested in ROS3 functionality on windows.

@djhoese
Copy link
Contributor

djhoese commented Jan 15, 2023

It isn't clear to me what ROS3 has to do with this PR or static builds. ROS3 should be made to work on all platforms if upstream supports it. If the tests fail on Windows due to some CI limitation then OK.

recipe/meta.yaml Outdated Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor Author

@conda-forge-admin please rerender

@github-actions
Copy link
Contributor

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/hdf5-feedstock/actions/runs/3924159046.

@mkitti
Copy link
Contributor

mkitti commented Jan 23, 2023

Do we need to synchronize this with a h5py update?

h5py 3.8.0 was released a few hours ago, including support for HDF5 1.14
https://github.com/h5py/h5py/releases/tag/3.8.0

@mkitti
Copy link
Contributor

mkitti commented Jan 24, 2023

h5py was updated to 3.8.0:
conda-forge/h5py-feedstock#125

This incorporates the patches needed for 1.14 support.

@hmaarrfk
Copy link
Contributor Author

Do we need to synchronize this with a h5py update?

In essence, not really. h5py is pinned to the previous version of HDF5 through the run_export.

So this could be merged, without any fear of breaking existing user's H5PY installation.

Im mostly leaving this here for people to comment on the removal of the static builds. But to me, it seems that they were added as an abundance of caution.

@mkitti
Copy link
Contributor

mkitti commented Jan 24, 2023

It would be nice if the shared versions were aliased to the static versions in this case via a symlink or just copying so that typing h5ls would still work.

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jan 24, 2023

h5ls-dynamic seems to be a creation of the cmake files.

doesn't h5ls still work? The tests say they do.

I aggressively patched windows to make h5ls exist (even when building dynamic only) but to remove h5ls-dynamic. I'm honestly not proud of the aggressive patching. Maybe we should just "rename" the executable instead of patching so much. I'm happy to report this as an issue upstream as well.

Edit: clarification. I am not proud of the aggressive patching.

@mkitti
Copy link
Contributor

mkitti commented Jan 24, 2023

Ah, I see. It might be worth discussing at https://forum.hdfgroup.org/ first?

@mkitti
Copy link
Contributor

mkitti commented Jan 24, 2023

It might make sense to split the tools off to another package. Some Linux distributions do this:
https://packages.debian.org/bullseye/hdf5-tools
https://pkgs.org/search/?q=hdf5-tools

@hmaarrfk
Copy link
Contributor Author

discussing at https://forum.hdfgroup.org/ first?

Presently, I am too tired to articulate to a wider audience, but maybe over the weekend i can formulate my thoughts?

I am tempted to reimplement this as a "file rename" in our build script instead of a huge patch.

@hmaarrfk hmaarrfk marked this pull request as draft January 24, 2023 02:19
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jan 24, 2023

It might make sense to split the tools off to another package

Two aspects:

  1. conda doesn't really make this "easy". For example, with RPMs I can specify a list of files I want in a subpackage, then everything else goes in the main package. This ensures I don't "miss" anything in the subpackage creation. This process in conda (to my knowledgE) is a little manual, and error prone.
  2. conda has historically been more "batteries included" (including more options than "the bare minimum") than other distributions. The binaries executables on linux only take up 2MB / 11.1MB as reported by ncdu. Unless somebody wants to step up and "help maintain", I'm not so inclined to split the package given 1.. As a note, uncompressed, the headers make up 2.8/11.1 MB.

@hmaarrfk
Copy link
Contributor Author

@conda-forge-admin please restart cis

@hmaarrfk hmaarrfk marked this pull request as ready for review January 28, 2023 05:00
@hmaarrfk
Copy link
Contributor Author

@conda-forge/hdf5 this is ready for review.

I'm hoping to merge Feb 5.

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Feb 5, 2023

Thank you all for your reviews.

@hmaarrfk hmaarrfk added the automerge Merge the PR when CI passes label Feb 5, 2023
@github-actions github-actions bot merged commit 160a6ed into conda-forge:main Feb 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

@hmaarrfk hmaarrfk mentioned this pull request Feb 5, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants