-
-
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
Add libcufile recipe #21908
Add libcufile recipe #21908
Conversation
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/libcufile:
|
cc @jakirkham @adibbley xref #21382 |
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 Matthew! 🙏
Had a couple initial suggestions below. Should fix the linter error
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/libcufile:
|
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
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/libcufile:
|
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.
Had a couple suggestions on the recent linter comments that should hopefully address them
recipes/libcufile/meta.yaml
Outdated
files: | ||
- lib/libcufile*.so | ||
- include/cufile* | ||
- man |
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.
@adibbley, what do you think about including man
in libcufile
? Wonder if it is helpful for the end user to have access to the man
files for configuration, etc.
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.
Yes it should be there, it is in the 12.0.0 branch of the original recipe.
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.
- man |
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/libcufile:
|
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/libcufile/build.bat
Outdated
@@ -0,0 +1,17 @@ | |||
if not exist %PREFIX% mkdir %PREFIX% |
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.
We don't need this Windows build script, but we do need a linux one to set up the targets directory:
https://github.com/conda-forge/staged-recipes/pull/21723/files#diff-e5bafa1dffa25db6c210c96887404cd7168b4c02772daf9fa5cbedb52b5fbd0e
The pkgconfig name will need to be updated to "cufile*.pc"
And the ppc64le line can be dropped
Co-authored-by: adibbley <103537006+adibbley@users.noreply.github.com>
Co-authored-by: adibbley <103537006+adibbley@users.noreply.github.com>
Co-authored-by: adibbley <103537006+adibbley@users.noreply.github.com>
Co-authored-by: adibbley <103537006+adibbley@users.noreply.github.com>
Co-authored-by: adibbley <103537006+adibbley@users.noreply.github.com>
Co-authored-by: adibbley <103537006+adibbley@users.noreply.github.com>
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 ( |
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
recipes/libcufile/meta.yaml
Outdated
- {{ pin_subpackage("libcufile", max_pin="x") }} | ||
files: | ||
- lib/libcufile*.so.* | ||
- etc/cufile.json |
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.
Not sure if it's already been checked, but does cuFile use the file put here as expected?
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 is a good question. Will ask
It's possible we would need to set CUFILE_ENV_PATH_JSON
for this to work. That could be done with activation script
Suppose a fair question would be whether we want to do this or whether users would rather maintain their own configuration file
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.
It looks like we don't need this file. So we dropped it to avoid confusion
We can always revisit this in the feedstock
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 Keith! 🙏
Agree with moving man
to -dev
. Have suggested some changes for that
Following up on cufile.json
offline
recipes/libcufile/meta.yaml
Outdated
- {{ pin_subpackage("libcufile", max_pin="x") }} | ||
files: | ||
- lib/libcufile*.so.* | ||
- etc/cufile.json |
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 is a good question. Will ask
It's possible we would need to set CUFILE_ENV_PATH_JSON
for this to work. That could be done with activation script
Suppose a fair question would be whether we want to do this or whether users would rather maintain their own configuration file
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
Co-authored-by: jakirkham <jakirkham@gmail.com>
Revise libcufile recipe / fix tests.
Thanks all! 🙏 Let's follow up on anything else in the feedstock 🙂 |
This recipe is based on the libcufile recipe in the nvidia conda channel. This is my first time submitting a recipe.
Note: I noticed the nvidia conda channel recipe doesn't have a
test
. Is that expected?Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).