Conversation
|
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 ( |
|
I agree to be a maintainer. |
|
@conda-forge/help-python-c, ready for review! |
|
@conda-forge-admin, please restart ci |
|
Current build error seems to be some issue with staged-recipes on OSX. Error message: Same issue in #23918 |
|
@conda-forge/help-python-c ready for review! |
|
@conda-forge/staged-recipes kindly submitting another request for review. If there's anything I can do to let me know. Cheers, |
recipes/rapidgzip/THIRDPARTY.yml
Outdated
| @@ -0,0 +1,27 @@ | |||
| --- | |||
There was a problem hiding this comment.
rapidgzip --oss-attributions
There was a problem hiding this comment.
We strongly prefer that you dynamically link to these dependenices via the existing conda-forge packages or by adding additional packages to this PR. OR provide a justification of why static linking to these packages is needed.
There was a problem hiding this comment.
- cxxopts: is a header-only library, dynamic linking makes no sense. I don't understand what the conda-forge package exists for. I guess to update the headers and rebuild dependant conda-forge packages?
- ISA-L: uses a custom fork made by me. Upstream cannot be used because of custom additions. It can be disabled with the
WITH_ISALCMake option. Rapidgzip is ~2x faster with ISA-L. (The upstream version would have been available on conda-forge) - rpmalloc: I did not bother dynamically linking against this, because it is not available as a package on Ubuntu. It can be disabled with the
WITH_RPMALLOCCMake option, which seems like the easiest option. Rpmalloc only leads to ~20% speedup for short workloads and possibly for longer workloads on high core counts. It is much less important for performance than ISA-L. I did not find rpmalloc on conda-forge. - zlib: can be dynamically linked with the
USE_SYSTEM_ZLIBCMake option. I don't know how to set this option for the conda build. I guess I'll need to take a look. Unsurprisingly exists on conda-forge.
There was a problem hiding this comment.
If zlib is dynamically linked, should it be removed from THIRDPARTY.yml?
There was a problem hiding this comment.
If this file can be generated, it should be generated during the build and not be submitting as part of the recipe.
There was a problem hiding this comment.
I have finally tried to use cxxopts and zlib via the existing conda packages but I get an import error because of an undefined symbol now:
The following NEW packages will be INSTALLED:
_libgcc_mutex: 0.1-conda_forge conda-forge
_openmp_mutex: 4.5-2_gnu conda-forge
bzip2: 1.0.8-hd590300_5 conda-forge
ca-certificates: 2023.11.17-hbcca054_0 conda-forge
cxxopts: 3.1.0-hf52228f_0 conda-forge
ld_impl_linux-64: 2.40-h41732ed_0 conda-forge
libffi: 3.4.2-h7f98852_5 conda-forge
libgcc-ng: 13.2.0-h807b86a_3 conda-forge
libgomp: 13.2.0-h807b86a_3 conda-forge
libnsl: 2.0.1-hd590300_0 conda-forge
libsqlite: 3.44.2-h2797004_0 conda-forge
libstdcxx-ng: 13.2.0-h7e041cc_3 conda-forge
libuuid: 2.38.1-h0b41bf4_0 conda-forge
libzlib: 1.2.13-hd590300_5 conda-forge
ncurses: 6.4-h59595ed_2 conda-forge
openssl: 3.2.0-hd590300_1 conda-forge
pip: 23.3.1-pyhd8ed1ab_0 conda-forge
python: 3.10.13-hd12c33a_0_cpython conda-forge
python_abi: 3.10-4_cp310 conda-forge
rapidgzip: 0.10.4-py310hd5d28ae_0 local
readline: 8.2-h8228510_1 conda-forge
setuptools: 68.2.2-pyhd8ed1ab_0 conda-forge
tk: 8.6.13-noxft_h4845f30_101 conda-forge
tzdata: 2023c-h71feb2d_0 conda-forge
wheel: 0.42.0-pyhd8ed1ab_0 conda-forge
xz: 5.2.6-h166bdaf_0 conda-forge
zlib: 1.2.13-hd590300_5 conda-forge
Preparing transaction: ...working... done
Verifying transaction: ...working... done
Executing transaction: ...working... done
export PREFIX=/miniconda3/conda-bld/rapidgzip_1701540286573/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place
export SRC_DIR=/miniconda3/conda-bld/rapidgzip_1701540286573/test_tmp
Traceback (most recent call last):
File "/miniconda3/conda-bld/rapidgzip_1701540286573/test_tmp/run_test.py", line 2, in <module>
import: 'rapidgzip'
import rapidgzip
ImportError: /miniconda3/conda-bld/rapidgzip_1701540286573/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/lib/python3.10/site-packages/rapidgzip.cpython-310-x86_64-linux-gnu.so: undefined symbol: inflatePrime
WARNING: Tests failed for rapidgzip-0.10.4-py310hd5d28ae_0.tar.bz2 - moving package to /miniconda3/conda-bld/broken
TESTS FAILED: rapidgzip-0.10.4-py310hd5d28ae_0.tar.bz2
According to the changelog, inflatePrime has been added in 2005 with version 1.2.2.4 and according to the list above, version 1.2.13 is loaded. So, why is inflatePrime still missing? And it seems to be the only missing symbol as many others are used but not shown as missing.
I have attached the patch, which probably should be cherry-picked by @ap-- ontop of this PR if only it would work ...
0001-Use-conda-packaged-cxxopts-and-zlib.txt
Edit: I have opened an issue here
There was a problem hiding this comment.
cxxopts: is a header-only library, dynamic linking makes no sense. I don't understand what the conda-forge package exists for. I guess to update the headers and rebuild dependant conda-forge packages?
Not every library revends its dependencies, so the package is there for convenience. If CXXOPTS is header only, then static linking is the only option, so that's the justification.
ISA-L: uses a custom fork made by me. Upstream cannot be used because of custom additions. It can be disabled with the WITH_ISAL CMake option. Rapidgzip is ~2x faster with ISA-L. (The upstream version would have been available on conda-forge)
If your custom fork contains unique changes that are not compatible with upstream, this is an acceptable justification because your fork is no longer the canonical ISA-L. However, we encourage you to upstream your changes, so others can benefit.
rpmalloc: I did not bother dynamically linking against this, because it is not available as a package on Ubuntu. It can be disabled with the WITH_RPMALLOC CMake option, which seems like the easiest option. Rpmalloc only leads to ~20% speedup for short workloads and possibly for longer workloads on high core counts. It is much less important for performance than ISA-L. I did not find rpmalloc on conda-forge.
Create an additional recipe folder for rpmalloc in this PR, and add rpmalloc as a host dependency for rapidgzip. Our build runner can determine in which order to build multiple recipes or disable this option until rpmalloc is added.
zlib: can be dynamically linked with the USE_SYSTEM_ZLIB CMake option. I don't know how to set this option for the conda build. I guess I'll need to take a look. Unsurprisingly exists on conda-forge.
Add zlib to the host requirements. Set USE_SYSTEM_ZLIB and ensure that your package links to libz.so in the host environment i.e. $PREFIX during the build step. Please push the commits in which you attempt to USE_SYSYEM_ZLIB so we can read the build logs and/or recreate your work.
|
@conda-forge/help-python-c, please review |
carterbox
left a comment
carterbox
left a comment
There was a problem hiding this comment.
It looks like the following tasks need to be completed for this PR:
- dynamically link to zlib instead of revending
- dynamically link to rpmalloc instead of revending
- generate THIRDPARTY.yaml during the build step, so that upstream licenses are updated automatically if changed
recipes/rapidgzip/THIRDPARTY.yml
Outdated
| @@ -0,0 +1,27 @@ | |||
| --- | |||
There was a problem hiding this comment.
cxxopts: is a header-only library, dynamic linking makes no sense. I don't understand what the conda-forge package exists for. I guess to update the headers and rebuild dependant conda-forge packages?
Not every library revends its dependencies, so the package is there for convenience. If CXXOPTS is header only, then static linking is the only option, so that's the justification.
ISA-L: uses a custom fork made by me. Upstream cannot be used because of custom additions. It can be disabled with the WITH_ISAL CMake option. Rapidgzip is ~2x faster with ISA-L. (The upstream version would have been available on conda-forge)
If your custom fork contains unique changes that are not compatible with upstream, this is an acceptable justification because your fork is no longer the canonical ISA-L. However, we encourage you to upstream your changes, so others can benefit.
rpmalloc: I did not bother dynamically linking against this, because it is not available as a package on Ubuntu. It can be disabled with the WITH_RPMALLOC CMake option, which seems like the easiest option. Rpmalloc only leads to ~20% speedup for short workloads and possibly for longer workloads on high core counts. It is much less important for performance than ISA-L. I did not find rpmalloc on conda-forge.
Create an additional recipe folder for rpmalloc in this PR, and add rpmalloc as a host dependency for rapidgzip. Our build runner can determine in which order to build multiple recipes or disable this option until rpmalloc is added.
zlib: can be dynamically linked with the USE_SYSTEM_ZLIB CMake option. I don't know how to set this option for the conda build. I guess I'll need to take a look. Unsurprisingly exists on conda-forge.
Add zlib to the host requirements. Set USE_SYSTEM_ZLIB and ensure that your package links to libz.so in the host environment i.e. $PREFIX during the build step. Please push the commits in which you attempt to USE_SYSYEM_ZLIB so we can read the build logs and/or recreate your work.
To speed up things, as I have no control over this PR, I have tried to reopen my own PR for now: #23903 but failed to do so and therefore created another one: #24835 I'm still not any further to fix this issue and really would like some help. In the meantime, I guess, I'll have to read deeper into conda and try to find out how and which shared libraries it provides to my own package. |
How do I do that? Is there an example I can merge into this PR? |
|
build/script can have multiple lines or you can use a file named build.sh |
|
Hello everyone, Sorry for the delay. It was a busy month and I'm currently on holiday. Happy holidays everyone 🎄 |
Thanks, I have pushed to this PR, which is probably for the best because there already have been reviews here. Unfortunately, I'm not any closer to fixing that zlib problem. |
|
Conda build has a link checker. If you look at the logs, you will notice that the link checker doesn't detect any links from any of the binaries in your package to libz. In fact, it warns you that the dependencies on libzlib are unused. I also don't see any linking args being passed to gcc in the build logs. There are some big L My suggestion is that the python build Extension definition is missing some linking args for each non-header library that is set to tl;dr inflatePrime is undefined probably because you didn't tell the compiler is needs to link to libz. |
|
Just a friendly ping to ask how this is progressing? I love rapidgzip and would love to be have a conda package for it. Though sounds like the zlib linking is being problematic? |
I'm unable to bundle zlib as desired by the conda maintainers and "developing" for conda is cumbersome. Any help would be welcome. |
|
Hi friend! We really, really, really appreciate that you have taken the time to make a PR on In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on Cheers and thank you for contributing to this community effort! |
|
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
mxmlnkn
force-pushed
the
rapidgzip
branch
2 times, most recently
from
36f7a81 to
442b4fa
Compare
|
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/rapidgzip/meta.yaml:
For recipes/rapidgzip/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/17048539144. Examine the logs at this URL for more detail. |
mxmlnkn
force-pushed
the
rapidgzip
branch
2 times, most recently
from
70d6e7c to
8604aa6
Compare
|
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 ( I do have some suggestions for making it better though... For recipes/rapidgzip/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/17048826010. Examine the logs at this URL for more detail. |
|
Everything should be in order now! @carterbox's observations were completely right, I was just too inept to translate them into a fix in setup.py. All dependencies are now built via conda (cxxopts, zlib) or disabled (rpmalloc, ISA-L). This also makes the THIRDPARTY.yml obsolete @conda-forge/staged-recipes ready for review. |
|
To help direct your pull request to the best reviewers, please mention a topic-specifc team if your recipe matches any of the following: conda-forge/help-c-cpp, conda-forge/help-cdts, conda-forge/help-go, conda-forge/help-java, conda-forge/help-julia, conda-forge/help-nodejs, conda-forge/help-perl, conda-forge/help-python, conda-forge/help-python-c, conda-forge/help-r, conda-forge/help-ruby,or conda-forge/help-rust. Thanks! |
github-actions
bot
added
review-requested
staged-recipes
and removed
Awaiting author contribution
labels
|
@conda-forge/help-python-c ready for review |
|
Good job! |
6 participants
Checklist
url) rather than a repo (e.g.git_url) is used in your recipe (see here for more details).