| Message ID | 20230824182107.3702766-2-nm@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Documentation: sphinx: Add sphinx-prompt | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-19 | success | Logs for test_progs_no_alu32_parallel on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-22 | success | Logs for test_progs_parallel on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-21 | success | Logs for test_progs_parallel on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-26 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-20 | success | Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-24 | success | Logs for test_verifier on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-27 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-28 | success | Logs for veristat |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-23 | success | Logs for test_progs_parallel on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-25 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-18 | success | Logs for test_progs_no_alu32_parallel on aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-0 | success | Logs for ShellCheck |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for aarch64 with gcc |
On 8/24/23 8:21 PM, Nishanth Menon wrote: > Sphinx-prompt[1] helps bring-in '.. prompt::' option that allows a > better rendered documentation, yet be able to copy paste without > picking up the prompt from the rendered documentation. > > [1] https://pypi.org/project/sphinx-prompt/ > Link: https://lore.kernel.org/all/87fs48rgto.fsf@baylibre.com/ > Suggested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Signed-off-by: Nishanth Menon <nm@ti.com> Given the patch 2/2 is targeted for bpf docs, we can route this via bpf-next. Jonathan, could we get an ack for this one if it looks good to you? Thanks, Daniel
Nishanth Menon <nm@ti.com> writes: > Sphinx-prompt[1] helps bring-in '.. prompt::' option that allows a > better rendered documentation, yet be able to copy paste without > picking up the prompt from the rendered documentation. > > [1] https://pypi.org/project/sphinx-prompt/ > Link: https://lore.kernel.org/all/87fs48rgto.fsf@baylibre.com/ > Suggested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Signed-off-by: Nishanth Menon <nm@ti.com> > --- > I would have added Reported-by for Simon, since he reported the issue in > the first place.. but it was for the u-boot documentation, so skipping > here. > > Documentation/conf.py | 2 +- > Documentation/sphinx/requirements.txt | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) So it would sure be nice for the changelog to say what this actually does. This appears to add a build dependency for the docs; we can't just add that without updating the documentation, adjusting scripts/sphinx-pre-install, and so on. But, beyond that, this extension goes entirely counter to the idea that the plain-text files are the primary form of documentation; it adds clutter and makes those files less readable. We can do that when the benefit is sufficient, but I'm pretty far from convinced that this is the case here. Certainly the case hasn't been made in the changelog. What *is* the benefit of making this change? Thanks, jon
Hi Jon, On 16:46-20230825, Jonathan Corbet wrote: > Nishanth Menon <nm@ti.com> writes: > > > Sphinx-prompt[1] helps bring-in '.. prompt::' option that allows a > > better rendered documentation, yet be able to copy paste without > > picking up the prompt from the rendered documentation. > > > > [1] https://pypi.org/project/sphinx-prompt/ > > Link: https://lore.kernel.org/all/87fs48rgto.fsf@baylibre.com/ > > Suggested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > > Signed-off-by: Nishanth Menon <nm@ti.com> > > --- > > I would have added Reported-by for Simon, since he reported the issue in > > the first place.. but it was for the u-boot documentation, so skipping > > here. > > > > Documentation/conf.py | 2 +- > > Documentation/sphinx/requirements.txt | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > So it would sure be nice for the changelog to say what this actually > does. All this does is to bring in a better rendered documentation when published in html format. https://youtu.be/ItjdVa59jjE shows how the "copy-paste" functionality is improved. If you could recommend changes, I'd be glad to incorporate the same. > > This appears to add a build dependency for the docs; we can't just add > that without updating the documentation, adjusting > scripts/sphinx-pre-install, and so on. I had checked scripts/shinx-pre-install and that picks up Documentation/sphinx/requirements.txt and installs the dependencies from there using pip. Am I missing something? Same thing with Documentation/doc-guide/sphinx.rst Am I missing something? > > But, beyond that, this extension goes entirely counter to the idea that > the plain-text files are the primary form of documentation; it adds > clutter and makes those files less readable. We can do that when the Are you sure this is going against the readable text documentation? If anything it reduces the clutter and allows the text doc to be copy-paste-able as well. https://lore.kernel.org/all/20230824182107.3702766-3-nm@ti.com/ As you see from the diffstat: 1 file changed, 10 insertions(+), 10 deletions(-) Nothing extra added. What kind of clutter are you suggesting we added with the change? prompt:: bash $ is clearly readable that this is prompt documentation in fact, dropping the "$" in the example logs, one can easily copy paste the documentation from rst files as well. > benefit is sufficient, but I'm pretty far from convinced that this is > the case here. Certainly the case hasn't been made in the changelog. > What *is* the benefit of making this change? Let me know *how* I can improve (note: I am not a native English speaker, so, I'd appreciate any suggestions to make the argument clear in the changelog). Intent here is to help make the rendered html documentation that we publish in docs.kernel.org such as https://docs.kernel.org/bpf/libbpf/libbpf_build.html better usable.
Nishanth Menon <nm@ti.com> writes: > Hi Jon, > > On 16:46-20230825, Jonathan Corbet wrote: >> So it would sure be nice for the changelog to say what this actually >> does. > > All this does is to bring in a better rendered documentation when > published in html format. > https://youtu.be/ItjdVa59jjE shows how the "copy-paste" functionality is > improved. Youtube references aren't a great way to explain the value of a patch; you'll find that maintainers will, in general, lack the time or inclination to follow them up. The patch should explain itself. >> This appears to add a build dependency for the docs; we can't just add >> that without updating the documentation, adjusting >> scripts/sphinx-pre-install, and so on. > > I had checked scripts/shinx-pre-install and that picks up > Documentation/sphinx/requirements.txt and installs the dependencies > from there using pip. Am I missing something? > > Same thing with Documentation/doc-guide/sphinx.rst > > Am I missing something? That works, I guess, but doesn't change the fact that you have added another docs build dependency. That will, among other things, break the build for anybody who is set up to do it now until they install your new package. That's not something we want to do without good reason. >> But, beyond that, this extension goes entirely counter to the idea that >> the plain-text files are the primary form of documentation; it adds >> clutter and makes those files less readable. We can do that when the > > Are you sure this is going against the readable text documentation? If > anything it reduces the clutter and allows the text doc to be > copy-paste-able as well. > > https://lore.kernel.org/all/20230824182107.3702766-3-nm@ti.com/ > > As you see from the diffstat: > 1 file changed, 10 insertions(+), 10 deletions(-) > > Nothing extra added. What kind of clutter are you suggesting we added > with the change? > > prompt:: bash $ is clearly readable that this is prompt documentation > in fact, dropping the "$" in the example logs, one can easily copy paste > the documentation from rst files as well. .. prompt:: is clutter. It also adds a bit of extra cognitive load to reading that part of the documentation. Quick copy-paste of multiple lines of privileged shell commands has never really been a requirement for the kernel docs; why do we need that so badly? I appreciate attempts to improve our documentation, and hope that you will continue to do so. I am far from convinced, though, that this change clears the bar for mainline inclusion. Thanks, jon
On 07:41-20230828, Jonathan Corbet wrote: > Nishanth Menon <nm@ti.com> writes: > > > Hi Jon, > > > > On 16:46-20230825, Jonathan Corbet wrote: > > >> So it would sure be nice for the changelog to say what this actually > >> does. > > > > All this does is to bring in a better rendered documentation when > > published in html format. > > https://youtu.be/ItjdVa59jjE shows how the "copy-paste" functionality is > > improved. > > Youtube references aren't a great way to explain the value of a patch; > you'll find that maintainers will, in general, lack the time or > inclination to follow them up. The patch should explain itself. > > >> This appears to add a build dependency for the docs; we can't just add > >> that without updating the documentation, adjusting > >> scripts/sphinx-pre-install, and so on. > > > > I had checked scripts/shinx-pre-install and that picks up > > Documentation/sphinx/requirements.txt and installs the dependencies > > from there using pip. Am I missing something? > > > > Same thing with Documentation/doc-guide/sphinx.rst > > > > Am I missing something? > > That works, I guess, but doesn't change the fact that you have added > another docs build dependency. That will, among other things, break the > build for anybody who is set up to do it now until they install your new > package. That's not something we want to do without good reason. True, and fair enough. > > >> But, beyond that, this extension goes entirely counter to the idea that > >> the plain-text files are the primary form of documentation; it adds > >> clutter and makes those files less readable. We can do that when the > > > > Are you sure this is going against the readable text documentation? If > > anything it reduces the clutter and allows the text doc to be > > copy-paste-able as well. > > > > https://lore.kernel.org/all/20230824182107.3702766-3-nm@ti.com/ > > > > As you see from the diffstat: > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > Nothing extra added. What kind of clutter are you suggesting we added > > with the change? > > > > prompt:: bash $ is clearly readable that this is prompt documentation > > in fact, dropping the "$" in the example logs, one can easily copy paste > > the documentation from rst files as well. > > .. prompt:: is clutter. It also adds a bit of extra cognitive load to > reading that part of the documentation. It is no additional cognitive load from what is already there: -.. code-block:: bash +.. prompt:: bash $ > > Quick copy-paste of multiple lines of privileged shell commands has > never really been a requirement for the kernel docs; why do we need that > so badly? Just hated people who read online documentation from having to spend extra few seconds in copy pasting and then realizing oops "$" came along with it. > > I appreciate attempts to improve our documentation, and hope that you > will continue to do so. I am far from convinced, though, that this > change clears the bar for mainline inclusion. :) OK - I tried.. Thanks for explaining (though I disagree).
On Mon, Aug 28, 2023 at 07:41:39AM -0600, Jonathan Corbet wrote: > Youtube references aren't a great way to explain the value of a patch; > you'll find that maintainers will, in general, lack the time or > inclination to follow them up. The patch should explain itself. I agree that the way this has been presented is awful. > > prompt:: bash $ is clearly readable that this is prompt documentation > > in fact, dropping the "$" in the example logs, one can easily copy paste > > the documentation from rst files as well. > > .. prompt:: is clutter. It also adds a bit of extra cognitive load to > reading that part of the documentation. > > Quick copy-paste of multiple lines of privileged shell commands has > never really been a requirement for the kernel docs; why do we need that > so badly? > > I appreciate attempts to improve our documentation, and hope that you > will continue to do so. I am far from convinced, though, that this > change clears the bar for mainline inclusion. I'd ask that you reconsider. Looking at patch 2, I prefer what is written there. I don't think it adds cognitive load when reading the plain docs. I find the "copy and paste from html" argument not very convincing, but I do like "copy and paste from rst", which this enables. I also have a certain fond memory of how the plan9 people set up 'rc' (their shell) so that ";" was both an empty statement, and the default prompt. So you could copy-paste lines starting with the ; prompt and they'd work. It's a small usabillity improvement, but it is there, and wow is it annoying when you don't have it any more.
Matthew Wilcox <willy@infradead.org> writes: > On Mon, Aug 28, 2023 at 07:41:39AM -0600, Jonathan Corbet wrote: >> I appreciate attempts to improve our documentation, and hope that you >> will continue to do so. I am far from convinced, though, that this >> change clears the bar for mainline inclusion. > > I'd ask that you reconsider. Looking at patch 2, I prefer what is > written there. I don't think it adds cognitive load when reading the > plain docs. I find the "copy and paste from html" argument not very > convincing, but I do like "copy and paste from rst", which this enables. Do you really think that the benefit from that justifies adding a build dependency and breaking everybody's docs build until they install it? I rather suspect I would hear back from people who feel otherwise if I did that... > I also have a certain fond memory of how the plan9 people set up 'rc' > (their shell) so that ";" was both an empty statement, and the default > prompt. So you could copy-paste lines starting with the ; prompt and > they'd work. It's a small usabillity improvement, but it is there, > and wow is it annoying when you don't have it any more. Ah, OK, so what we really need is a bash patch :) Thanks, jon
Em Mon, 28 Aug 2023 09:12:07 -0600 Jonathan Corbet <corbet@lwn.net> escreveu: > Matthew Wilcox <willy@infradead.org> writes: > > > On Mon, Aug 28, 2023 at 07:41:39AM -0600, Jonathan Corbet wrote: > >> I appreciate attempts to improve our documentation, and hope that you > >> will continue to do so. I am far from convinced, though, that this > >> change clears the bar for mainline inclusion. > > > > I'd ask that you reconsider. Looking at patch 2, I prefer what is > > written there. I don't think it adds cognitive load when reading the > > plain docs. I find the "copy and paste from html" argument not very > > convincing, but I do like "copy and paste from rst", which this enables. > > Do you really think that the benefit from that justifies adding a build > dependency and breaking everybody's docs build until they install it? I > rather suspect I would hear back from people who feel otherwise if I did > that... I agree with Jon: it needs at least a patch for scripts/sphinx-pre-install. Adding dependencies there is not the easiest thing to do, as one needs to test the change against all supported distros to ensure that the new package name will be the same everywhere. Also, if I'm not mistaken, some developers don't want to use pip to install packages, wanting instead to have the distro-provided package. Also, having an extra build dependency will surely break already-existing CI automation. Making the new dependency optional would be a way to go, but this will cause troubles at the html output after such change. > > I also have a certain fond memory of how the plan9 people set up 'rc' > > (their shell) so that ";" was both an empty statement, and the default > > prompt. So you could copy-paste lines starting with the ; prompt and > > they'd work. It's a small usabillity improvement, but it is there, > > and wow is it annoying when you don't have it any more. > > Ah, OK, so what we really need is a bash patch :) Probably the hardest part would be to do copy-and-paste on places where there are both shell prompt commands and their results. I'm pretty sure we have things like: some example:: $ run_some_command comand results line 1 comand results line 2 comand results line 3 ... comand results line n $ run_another_command does sphinx-prompt handle things like that, placing just: run_some_command run_another_command at the paste buffer, ignoring any command result lines? IMO, the above described usease is where having a prompt will help to identify what should be copied/pasted and what are the command results. I mean, if someone wants to just place the commands to run, he could write, instead: Run those shell commands to do something:: run_some_command run_another_command Regards, Mauro
Mauro Carvalho Chehab <mchehab@kernel.org> writes: > Adding dependencies there is not the easiest thing to do, as one needs to > test the change against all supported distros to ensure that the new package > name will be the same everywhere. Also, if I'm not mistaken, some developers > don't want to use pip to install packages, wanting instead to have the > distro-provided package. That, actually, is something we definitely need to keep in mind. The security record for PyPI (as with almost all of the language-specific repos) is not great. We need to think pretty hard before telling developers (or, say, the build process on kernel.org) that they need to install packages from there on their systems. Thanks, jon
diff --git a/Documentation/conf.py b/Documentation/conf.py index d4fdf6a3875a..2eff713c4728 100644 --- a/Documentation/conf.py +++ b/Documentation/conf.py @@ -55,7 +55,7 @@ needs_sphinx = '1.7' extensions = ['kerneldoc', 'rstFlatTable', 'kernel_include', 'kfigure', 'sphinx.ext.ifconfig', 'automarkup', 'maintainers_include', 'sphinx.ext.autosectionlabel', - 'kernel_abi', 'kernel_feat'] + 'kernel_abi', 'kernel_feat', 'sphinx-prompt'] if major >= 3: if (major > 3) or (minor > 0 or patch >= 2): diff --git a/Documentation/sphinx/requirements.txt b/Documentation/sphinx/requirements.txt index 335b53df35e2..24a59ceda582 100644 --- a/Documentation/sphinx/requirements.txt +++ b/Documentation/sphinx/requirements.txt @@ -1,3 +1,4 @@ # jinja2>=3.1 is not compatible with Sphinx<4.0 jinja2<3.1 Sphinx==2.4.4 +sphinx-prompt==1.5.0
Sphinx-prompt[1] helps bring-in '.. prompt::' option that allows a better rendered documentation, yet be able to copy paste without picking up the prompt from the rendered documentation. [1] https://pypi.org/project/sphinx-prompt/ Link: https://lore.kernel.org/all/87fs48rgto.fsf@baylibre.com/ Suggested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> Signed-off-by: Nishanth Menon <nm@ti.com> --- I would have added Reported-by for Simon, since he reported the issue in the first place.. but it was for the u-boot documentation, so skipping here. Documentation/conf.py | 2 +- Documentation/sphinx/requirements.txt | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)