-
-
Notifications
You must be signed in to change notification settings - Fork 8
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 documentation #32
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 ( |
I didn't bother bumping the build number since we don't need a new build for this, it's just documentation. |
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 have a few comments. Please resolve however you see fit. I am approving since having some docs is better than the current state, and it's always fine to iterate more later if we want to merge this sooner.
recipe/doc/recipe_guide.md
Outdated
This package is split into the `cuda-nvcc` package – which is architecture specific and must be installed on the appropriate target platform (e.g. | ||
x86-64 Linux) – and the `cuda-nvcc-${TARGET_PLATFORM}` packages – each of which is architecture-independent and may be installed on any target, but are only suitable for use in compiling code for the specified target platform. |
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 seems like this file is using manual line wraps but the others are not. Let's make sure this is consistent. I prefer semantic line breaks (each sentence on a line) if you need to make a choice.
On one hand, the packages aim to retain as similar a structure as possible to the CUDA packages that may be installed via system package manager (e.g. | ||
apt). | ||
On the other hand, the packages aim to provide a seamless experience at both build-time and runtime within conda environments. | ||
To satisfy the first requirement, all files in CUDA conda packages are installed into the `$PREFIX/targets` directory. |
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.
Should we clarify that this is really something like $PREFIX/targets/$TARGET_PLATFORM/
?
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 think we need a table for what $TARGET_PLATFORM
really means here. The same variable is expanded into different strings between the previous and this paragraphs.
recipe/doc/recipe_guide.md
Outdated
Package structure on Windows. | ||
Doesn’t have `x64` directory. |
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.
Let's just write TODO here if it's going to be so sparse. It's okay to have incomplete docs as long as it's clear that it's incomplete and not just a dead end / lost content.
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 actually a bug we haven't had a chance to address... (conda-forge/cuda-cudart-feedstock#28)
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.
Agreed, I'll remove.
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.
@vyasr By "remove" you meant removing the whole Windows section? It's gone now and it doesn't sound right to me. Windows CUDA packages are fully working.
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 haven't removed any indication that Windows is working, I've just removed the section on Windows-specific guidance for recipe maintainers because there wasn't much of note and we can fill that in later.
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 finished reviewing the maintainer guide.
recipe/doc/recipe_guide.md
Outdated
Package structure on Windows. | ||
Doesn’t have `x64` directory. |
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 actually a bug we haven't had a chance to address... (conda-forge/cuda-cudart-feedstock#28)
Co-authored-by: Bradley Dice <bdice@bradleydice.com> Co-authored-by: Leo Fang <leo80042@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.
I reviewed end_user_compile_guide.md
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 am less confident in the compiler package doc, maybe Rob or John could take a look?
Co-authored-by: Leo Fang <leo80042@gmail.com>
There are a couple of outstanding items that we've decided to wait on to address in follow-ups:
|
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.
A few nits / typos to fix, then I will merge this PR. Also, @leofang approved this PR offline.
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)This PR adds documentation for all of the CUDA packages for end-users, developers, and maintainers.