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

Update unifrac-binaries recipe, remove hdf5 dependency #38396

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

sfiligoi
Copy link
Contributor

HDF5 is fully statically linked in unifrac-binaries 1.2.
Remove runtime dependencies from the recipe.


Please read the guidelines for Bioconda recipes before opening a pull request (PR).

  • If this PR adds or updates a recipe, use "Add" or "Update" appropriately as the first word in its title.
  • New recipes not directly relevant to the biological sciences need to be submitted to the conda-forge channel instead of Bioconda.
  • PRs require reviews prior to being merged. Once your PR is passing tests and ready to be merged, please issue the @BiocondaBot please add label command.
  • Please post questions on Gitter or ping @bioconda/core in a comment.
Please use the following BiocondaBot commands:

Everyone has access to the following BiocondaBot commands, which can be given in a comment:

@BiocondaBot please update Merge the master branch into a PR.
@BiocondaBot please add label Add the please review & merge label.
@BiocondaBot please fetch artifacts Post links to CI-built packages/containers.
You can use this to test packages locally.

For members of the Bioconda project, the following command is also available:

@BiocondaBot please merge Upload built packages/containers and merge a PR.
Someone must approve a PR first!
This reduces CI build time by reusing built artifacts.

Also, the bot watches for comments from non-members that include @bioconda/<team> and will automatically re-post them to notify the addressed <team>.

@sfiligoi
Copy link
Contributor Author

@BiocondaBot please fetch artifacts

@BiocondaBot
Copy link
Collaborator

Package(s) built on Azure are ready for inspection:

Arch Package Zip File
linux-64 unifrac-binaries-1.2-h73f19ed_1.tar.bz2 LinuxArtifacts
osx-64 unifrac-binaries-1.2-h6f31009_1.tar.bz2 OSXArtifacts

You may also use conda to install these after downloading and extracting the appropriate zip file. From the LinuxArtifacts or OSXArtifacts directories:

conda install -c ./packages <package name>

Docker image(s) built (images are in the LinuxArtifacts zip file above):

Package Tag Install with docker
unifrac-binaries 1.2--h73f19ed_1
showgzip -dc LinuxArtifacts/images/unifrac-binaries:1.2--h73f19ed_1.tar.gz | docker load

@sfiligoi
Copy link
Contributor Author

@BiocondaBot please add label

@BiocondaBot BiocondaBot added the please review & merge set to ask for merge label Dec 14, 2022
@sfiligoi
Copy link
Contributor Author

Tested the artifacts locally, and they look good

@bgruening
Copy link
Member

The readme has mentioned HDF5 everywhere: https://github.com/biocore/unifrac-binaries#install-native

Is this really how it should be done?

@sfiligoi
Copy link
Contributor Author

sfiligoi commented Dec 14, 2022

The readme has mentioned HDF5 everywhere: https://github.com/biocore/unifrac-binaries#install-native

Is this really how it should be done?

hdf5 is needed during compile, but not at runtime.
We link against the static hdf5 libraries, so we can co-exist with other HDF5 library versions (if any) without any problems.

PS: We could possibly improve the README, yes.

@ebolyen
Copy link
Contributor

ebolyen commented Dec 14, 2022

@sfiligoi, but that wasn't previously the case correct? It looks like that was added/fixed in biocore/unifrac-binaries#15, which is included in this latest release.

(Thanks for updating unifrac by the way! We had just tracked down some incompatibilities between h5py/scikit-bio and unifrac-binaries in our latest pre-release envs, and these changes should fix that)

@sfiligoi
Copy link
Contributor Author

@ebolyen Yes, unifrac-binaries 1.1.x needed hdf5 at runtime.

Note: The hdf5 dependency in 1.1.x was actually a Makefile bug; the dependency was never supposed to be there.
But that was fixed in biocore/unifrac-binaries#15

@ebolyen
Copy link
Contributor

ebolyen commented Dec 15, 2022

Anything we can do to help get this merged?

(I realize this is isn't ideal, but we're waiting on these changes so that we can get the latest scikit-bio in the next QIIME 2 release.)

@ebolyen
Copy link
Contributor

ebolyen commented Dec 15, 2022

We've tested this locally as well now and can confirm that the static linking appears to be working.

We succesfully installed these built packages alongside a source install of unifrac with the latest version of hdf5 (1.12.2) which differs from what unifrac-binaries was statically-linked against (1.12.1).

The tests from our downstream packages (q2-diversity-lib) also succeed with these new versions.

@ebolyen
Copy link
Contributor

ebolyen commented Dec 15, 2022

(I'm not really sure why bioconda built with 1.12.1 and not 1.12.2, but it's what appears in the rendered recipe inside the package.)

@sfiligoi
Copy link
Contributor Author

@bgruening Anything blocking this revision?

@ebolyen ebolyen mentioned this pull request Dec 16, 2022
@ebolyen
Copy link
Contributor

ebolyen commented Dec 19, 2022

Is anyone on the Bioconda team available to review this? Thanks!

@sfiligoi
Copy link
Contributor Author

@rpetit3 Any chance you can look at this?

@sfiligoi
Copy link
Contributor Author

@matthdsm Any chance you can review this?

@matthdsm
Copy link
Contributor

@BiocondaBot please merge

@BiocondaBot
Copy link
Collaborator

I will attempt to upload artifacts and merge this PR. This may take some time, please have patience.

@BiocondaBot BiocondaBot merged commit 33e3348 into bioconda:master Dec 22, 2022
cokelaer pushed a commit to cokelaer/bioconda-recipes that referenced this pull request Apr 28, 2023
Merge PR bioconda#38396, commits were: 
 * Remove hdf5 dependency at runtime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please review & merge set to ask for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants