-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
NEW: Add libnvjpeg2k and libnvtiff #28142
base: main
Are you sure you want to change the base?
Conversation
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
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 ( |
recipes/nvjpeg2k/build.sh
Outdated
ln -s ${PREFIX}/${targetsDir}/$j ${PREFIX}/$j | ||
|
||
if [[ $j =~ \.so\. ]]; then | ||
patchelf --set-rpath '$ORIGIN' --force-rpath ${PREFIX}/${targetsDir}/$j |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thanks Daniel! 🙏 Likely we need the same RPATH fixes that we have added for the CTK: conda-forge/cuda-feedstock#10 Perhaps the changes from this PR ( conda-forge/libnvjpeg-feedstock#8 ) would be a good starting point for fixing the issues in libraries If we have executables that need RPATH fixes, we can discuss those as well. Sometimes these differ depending on the layout |
I used the nvjpeg-feedstock as a template to create this recipe. Those fixes had already been merged before I started this recipe, so they are already incorporated. The same overlinking errors occur and are suppressed in the libnjpeg feedstock. This is why I have added the conda-forge.yml to this recipe which supressed that error. Unfortunately, staged-recipes is not a perfect replica of the end feedstock, so it is ignored for now.
|
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/nvjpeg2k/meta.yaml:
For recipes/nvtiff/meta.yaml:
For recipes/nvtiff/meta.yaml:
|
@adibbley, please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using the targetsDir
layout necessary for these packages? This is generally only used by packages included in cuda. The rpms/debs for these libraries install to system standard locations, such as:
$ rpm -qpl libnvjpeg2k0-cuda-12-0.8.0.38-1.x86_64.rpm
warning: libnvjpeg2k0-cuda-12-0.8.0.38-1.x86_64.rpm: Header V4 RSA/SHA512 Signature, key ID d42d0685: NOKEY
/usr/lib64/libnvjpeg2k
/usr/lib64/libnvjpeg2k/12
/usr/lib64/libnvjpeg2k/12/libnvjpeg2k.so.0
/usr/lib64/libnvjpeg2k/12/libnvjpeg2k.so.0.8.0.38
/usr/share/licenses/libnvjpeg2k0-cuda-12
/usr/share/licenses/libnvjpeg2k0-cuda-12/LICENSE
Note: libnvjpeg2k/12
subdir is used as those formats support side by side installs of the cuda variants, which is not needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume by "cuda" you mean the "cuda toolkit"?
Is using the targetsDir layout necessary for these packages?
You tell me! I believe the goal is to be consistent with layout on other platforms, so this is the feedback I'm looking for.
When you have a moment, could you please add a os_version:
linux_64: cos7
linux_aarch64: cos7 It looks like This will help up track which OS version these binaries support and help us update to new OS versions/images when ready |
Hi! This is the staged-recipes linter and I found some lint. It looks like some changes were made outside the If these changes are intentional (and you aren't submitting a recipe), please add a File-specific lints and/or hints:
|
649517b
to
c1f444c
Compare
The builds will not succeed because nvtiff requires a later version of glibc than the containers in staged recipes provide. |
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if you can really use alma8 at this point ? My understanding is conda-forge/conda-forge.github.io#1941 and conda-forge/conda-forge-pinning-feedstock#6626 need to be merged first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config parameter doesn't work yet which is why the docker_image
key is being used in the conda_build_config.yml. However, John wants it here so that when the new pinnings go live, there is no accidental building against to alma9.
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).Closes #28216
Closes #28217