| Message ID | 20220823084719.13613-1-quentin@isovalent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: song@kernel.org martin.lau@linux.dev |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-16 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
Hi Quentin, On 8/23/22 10:47, Quentin Monnet wrote: > The bpf-helpers(7) manual page shipped in the man-pages project is > generated from the documentation contained in the BPF UAPI header, in > the Linux repository, parsed by script/bpf_doc.py and then fed to > rst2man. > > After a recent update of that page [0], Alejandro reported that the > linter used to validate the man pages complains about the generated > document [1]. The header for the page is supposed to contain some > attributes that we do not set correctly with the script. This commit > updates some of them; please refer to the previous discussion for the > meaning of those fields and the value we use (tl;dr: setting "Version" > to "Linux" seems acceptable). > > Before: > > $ ./scripts/bpf_doc.py helpers | rst2man | grep '\.TH' > .TH BPF-HELPERS 7 "" "" "" > > After: > > $ ./scripts/bpf_doc.py helpers | rst2man | grep '\.TH' > .TH BPF-HELPERS 7 "" "Linux" "Linux Programmer's Manual" > > Note that this commit does not update the date field. This date should > ideally be updated when generating the page to the date of the last edit > of the documentation (which we can maybe approximate to the last edit of > the BPF UAPI header). There is a --date option in rst2man; it does not > update that field, but Alejandro raised an issue about it [2] so it > might do in the future. Anyway, we just leave the date empty for now. > > [0] https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man7/bpf-helpers.7?id=19c7f78393f2b038e76099f87335ddf43a87f039 > [1] https://lore.kernel.org/all/20220721110821.8240-1-alx.manpages@gmail.com/t/#m8e689a822e03f6e2530a0d6de9d128401916c5de > [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1016527 > > Cc: Alejandro Colomar <alx.manpages@gmail.com> > Reported-by: Alejandro Colomar <alx.manpages@gmail.com> > Signed-off-by: Quentin Monnet <quentin@isovalent.com> Heh, we very recently changed the .TH line in the Linux man-pages for consistency with tradition and most other manual pages out there): <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=7bd6328fd40871ad75cbc3b6aa5d4a4b70f53ac7> <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=45186a5da74285d72199744eb5d2888fe348f680> So, we now omit the last (5th) argument to .TH, and the Version one really contains a version now. I'll comment below with what I think you should do. An example may show it better: $ grep ^.TH <man2/membarrier.2 .TH MEMBARRIER 2 2021-08-27 "Linux man-pages (unreleased)" Of course, that '(unreleased)' is replaced by the actual version at the time of `make dist` (creating the tarball). > --- > scripts/bpf_doc.py | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py > index dfb260de17a8..e66ef4f56e95 100755 > --- a/scripts/bpf_doc.py > +++ b/scripts/bpf_doc.py > @@ -378,6 +378,8 @@ list of eBPF helper functions > ------------------------------------------------------------------------------- > > :Manual section: 7 > +:Manual group: Linux Programmer's Manual Remove "Manual group" completely. If we don't specify that, groff(1) (or mandoc(1)) will produce sane defaults. For section 7, it uses "Miscellaneous Information Manual". I will report a bug to rst2man(1) that it shouldn't leave the field as "" if not specified, but it should just not add the field at all if not specified. > +:Version: Linux You could append the version here. Or maybe put a placeholder that the script should fill with information from the makefile or git-describe(1)? > > DESCRIPTION > =========== Cheers, Alex
On 23/08/2022 13:26, Alejandro Colomar wrote: > Hi Quentin, > > On 8/23/22 10:47, Quentin Monnet wrote: >> The bpf-helpers(7) manual page shipped in the man-pages project is >> generated from the documentation contained in the BPF UAPI header, in >> the Linux repository, parsed by script/bpf_doc.py and then fed to >> rst2man. >> >> After a recent update of that page [0], Alejandro reported that the >> linter used to validate the man pages complains about the generated >> document [1]. The header for the page is supposed to contain some >> attributes that we do not set correctly with the script. This commit >> updates some of them; please refer to the previous discussion for the >> meaning of those fields and the value we use (tl;dr: setting "Version" >> to "Linux" seems acceptable). >> >> Before: >> >> $ ./scripts/bpf_doc.py helpers | rst2man | grep '\.TH' >> .TH BPF-HELPERS 7 "" "" "" >> >> After: >> >> $ ./scripts/bpf_doc.py helpers | rst2man | grep '\.TH' >> .TH BPF-HELPERS 7 "" "Linux" "Linux Programmer's Manual" >> >> Note that this commit does not update the date field. This date should >> ideally be updated when generating the page to the date of the last edit >> of the documentation (which we can maybe approximate to the last edit of >> the BPF UAPI header). There is a --date option in rst2man; it does not >> update that field, but Alejandro raised an issue about it [2] so it >> might do in the future. Anyway, we just leave the date empty for now. >> >> [0] >> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man7/bpf-helpers.7?id=19c7f78393f2b038e76099f87335ddf43a87f039 >> [1] >> https://lore.kernel.org/all/20220721110821.8240-1-alx.manpages@gmail.com/t/#m8e689a822e03f6e2530a0d6de9d128401916c5de >> [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1016527 >> >> Cc: Alejandro Colomar <alx.manpages@gmail.com> >> Reported-by: Alejandro Colomar <alx.manpages@gmail.com> >> Signed-off-by: Quentin Monnet <quentin@isovalent.com> > > Heh, we very recently changed the .TH line in the Linux man-pages for > consistency with tradition and most other manual pages out there): > > <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=7bd6328fd40871ad75cbc3b6aa5d4a4b70f53ac7> > <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=45186a5da74285d72199744eb5d2888fe348f680> Wow, I'm two days late! > > So, we now omit the last (5th) argument to .TH, > and the Version one really contains a version now. > I'll comment below with what I think you should do. > > An example may show it better: > > $ grep ^.TH <man2/membarrier.2 > .TH MEMBARRIER 2 2021-08-27 "Linux man-pages (unreleased)" > > Of course, that '(unreleased)' is replaced by the actual version at the > time of `make dist` (creating the tarball). > > >> --- >> scripts/bpf_doc.py | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py >> index dfb260de17a8..e66ef4f56e95 100755 >> --- a/scripts/bpf_doc.py >> +++ b/scripts/bpf_doc.py >> @@ -378,6 +378,8 @@ list of eBPF helper functions >> >> ------------------------------------------------------------------------------- >> :Manual section: 7 >> +:Manual group: Linux Programmer's Manual > > Remove "Manual group" completely. If we don't specify that, groff(1) > (or mandoc(1)) will produce sane defaults. For section 7, it uses > "Miscellaneous Information Manual". > > I will report a bug to rst2man(1) that it shouldn't leave the field as > "" if not specified, but it should just not add the field at all if not > specified. > >> +:Version: Linux > > You could append the version here. Or maybe put a placeholder that the > script should fill with information from the makefile or git-describe(1)? So if I understand correctly, running bpf_doc.py should currently produce the following string: .TH BPF-HELPERS 7 "" "Linux (5.19.0)" "" Is this what you expect? I can make the script call "make kernelversion" to produce the above. I'm not 100% convinced it should be the role of that script vs. when copying it (we risk having some inaccuracies, for example I generated the above from the bpf-next, so it doesn't really correspond to 5.19), but maybe it's easier that way and avoids adding another script in the middle of the generation so OK. Thanks, Quentin
Hi Quentin, On 8/23/22 16:23, Quentin Monnet wrote: > Wow, I'm two days late! ;-) [...] >> >> You could append the version here. Or maybe put a placeholder that the >> script should fill with information from the makefile or git-describe(1)? > > So if I understand correctly, running bpf_doc.py should currently > produce the following string: > > .TH BPF-HELPERS 7 "" "Linux (5.19.0)" "" > > Is this what you expect? Almost. I expect: .TH BPF-HELPERS 7 "" "Linux 5.19.0" Notice the differences: - No 5th empty field "". - No parentheses in the version string. But since you're not in control of the last field, and it's just a bug in rst2man, an empty 5th arg as you suggested is the best you can provide. I'll file the bug. As for the parentheses in the version, I wouldn't put them; I only put them in the man-pages unreleased string, to make it clear that it's a version, but in the final version I remove them. See: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/lib/version.mk> and <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/lib/dist.mk#n33> > > I can make the script call "make kernelversion" to produce the above. > I'm not 100% convinced it should be the role of that script vs. when > copying it (we risk having some inaccuracies, for example I generated > the above from the bpf-next, so it doesn't really correspond to 5.19), > but maybe it's easier that way and avoids adding another script in the > middle of the generation so OK. I like it that way. Moreover, I'll always run this script from a release tag from Linus' repo, so the version should match exactly the code. Anyway, and I think this affects many projects out there have the same issue: The versioning should always be correct. git-describe(1) should be preferred, or in absence of that, a generic (unreleased) string should be used. Describing any commit after v5.18 and before v5.19-rc1 to be '5.18.0' is plain wrong/misleading. The Makefile should probably autogenerate that info from git-describe(1). See how the Linux man-pages do it (in the links above you can see it) for example. Cheers, Alex
On 23/08/2022 16:04, Alejandro Colomar wrote: > Hi Quentin, > > On 8/23/22 16:23, Quentin Monnet wrote: >> Wow, I'm two days late! > > ;-) > > [...] >>> >>> You could append the version here. Or maybe put a placeholder that the >>> script should fill with information from the makefile or >>> git-describe(1)? >> >> So if I understand correctly, running bpf_doc.py should currently >> produce the following string: >> >> .TH BPF-HELPERS 7 "" "Linux (5.19.0)" "" >> >> Is this what you expect? > > Almost. I expect: > > .TH BPF-HELPERS 7 "" "Linux 5.19.0" > > Notice the differences: > - No 5th empty field "". > - No parentheses in the version string. > > But since you're not in control of the last field, and it's just a bug > in rst2man, an empty 5th arg as you suggested is the best you can > provide. Agreed > I'll file the bug. > > As for the parentheses in the version, I wouldn't put them; I only put > them in the man-pages unreleased string, to make it clear that it's a > version, but in the final version I remove them. OK, I wasn't sure. Thanks for the details. > See: > > <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/lib/version.mk> > and > <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/lib/dist.mk#n33> > >> >> I can make the script call "make kernelversion" to produce the above. >> I'm not 100% convinced it should be the role of that script vs. when >> copying it (we risk having some inaccuracies, for example I generated >> the above from the bpf-next, so it doesn't really correspond to 5.19), >> but maybe it's easier that way and avoids adding another script in the >> middle of the generation so OK. > > I like it that way. Moreover, I'll always run this script from a > release tag from Linus' repo, so the version should match exactly the code. > > Anyway, and I think this affects many projects out there have the same > issue: > > The versioning should always be correct. git-describe(1) should be > preferred, or in absence of that, a generic (unreleased) string should > be used. Describing any commit after v5.18 and before v5.19-rc1 to be > '5.18.0' is plain wrong/misleading. The Makefile should probably > autogenerate that info from git-describe(1). See how the Linux > man-pages do it (in the links above you can see it) for example. It's not really about how to do it, more that I don't want to have a hard dependency on git for the script. I want it to run just as well when there's no Git repo around. But I can try to run "git describe" and fall back on the Makefile (or on an empty version) if the command fails. OK I'll prepare a new version, thanks for your help. Quentin
Hi Quentin, On 8/23/22 17:18, Quentin Monnet wrote: >> The versioning should always be correct. git-describe(1) should be >> preferred, or in absence of that, a generic (unreleased) string should >> be used. Describing any commit after v5.18 and before v5.19-rc1 to be >> '5.18.0' is plain wrong/misleading. The Makefile should probably >> autogenerate that info from git-describe(1). See how the Linux >> man-pages do it (in the links above you can see it) for example. > > It's not really about how to do it, more that I don't want to have a > hard dependency on git for the script. I want it to run just as well > when there's no Git repo around. But I can try to run "git describe" and > fall back on the Makefile (or on an empty version) if the command fails. Yeah, I didn't mean it's a bug in the script, but seems to me like a bug in the kernel Makefile. If git fails because it's not present, there could be a fallback like "unknown-version". In the man-pages, when I prepare the tarball, with `make dist`, I hardcode the version in the pages. Maybe the kernel Makefile should have unreleased as the version in the git repo, and when a tarball is generated the Makefile should replace that unreleased version by the appropriate git version. Since git will not be found in the tarballs, that version will be used in that case. But when running from git, git-describe(1) should be preferred. > > OK I'll prepare a new version, thanks for your help. It's a pleasure! Cheers, Alex > Quentin
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index dfb260de17a8..e66ef4f56e95 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -378,6 +378,8 @@ list of eBPF helper functions ------------------------------------------------------------------------------- :Manual section: 7 +:Manual group: Linux Programmer's Manual +:Version: Linux DESCRIPTION ===========
The bpf-helpers(7) manual page shipped in the man-pages project is generated from the documentation contained in the BPF UAPI header, in the Linux repository, parsed by script/bpf_doc.py and then fed to rst2man. After a recent update of that page [0], Alejandro reported that the linter used to validate the man pages complains about the generated document [1]. The header for the page is supposed to contain some attributes that we do not set correctly with the script. This commit updates some of them; please refer to the previous discussion for the meaning of those fields and the value we use (tl;dr: setting "Version" to "Linux" seems acceptable). Before: $ ./scripts/bpf_doc.py helpers | rst2man | grep '\.TH' .TH BPF-HELPERS 7 "" "" "" After: $ ./scripts/bpf_doc.py helpers | rst2man | grep '\.TH' .TH BPF-HELPERS 7 "" "Linux" "Linux Programmer's Manual" Note that this commit does not update the date field. This date should ideally be updated when generating the page to the date of the last edit of the documentation (which we can maybe approximate to the last edit of the BPF UAPI header). There is a --date option in rst2man; it does not update that field, but Alejandro raised an issue about it [2] so it might do in the future. Anyway, we just leave the date empty for now. [0] https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man7/bpf-helpers.7?id=19c7f78393f2b038e76099f87335ddf43a87f039 [1] https://lore.kernel.org/all/20220721110821.8240-1-alx.manpages@gmail.com/t/#m8e689a822e03f6e2530a0d6de9d128401916c5de [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1016527 Cc: Alejandro Colomar <alx.manpages@gmail.com> Reported-by: Alejandro Colomar <alx.manpages@gmail.com> Signed-off-by: Quentin Monnet <quentin@isovalent.com> --- scripts/bpf_doc.py | 2 ++ 1 file changed, 2 insertions(+)