-
-
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 nsight-compute recipe #23001
Add nsight-compute recipe #23001
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: jakirkham <[email protected]>
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 Alex! 🙏
Had a couple questions below
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.
Small request for testing the executable, otherwise please apply @jakirkham's suggestions.
nsightVersion=$(echo $PKG_VERSION | cut -d. -f1-3) | ||
nsightLib32="nsight-compute/${nsightVersion}/target/linux-desktop-glibc_2_11_3-x86/" | ||
|
||
# Remove 32bit libraries | ||
[[ -d "${nsightLib32}" ]] && rm -rf ${nsightLib32} |
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.
FWIW this could probably be generalized using something like
set +e
rm -rf nsight-compute/*/target/*x86
set -e
The set would only be needed if a nonzero exit code is problematic.
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.
IMO when it comes to removing things from the build I would rather keep this specific, so if something gets added we aren't removing it without knowing.
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.
LGTM once open conversations are resolved.
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 Alex! 🙏
Maybe we can try adding wrapper scripts to call the .exe
s (as below). This is a strategy we have used for similar situations in the past (though it is always possible the code below needs additional tweaks)
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.
Suggesting dropping the missing_dso_whitelist
portion since this may already be addressed by dependencies since added. Even if not, would be good if we can solve this with adding dependencies instead to gain some confidence that this will work in a user's environment
Co-authored-by: jakirkham <[email protected]>
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 Alex! 🙏
Think we need to add these dependencies based on the linking errors in CI. Though we may need more. Will need to look at CI based on the results after adding these
Co-authored-by: jakirkham <[email protected]>
Thanks all! 🙏 Let's get this one out. We can follow up on anything else in the feedstock 🙂 |
xref: #21382
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).