bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 300 bytes --]

Severity: wishlist

The attached patch adds a new check-man target to check our man pages
for errors.  I know we prefer info over man, but why not.  It will catch
mistakes, like using unknown macros, etc.

To test this, change a macro like ".B" to ".BBB" in doc/man/emacs.1 and
run "make check-man".

[-- Attachment #2: 0001-Add-new-check-man-target.patch --]
[-- Type: text/x-diff, Size: 1067 bytes --]

From cc83e75ad0bc02d003b14a789f15dc94c6ab7899 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 27 Nov 2022 16:03:31 +0100
Subject: [PATCH] Add new check-man target

* Makefile.in (check-man): New target to check man pages for errors.
---
 Makefile.in | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Makefile.in b/Makefile.in
index 93609a4e16..2d86df09bb 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -69,6 +69,10 @@
 #      check-expensive includes additional tests that can be slow.
 #      check-all runs all tests, including ones that can be slow, or
 #        fail unpredictably
+#
+# make check-man
+#      Check man pages for errors.
+#
 
 SHELL = @SHELL@
 
@@ -1322,6 +1326,15 @@ check-declare:
 	$(MAKE) -C lisp $@
 	$(MAKE) -C test $@
 
+.PHONY: check-man
+
+check-man:
+	@cd ${mansrcdir}; \
+	for page in *.1; do \
+	  LC_ALL=C.UTF-8 MANROFFSEQ='' MANWIDTH=80 \
+	  man --warnings -E UTF-8 -l -Tutf8 -Z "$$page" >/dev/null; \
+	done
+
 .PHONY: gitmerge
 
 GITMERGE_EMACS = ./src/emacs${EXEEXT}
-- 
2.35.1

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 27 Nov 2022 07:23:49 -0800
> 
> The attached patch adds a new check-man target to check our man pages
> for errors.  I know we prefer info over man, but why not.  It will catch
> mistakes, like using unknown macros, etc.
> 
> To test this, change a macro like ".B" to ".BBB" in doc/man/emacs.1 and
> run "make check-man".

Thanks, but I think it is better to have this as a script in admin/, not a
rule in the top-level Makefile.  Especially since it requires specialized
tools and setups.

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but I think it is better to have this as a script in admin/, not a
> rule in the top-level Makefile.  Especially since it requires specialized
> tools and setups.

OK, please find attached a new patch making this into a script instead.

[-- Attachment #2: 0001-New-script-admin-check-man-pages.patch --]
[-- Type: text/x-diff, Size: 1932 bytes --]

From 559b88afc3d56ee9d5116754e013383920290dea Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 27 Nov 2022 18:37:37 +0100
Subject: [PATCH] New script admin/check-man-pages

* admin/check-man-pages: New file with script used to check man pages
for errors.
---
 admin/check-man-pages | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100755 admin/check-man-pages

diff --git a/admin/check-man-pages b/admin/check-man-pages
new file mode 100755
index 0000000000..3f3190d495
--- /dev/null
+++ b/admin/check-man-pages
@@ -0,0 +1,44 @@
+#!/bin/bash
+### check-man-pages - check man pages for errors
+
+## Copyright (C) 2022 Free Software Foundation, Inc.
+
+## This file is part of GNU Emacs.
+
+## GNU Emacs is free software: you can redistribute it and/or modify
+## it under the terms of the GNU General Public License as published by
+## the Free Software Foundation, either version 3 of the License, or
+## (at your option) any later version.
+
+## GNU Emacs is distributed in the hope that it will be useful,
+## but WITHOUT ANY WARRANTY; without even the implied warranty of
+## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+## GNU General Public License for more details.
+
+## You should have received a copy of the GNU General Public License
+## along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+### Commentary:
+
+## Check our man pages for errors using `man'.
+
+### Code:
+
+source "${0%/*}/emacs-shell-lib"
+
+exit_status=0
+
+cd "$PD"/../doc/man
+for page in *.1; do
+    log=$(emacs_mktemp)
+    LC_ALL=C.UTF-8 MANROFFSEQ='' MANWIDTH=80 \
+        man --warnings -E UTF-8 -l -Tutf8 -Z "$page" >/dev/null 2> $log
+    log_size=$(stat --format=%s "$log")
+    if [ $log_size -ne 0 ]; then
+        echo "doc/man/$page:"
+        cat $log
+        exit_status=1
+    fi
+done
+
+exit $exit_status
-- 
2.35.1

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 27 Nov 2022 09:49:13 -0800
> Cc: 59631@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but I think it is better to have this as a script in admin/, not a
> > rule in the top-level Makefile.  Especially since it requires specialized
> > tools and setups.
> 
> OK, please find attached a new patch making this into a script instead.

Thanks, LGTM.

Maybe this should be mentioned in some notes, like make-tarball or
something?  When do we want to run this?

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe this should be mentioned in some notes, like make-tarball or
> something?  When do we want to run this?

I think adding it to make-tarball is a good idea.  Perhaps we could do
it in the same step as checking the refcards?  I've attached a new patch
with a suggested wording, please let me know what you think.

I also enabled a few more groff warnings, and cleaned up the output to
enable automatically jumping to errors from *compilation* buffers.

[-- Attachment #2: 0001-New-script-admin-check-man-pages.patch --]
[-- Type: text/x-diff, Size: 2816 bytes --]

From c3bcdbe7a71e95e6343d6fcfcce1767ae4337381 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 27 Nov 2022 18:37:37 +0100
Subject: [PATCH] New script admin/check-man-pages

* admin/check-man-pages: New file with script used to check man pages
for errors.
* admin/make-tarball.txt: Document when to use the above script.
---
 admin/check-man-pages  | 47 ++++++++++++++++++++++++++++++++++++++++++
 admin/make-tarball.txt |  6 ++++++
 2 files changed, 53 insertions(+)
 create mode 100755 admin/check-man-pages

diff --git a/admin/check-man-pages b/admin/check-man-pages
new file mode 100755
index 0000000000..52cc777205
--- /dev/null
+++ b/admin/check-man-pages
@@ -0,0 +1,47 @@
+#!/bin/bash
+### check-man-pages - check man pages for errors
+
+## Copyright (C) 2022 Free Software Foundation, Inc.
+
+## This file is part of GNU Emacs.
+
+## GNU Emacs is free software: you can redistribute it and/or modify
+## it under the terms of the GNU General Public License as published by
+## the Free Software Foundation, either version 3 of the License, or
+## (at your option) any later version.
+
+## GNU Emacs is distributed in the hope that it will be useful,
+## but WITHOUT ANY WARRANTY; without even the implied warranty of
+## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+## GNU General Public License for more details.
+
+## You should have received a copy of the GNU General Public License
+## along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+### Commentary:
+
+## Check our man pages for errors using `man'.
+
+### Code:
+
+source "${0%/*}/emacs-shell-lib"
+
+exit_status=0
+
+cd "$PD"/../doc/man
+for page in *.1; do
+    log=$(emacs_mktemp)
+    LC_ALL=C.UTF-8 MANROFFSEQ='' MANWIDTH=80 \
+        man --warnings=all,mac -E UTF-8 -l -Tutf8 -Z "$page" >/dev/null 2> "$log"
+    log_size=$(stat --format=%s "$log")
+    if [ "$log_size" -ne 0 ]; then
+        echo "doc/man/$page:"
+        # Point to the correct file for *compilation* buffers.
+        cat "$log" \
+            | sed 's/troff: man1\/\([^ ]\+\)\.1/troff: doc\/man\/\1.1/' \
+            | sed "s/<standard input>/doc\/man\/$page/"
+        exit_status=1
+    fi
+done
+
+exit $exit_status
diff --git a/admin/make-tarball.txt b/admin/make-tarball.txt
index d881b81612..ff6f8845b5 100644
--- a/admin/make-tarball.txt
+++ b/admin/make-tarball.txt
@@ -150,6 +150,12 @@ General steps (for each step, check for possible errors):
 4.    autoreconf -i -I m4 --force
       make bootstrap
 
+      ./admin/check-man-pages
+
+    The above script checks for any mistakes in the source text of
+    manual pages.  Fix any errors, and then re-run the check-man-pages
+    script to verify.  Then do this:
+
       make -C etc/refcards
       make -C etc/refcards clean
 
-- 
2.35.1

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 27 Nov 2022 14:59:34 -0800
> Cc: 59631@debbugs.gnu.org
> 
> I think adding it to make-tarball is a good idea.  Perhaps we could do
> it in the same step as checking the refcards?  I've attached a new patch
> with a suggested wording, please let me know what you think.

SGTM.

> I also enabled a few more groff warnings, and cleaned up the output to
> enable automatically jumping to errors from *compilation* buffers.

Can this be done by running Groff directly instead of via 'man'?  If it's
possible, it's IMO better, since Groff installations are more widespread
than the particular variant of 'man' you used in the script.  E.g., I think
the requirement for C.UTF-8 locale being available can be dropped in that
case.

But if doing so requires too much effort or research, feel free to install
what you have.

Thanks.

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Can this be done by running Groff directly instead of via 'man'?  If it's
> possible, it's IMO better, since Groff installations are more widespread
> than the particular variant of 'man' you used in the script.  E.g., I think
> the requirement for C.UTF-8 locale being available can be dropped in that
> case.

The version of man used is man-db, which AFAIK is used by all major
GNU/Linux distributions.

    https://man-db.gitlab.io/man-db/

Looking into this with PIPELINE_DEBUG=1, the processing done is
non-trivial, and depends on e.g. /usr/libexec/man-db/zsoelim from
man-db.  So I think trying to duplicate all of that for the purposes of
this script might not be the best use of our time.

For reference, I've attached the debug output from man.

[-- Attachment #2: man-output.txt --]
[-- Type: text/plain, Size: 1977 bytes --]

$ PIPELINE_DEBUG=1 LC_ALL=C.UTF-8 MANROFFSEQ='' MANWIDTH=80 \
man --warnings=all,mac -E UTF-8 -l -Tutf8 -Z doc/man/emacs.1 >/dev/null                  
Starting pipeline: (echo .nh && echo .de hy && echo .. && echo .na && echo .de ad && echo .. && cat) [input: {0, doc/man/emacs.1}, output: {-1, NULL}]
Started "decompressor", pid 1636161
Started "echo .nh && echo .de hy && echo ..", pid 1636162
  "echo .nh && echo .de hy && echo .." (1636162) -> 0
Started "echo .na && echo .de ad && echo ..", pid 1636163
  "echo .na && echo .de ad && echo .." (1636163) -> 0
Started "cat", pid 1636164
Starting pipeline: /usr/libexec/man-db/zsoelim | preconv -e UTF-8 | groff -mandoc -Z -wall -wmac -Tutf8  "cat" (1636164) -> 0
 [input: {-1, NULL}, output: {0, NULL}]
Started "/usr/libexec/man-db/zsoelim", pid 1636165
Started "preconv", pid 1636166
Started "groff", pid 1636167
source pipeline 0 returned error or EOF
Waiting for pipeline: (echo .nh && echo .de hy && echo .. && echo .na && echo .de ad && echo .. && cat) [input: {0, doc/man/emacs.1}, output: {-1, NULL}]
Active processes (1):
  "decompressor" (1636161) -> 0
Waiting for pipeline: /usr/libexec/man-db/zsoelim | preconv -e UTF-8 | groff -mandoc -Z -wall -wmac -Tutf8 [input: {-1, NULL}, output: {0, NULL}]
Active processes (3):
  "/usr/libexec/man-db/zsoelim" (1636165) -> -1
  "preconv" (1636166) -> -1
  "groff" (1636167) -> -1
Starting pipeline: zcat [input: {3, NULL}, output: {-1, NULL}]
Started "zcat", pid 1636168
Waiting for pipeline: zcat [input: {3, NULL}, output: {-1, NULL}]
Active processes (1):
  "zcat" (1636168) -> -1
Active processes (1):
  "zcat" (1636168) -> 0
Active processes (3):
  "/usr/libexec/man-db/zsoelim" (1636165) -> 0
  "preconv" (1636166) -> -1
  "groff" (1636167) -> -1
Active processes (2):
  "preconv" (1636166) -> 0
  "groff" (1636167) -> -1
troff: <standard input>:18: warning: macro 'SHX' not defined (possibly missing space after 'SH')
Active processes (1):
  "groff" (1636167) -> 0

[-- Attachment #3: Type: text/plain, Size: 118 bytes --]


> But if doing so requires too much effort or research, feel free to install
> what you have.

I'll do that, thanks.

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Thu, 1 Dec 2022 07:02:16 -0800
> Cc: 59631@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can this be done by running Groff directly instead of via 'man'?  If it's
> > possible, it's IMO better, since Groff installations are more widespread
> > than the particular variant of 'man' you used in the script.  E.g., I think
> > the requirement for C.UTF-8 locale being available can be dropped in that
> > case.
> 
> The version of man used is man-db, which AFAIK is used by all major
> GNU/Linux distributions.

Exactly.  So anyone who doesn't have man-db or isn't on GNU/Linux is toast.
Groff is another matter: there's even a Windows port.

> Looking into this with PIPELINE_DEBUG=1, the processing done is
> non-trivial, and depends on e.g. /usr/libexec/man-db/zsoelim from
> man-db.  So I think trying to duplicate all of that for the purposes of
> this script might not be the best use of our time.

Fine with me, but just so you know: zsoelim is just a variant of soelim
which comes with Groff; it differs from soelim in that it knows about
compressed man pages (which are not the issue here).

AFAICT, the pipeline is simple:

 soelim FILE | preconv -e UTF-8 | groff -mandoc -Z -wall -wmac -Tutf8 > /dev/null

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > The version of man used is man-db, which AFAIK is used by all major
  > > GNU/Linux distributions.

  > Exactly.  So anyone who doesn't have man-db or isn't on GNU/Linux is toast.

Saying a user "is toast" means that the user is dead.  Even if that is
taken as a metaphor, it is a bit exaggeration.

A feature limited to GNU-Like systems is not a horrible thing.  Our
policy is that support for non-GNU-like systems is at best a secondary
goal -- something users can implement if they wish, but we don't
consider the lack of that support as a problem.

It's fine to make this call Groff directly, if it has no major
drawbacks.  But please don't imply that the absence of that support
would be horrible.


-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Cc: stefankangas@gmail.com, 59631@debbugs.gnu.org
> Date: Fri, 02 Dec 2022 17:50:05 -0500
> 
> A feature limited to GNU-Like systems is not a horrible thing.  Our
> policy is that support for non-GNU-like systems is at best a secondary
> goal -- something users can implement if they wish, but we don't
> consider the lack of that support as a problem.
> 
> It's fine to make this call Groff directly, if it has no major
> drawbacks.  But please don't imply that the absence of that support
> would be horrible.

This is not a user-level feature.  This is a feature for Emacs developers,
and only for those of them who routinely work on producing the release
tarballs.  From my POV, it is important to make this kind of features to
work on as many systems as possible, if only to allow me to do these jobs
from my local system.

The feature is implemented in Groff, btw, the man page of 'man' says that
explicitly.

And I didn't say the inability to use Groff directly would be "horrible",
quite the contrary.  Quote:

> Can this be done by running Groff directly instead of via 'man'?  If it's
> possible, it's IMO better, since Groff installations are more widespread
> than the particular variant of 'man' you used in the script.  E.g., I think
> the requirement for C.UTF-8 locale being available can be dropped in that
> case.
> 
> But if doing so requires too much effort or research, feel free to install
> what you have.

Note the last paragraph.

So I consider your reprimand gratuitous and even mildly insulting.  It
sounds like you reacted to a single sentence, taken out of context, and
either didn't read or forgot the other messages I wrote in this thread.

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> AFAICT, the pipeline is simple:
>
>  soelim FILE | preconv -e UTF-8 | groff -mandoc -Z -wall -wmac -Tutf8 > /dev/null

My concern is that the command is more brittle in the face of changes,
and also all the extra "echo" stuff that man-db is doing (probably
because I don't understand what it's doing).

I'm thinking that we could provide the above in case man-db isn't
available.  Is there any way to auto-detect that?  If not, how about
putting this behind a flag?

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Eli Zaretskii]

On December 3, 2022 4:20:50 PM GMT+02:00, Stefan Kangas <stefankangas@gmail.com> wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > AFAICT, the pipeline is simple:
> >
> >  soelim FILE | preconv -e UTF-8 | groff -mandoc -Z -wall -wmac -Tutf8 > /dev/null
> 
> My concern is that the command is more brittle in the face of changes,
> and also all the extra "echo" stuff that man-db is doing (probably
> because I don't understand what it's doing).
> 
> I'm thinking that we could provide the above in case man-db isn't
> available.  Is there any way to auto-detect that?  If not, how about
> putting this behind a flag?
> 

Nah, just install your variant.  It isn't worth the hassle to do anything more complex.  I have the command in my notes and will use it if and when required.

bug#59631: 29.0.50; [PATCH] New check-man target to check man pages for errors

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Nah, just install your variant.  It isn't worth the hassle to do
> anything more complex.  I have the command in my notes and will use it
> if and when required.

Thanks, pushed to emacs-29 (commit d3d9676bf8).  Closing.