-
-
Notifications
You must be signed in to change notification settings - Fork 726
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
Overhaul Building the Documentation section for clarity #1038
Overhaul Building the Documentation section for clarity #1038
Conversation
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.
Looks good! Tested out in a new clone as well on Mac and worked as expected.
|
||
sphinx-build -b<builder> . build/<builder> | ||
python -m sphinx -b html . build/html |
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.
Also -j auto
to 'build in parallel with N processes where possible (special value "auto" will set N to cpu-count)'?
python -m sphinx -b html . build/html | |
python -m sphinx -b html . build/html -j auto |
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.
Hmm, perhaps, but since this is primarily intended to provide a minimal working invocation specifically for advanced users who want to pass their own custom options anyway, I'm not sure whether it makes sense to add here, considering it is entirely optional, can easily be added if desired, doesn't appear to do anything on Windows (the platform most likely to use it), and may not make sense for many use cases for which the direct invocation would be used, e.g. rebuilding only a single file or running on CI.
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.
Shame it doesn't work on Windows.
So it will either do nothing or make things much quicker: 62s -> 41s on my machine, which I think is a good win without much complexity. Especially if this bit is intended for advanced users.
And some Linux/Mac users who could use make
will prefer to call things directly.
Anyway, I don't mind too much, take your pick here too!
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'm kinda torn here...it's nice that it speeds things up for some people, but adding extra optional, non-self-documenting arguments to the baseline invocation causes me a bit of concern over complicating things for users, and them retaining it in cases where it is not helpful (e.g. rebuilding only one file). Though, on the other hand, under the same assumption that users care about the arguments they are passing, they could just as easily be aware of what -j auto
does, or be looking up the arguments anyway.
Shame it doesn't work on Windows.
My understanding from what Adam told me is its due to lack of fork
support, which makes spinning up new processes very expensive and rarely worth the trouble for the types of workloads Sphinx is typically run on.
And some Linux/Mac users who could use
make
will prefer to call things directly.
FWIW, I have a working make
installed through Rtools on Windows (and also exclusively use Git Bash), and use it myself to build the Spyder Docs I maintain (since we have a fairly complex workflow for multi-version support), but don't use it for the CPython-related projects that use it to build docs, since it assumes an in-tree venv (whereas I'm using conda environments), I need precise control over both the Python invocation and the arguments passed to Sphinx, and I prefer a consistent invocation and behavior, with no extra magic, across the many docs projects I work on.
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.
Does python -m sphinx
not accept --parallel
, as a synonym for -j
? If not (and AFAICT it truly, in fact, does not), that's probably something that should be fixed (in sphinx), and would at least help to address the "non-self-documenting" aspect.
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've submitted sphinx-doc/sphinx#11169 to do just that.)
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.
-j
is usually a shortcut for --jobs
. From make --help
:
-j [N], --jobs[=N] Allow N jobs at once; infinite jobs with no arg.
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
52ccb54
to
5f4ccf2
Compare
It was pointed out[1] that `-j auto` (or any other use of `-j`) isn't particularly self-documenting, as command-line arguments go. [1]: python/devguide#1038 (comment) Signed-off-by: FeRD (Frank Dana) <ferdnyc@gmail.com>
As discussed in #1037 , the Building the documentation section is potentially confusing and difficult to follow to a new contributor (i.e. the primary target audience), as it lacks a clear, concrete, step by step structure, muddles minor or advanced details and elaboration with key basic information, and has some out of date information (sphinx-build, dependency names/versions, third-party virtualenv, etc), which also isn't in keeping with the Diataxis principles for guides like this.
Furthermore, as @peterjpxie mentioned, it manually lists certain dependencies and states that they are required to be installed no less than three times (including twice in the intro paragraph), which can naturally confuse the contributor into thinking they need to install such themselves first, which is instead handled for the user automatically with
make venv
/runningmake.bat
, and should be installed via therequirements.txt
rather than manually otherwise. Furthermore, it doesn't at least link to how to create and activate the necessary environment on Windows and non-make usage, nor state how to install the required dependencies or link to the canonical listing of them (in therequirements.txt
).Therefore, this overhauls the section to address the above issues, with the following major changes (not exhaustive):
requirements.txt
when appropriateDoc
, and the user shouldcd
to it if not, instead of an inconstantly either repeatingcd Docs
, assuming it's been run, or assuming the opposite.virtualenv
package, include a short section explicitly stating how to create a venv with the Makefile, or otherwise, linking to the up-to-date section in the PyPUG describing venv creation and activation, and more explicitly remind Windows users they need to manually activate it.make.bat
into the above section, making the actual Build with make/make.bat section work the same on *nix and Windowsrequirements.txt
(instead of manually listing them yet again), and follow modern best practices with the invocation.Fixes #1037