-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[docs] update installation guide #6696
base: master
Are you sure you want to change the base?
Conversation
docs/_static/js/script.js
Outdated
@@ -23,13 +23,15 @@ $(function() { | |||
|
|||
/* Collapse specified sections in the installation guide */ | |||
if(window.location.pathname.toLocaleLowerCase().indexOf('installation-guide') != -1) { | |||
$('<style>.closed, .opened {cursor: pointer;} .closed:before, .opened:before {font-family: FontAwesome; display: inline-block; padding-right: 6px;} .closed:before {content: "\\f078";} .opened:before {content: "\\f077";}</style>').appendTo('body'); | |||
$('<style>.closed, .opened {cursor: pointer;} .closed:before, .opened:before {font-family: FontAwesome; display: inline-block; padding-right: 6px;} .closed:before {content: "\\f054";} .opened:before {content: "\\f078";}</style>').appendTo('body'); |
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 that chevron-right: >
for closed section and chevron-down: v
for opened section are more intuitive.
Maybe it's too verbose to list variants with |
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.
Thank you SO MUCH for going through these instructions and for all the fixes! The instructions that involving GUIs and Windows are particularly challenging to test, so I really appreciate you fixing them.
I agree with your comment...it's too verbose to have an "and Ninja" section for so many of these, which is identical to other sections but just with -G Ninja
added to the configure step.
And in general, I think this installation guide was already much too verbose even before this pull request.
Some examples:
- repeating how to install CMake many many times
- (there are 20 uses of
brew install cmake
indocs/Installation-Guide.rst
on this branch, for example) - we should just have a "Prerequisites" section that says CMake is required and links to docs on how to install it
- (there are 20 uses of
clang
andgcc
examples are nearly identical- they just differ by setting
CC
andCXX
but otherwise are identical... that's a lot of duplication that could be avoided by having a section on choosing a compiler near the top
- they just differ by setting
- repetition of CMake options, and no central listing of them
- there are a few paragraphs of text mentioning some CMake options (like
BUILD_STATIC_LIB
), and then others are in the named sections, often repeated multiple times across operating system / compiler - something like https://llvm.org/docs/CMake.html#options-and-variables would be better, I think
- there are a few paragraphs of text mentioning some CMake options (like
I think it's possible to significantly reduce the complexity of this document while still providing all the information people need to build LightGBM.
What do you think about this plan?
- remove all the "and Ninja" stuff from this PR
- I'll review the rest of the changes (which seem to preserve the structure of the document), and then we merge this
- I'll put up a PR (or maybe set of PRs) proposing some of the simplifications I'm describing above, starting with removing duplicate "install CMake" instructions
@jameslamb Thanks a lot for your detailed response! Really appreciate it.
Agree with this statement, but would like to add one important (IMHO) note
In other words, try to not hurt readability during reducing duplication. So that newcomers won't be needed to scroll up and down between sections with Prerequisites and actual installation commands to just build CUDA version, for example.
Nice plan! I'll go further with |
Sure, that makes sense. |
All instructions below are aimed at compiling the 64-bit version of LightGBM. | ||
It is worth compiling the 32-bit version only in very rare special cases involving environmental limitations. | ||
The 32-bit version is slow and untested, so use it at your own risk and don't forget to adjust some of the commands below when installing. | ||
|
||
By default, instructions below will use **VS Build Tools** or **make** tool to compile the code. | ||
It it possible to use `Ninja`_ tool instead of make on all platforms, but VS Build Tools cannot be replaced with Ninja. |
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 VS Build Tools cannot be replaced with Ninja.
Actually, it can be. But with some headache.
OK, I just removed all Ninja sections in the latest commit. |
Actually, I don't mind removing
|
Add all supported installation options and at the same time try to keep guide as simple and compact as possible.
Every installation method was tested either manually (Windows, CUDA) or with the help of our CI.
P.S. Those checks are causes of recently proposed PRs: #6676, #6689, #6690, #6688.
Closed #6666.