Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Shim package to interact with an **existing** NVCC install #8229
Shim package to interact with an **existing** NVCC install #8229
Changes from 14 commits
807386f
9dafb78
43e8080
2fa0d01
8028f5a
2b0d199
d80b660
0c2fa29
18ef258
c4d7c8c
3cf1715
cabbd8b
a4b5c68
6ed2727
f0d805c
2a40df8
52439ef
4cc4562
3ca9de4
15f7122
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can you add a
recipe/conda_build_config.yaml
with,and use the value like
set version = CUDA_VER
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.
The
9.2
is really only here to make the linter happy. Personally I'd rather not have it here at all if it could be avoided.Basically what this is doing is picking up
CUDA_VER
from the docker image.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.
You need the
run_exports
of the C++ compiler runtime here as well.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.
Why? If the user is using the C++ compiler, that will be added by this anyways.
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.
But the runtimes like
libgcc-ng
andlibstdcxx-ng
will not be added.run_exports
is not transitive.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 don't think users should be relying on
nvcc
to add these things. If they are using a C or C++ compiler, they should list those requirements explicitly in their 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.
Then the C and C++ compiler should not be in run requirements. The way the compiler package is set up is that if the user tries to use a C++ compiler in the docker image, it will not be found if it is not an explicit requirement. Having the explicit requirement means that the
run_exports
forlibgcc-ng
is added.But now, if the user adds
nvcc_linux-64
, they get the C++ compiler, but norun_exports
forlibgcc-ng
.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.
That makes sense. Have dropped the C and C++ compiler from
run
. They won't be installed now. The test has been updated to list C and C++ as requirements of the test.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.
Can you remove this line? It should have been run already
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 could, but this makes it clearer to the reader what we are testing. Explicit is better than implicit.
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.
But then, we are not testing that
CUDA_HOME
is automatically set. We are checking thatCUDA_HOME
is set when that script is run.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 see your point. That said, do these test modifications need to be handled in
staged-recipes
or can they be handled in the feedstock? Note that more work will need to be done in the feedstock anyways.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.
Okay. If you fix the other issue, then I'll merge. Also, please wait for my review on a PR in the feedstock before this package is released.
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.
As a higher level comment, making these suggestions is good. Just want to make sure we are progressing on the larger objectives they sit within as well.
I switched things around so now we test activation works without running the activation script first. Also added an explicit test of the activation script after running/testing the deactivation script. This also allowed a little bit of cleanup around the build step. So hopefully a bit nicer. Though we can discuss more 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.
Please add me as well.
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.
Sure would be happy to have your help here. 🙂 Added you in the last commit.