-
-
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
Adding cuda-nvtx recipe #22052
Adding cuda-nvtx recipe #22052
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 ( |
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
- Add target_name - Add arm-variant - More explicit tests - About section in every output
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.
Recipe looks fine aside from minor suggestions on doc_url
. I haven't checked the package contents yet.
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Currently we have This in turn is used by With this PR, we would now have This situation is somewhat reminiscent of the CCCL situation we just worked through Wonder if we should do something to...
Would be interested in hearing thoughts from folks here 🙂 |
cc @shwina (who may also have thoughts on this) |
We could change the |
I support @shwina’s plan. |
Generally that seems reasonable. One suggestion would be to introduce a conflict in |
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.
One change request.
Also, do we need to make changes to nvtx-c before we merge this? CC @jakirkham
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.
Approving assuming the nvtx-c conflict is addressed at some point.
I think we need a repodata patch on nvtx-c to ensure these don't clobber each other and then we can maybe consider making it an alias? Additionally, what is the relationship and versioning in relation to the GitHub repo https://github.com/NVIDIA/NVTX? |
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.
To avoid clobbering nvtx-c
, suggesting that we place the contents here in targets
on Windows as we did with cuda-cccl
(for a similar reason)
Co-authored-by: jakirkham <jakirkham@gmail.com>
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.
Noticed a clobbering issue regarding headers on Windows CI
In looking into this, it appears cuda-nvtx
may not actually be needed on Windows since there are no .dll
s in the redist
or the package here and the headers are already in cuda-nvtx-dev
Suggesting we disable cuda-nvtx
builds on Windows
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.
If we drop cuda-nvtx
on Windows, there would be some follow up changes needed. Added those below
files: | ||
- lib/libnv*.so.* # [linux] | ||
- targets/{{ target_name }}/lib/*.so.* # [linux] |
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.
Maybe this is a better approach for the clobbering issue on Windows?
files: | |
- lib/libnv*.so.* # [linux] | |
- targets/{{ target_name }}/lib/*.so.* # [linux] | |
files: [] # [win] | |
files: # [linux] | |
- lib/libnv*.so.* # [linux] | |
- targets/{{ target_name }}/lib/*.so.* # [linux] |
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/cuda-nvtx:
|
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 ( |
Resolved the Windows clobbering issues by dropping |
Thanks all! 🙏 Let's follow up on anything else in the feedstock 🙂 |
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).