[PATCH bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page

[PATCH bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page

[Quentin Monnet]

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(+)

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
 ===========
-- 
2.25.1

Re: [PATCH bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 3826 bytes --]

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

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page

[Quentin Monnet]

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

Re: [PATCH bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2294 bytes --]

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


-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page

[Quentin Monnet]

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

Re: [PATCH bpf-next] scripts/bpf: Fix attributes for bpf-helpers(7) man page

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1622 bytes --]

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

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]