[PATCH] INSTALL, Makefile, cmd.mk, lint-man.mk: Lint about '\" t' comment for tbl(1)

[PATCH] INSTALL, Makefile, cmd.mk, lint-man.mk: Lint about '\" t' comment for tbl(1)

[Alejandro Colomar]

This is to make sure that we have correct \" t comments in the pages,
which are necessary for the Debian package checker:

On 8/19/22 22:21, Jakub Wilk wrote:
> * Michael Kerrisk <mtk.manpages@gmail.com>, 2020-07-24 12:13:
>> For 15 years or at least, I've not paid any attention to adding the
>> 't' comments when I added tables to pages, and I do recall anyone
>> reporting ill effects.  So, I'm inclined to apply Mike's patch, but
>> will hold off a moment, in case there's other feedback.
>
> I'm a bit late, but...
>
> Lintian, the Debian package checker, sets the MANROFFSEQ environment
> variable to empty string as a speed optimization.  This turns off
> loading preprocessors that weren't explicitly declared in the source.
> The lack of '\" comments can cause false positives (and maybe also
> false negatives?) in Lintian.
>
> The use of $MANROFFSEQ for Lintian was proposed here:
> https://bugs.debian.org/677874
>
> Beware that the man(1) man page does not correctly explain what
> $MANROFFSEQ does: <https://bugs.debian.org/971009>

Also update the dependencies list, since now we also need head(1) and
tail(1) for linting man(7) source.

Link: <https://lore.kernel.org/linux-man/07a7d4e7-79a6-b2c3-6892-1e39a0679f27@gmail.com/T/#mcf36c8a387fd5ff4f800dc220e3dbdd229b556bd>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Mike Frysinger <vapier@chromium.org>
Cc: "G. Branden Robinson" <g.branden.robinson@gmail.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Reported-by: Jakub Wilk <jwilk@jwilk.net>
Co-developed-by: Jakub Wilk <jwilk@jwilk.net>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---

Hi Jakub,

Since you contributed the initial rough scripts used in this patch, it's
fair to say that you co-developed it.  Would you mind signing off the
patch?

Cheers,

Alex


 INSTALL         |  2 ++
 Makefile        |  1 +
 lib/cmd.mk      |  2 ++
 lib/lint-man.mk | 22 +++++++++++++++++++++-
 4 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/INSTALL b/INSTALL
index bcaadf763..ab513f8c4 100644
--- a/INSTALL
+++ b/INSTALL
@@ -93,7 +93,9 @@ Description
           -  For linting man(7) source:
              -  eqn(1)
              -  grotty(1)
+             -  head(1)
              -  mkdir(1)
+             -  tail(1)
              -  tbl(1)
              -  troff(1) >= 1.23.0 - GNU troff is required.
 
diff --git a/Makefile b/Makefile
index e7be1e291..66f02dea8 100644
--- a/Makefile
+++ b/Makefile
@@ -71,6 +71,7 @@ help:
 	$(info	lint-man		Wrapper for lint-man-* targets)
 	$(info	lint-man-groff		Lint man pages with groff(1))
 	$(info	lint-man-mandoc		Lint man pages with mandoc(1))
+	$(info	lint-man-tbl		Lint man pages about '\" t' comment for tbl(1))
 	$(info	)
 	$(info	[un]install		Alias for "[un]install-man")
 	$(info	[un]install-man		Wrapper for [un]install-man* targets)
diff --git a/lib/cmd.mk b/lib/cmd.mk
index c9c22d0ae..7d6d9abd9 100644
--- a/lib/cmd.mk
+++ b/lib/cmd.mk
@@ -14,12 +14,14 @@ FIND       := find
 GIT        := git
 GREP       := grep
 GZIP       := gzip
+HEAD       := head
 INSTALL    := install
 LOCALE     := locale
 PKG-CONFIG := pkg-config
 SED        := sed
 SORT       := sort
 TAC        := tac
+TAIL       := tail
 TAR        := tar
 XARGS      := xargs
 XZ         := xz
diff --git a/lib/lint-man.mk b/lib/lint-man.mk
index 18b71446b..c86f3addd 100644
--- a/lib/lint-man.mk
+++ b/lib/lint-man.mk
@@ -66,9 +66,10 @@ MANDOC              := mandoc
 
 _LINT_man_groff :=$(patsubst $(MANDIR)/%,$(_LINTDIR)/%.lint-man.groff.touch,$(LINTMAN))
 _LINT_man_mandoc:=$(patsubst $(MANDIR)/%,$(_LINTDIR)/%.lint-man.mandoc.touch,$(LINTMAN))
+_LINT_man_tbl   :=$(patsubst $(MANDIR)/%,$(_LINTDIR)/%.lint-man.tbl.touch,$(LINTMAN))
 
 
-linters_man := groff mandoc
+linters_man := groff mandoc tbl
 lint_man    := $(foreach x,$(linters_man),lint-man-$(x))
 
 
@@ -87,6 +88,25 @@ $(_LINT_man_mandoc): $(_LINTDIR)/%.lint-man.mandoc.touch: $(MANDIR)/% | $$(@D)/.
 	$(MANDOC) $(MANDOCFLAGS) $<
 	touch $@
 
+$(_LINT_man_tbl): $(_LINTDIR)/%.lint-man.tbl.touch: $(MANDIR)/% | $$(@D)/.
+	$(info LINT (tbl)	$@)
+	if $(GREP) '^\.TS$$' <$< >/dev/null && ! $(HEAD) -n1 <$< | $(GREP) '\\" t$$' >/dev/null; then \
+		2>&1 $(ECHO) "$<: 1: missing '\\\" t' comment:"; \
+		2>&1 $(HEAD) -n1 <$<; \
+		exit 1; \
+	fi;
+	if $(HEAD) -n1 <$< | $(GREP) '\\" t$$' >/dev/null && ! $(GREP) '^\.TS$$' <$< >/dev/null; then \
+		2>&1 $(ECHO) "$<: 1: spurious '\\\" t' comment:"; \
+		2>&1 $(HEAD) -n1 <$<; \
+		exit 1; \
+	fi;
+	if $(TAIL) -n+2 <$< | $(GREP) '\\" t$$' >/dev/null ; then \
+		2>&1 $(ECHO) "$<: spurious '\\\" t' not in first line:"; \
+		2>&1 $(GREP) -n '\\" t$$' $< /dev/null; \
+		exit 1; \
+	fi;
+	touch $@
+
 
 .PHONY: $(lint_man)
 $(lint_man): lint-man-%: $$(_LINT_man_%)
-- 
2.38.1

Re: [PATCH] INSTALL, Makefile, cmd.mk, lint-man.mk: Lint about '\" t' comment for tbl(1)

[Alejandro Colomar]

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

Hi Jakub!

On 11/9/22 16:18, Alejandro Colomar wrote:
> This is to make sure that we have correct \" t comments in the pages,
> which are necessary for the Debian package checker:
> 
> On 8/19/22 22:21, Jakub Wilk wrote:
>> * Michael Kerrisk <mtk.manpages@gmail.com>, 2020-07-24 12:13:
>>> For 15 years or at least, I've not paid any attention to adding the
>>> 't' comments when I added tables to pages, and I do recall anyone
>>> reporting ill effects.  So, I'm inclined to apply Mike's patch, but
>>> will hold off a moment, in case there's other feedback.
>>
>> I'm a bit late, but...
>>
>> Lintian, the Debian package checker, sets the MANROFFSEQ environment
>> variable to empty string as a speed optimization.  This turns off
>> loading preprocessors that weren't explicitly declared in the source.
>> The lack of '\" comments can cause false positives (and maybe also
>> false negatives?) in Lintian.
>>
>> The use of $MANROFFSEQ for Lintian was proposed here:
>> https://bugs.debian.org/677874
>>
>> Beware that the man(1) man page does not correctly explain what
>> $MANROFFSEQ does: <https://bugs.debian.org/971009>
> 
> Also update the dependencies list, since now we also need head(1) and
> tail(1) for linting man(7) source.
> 
> Link: <https://lore.kernel.org/linux-man/07a7d4e7-79a6-b2c3-6892-1e39a0679f27@gmail.com/T/#mcf36c8a387fd5ff4f800dc220e3dbdd229b556bd>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Cc: Mike Frysinger <vapier@chromium.org>
> Cc: "G. Branden Robinson" <g.branden.robinson@gmail.com>
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Stefan Puiu <stefan.puiu@gmail.com>
> Reported-by: Jakub Wilk <jwilk@jwilk.net>
> Co-developed-by: Jakub Wilk <jwilk@jwilk.net>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---

I also used your script to add the comment to the pages, and used this linter to 
make sure that it works fine, so I added the following commit:

commit 629a402fe4eee48f31c658b2a7ba6de28990e780 (HEAD -> tbl)
Author: Alejandro Colomar <alx@kernel.org>
Date:   Wed Nov 9 16:36:36 2022 +0100

     Many pages: Add '\" t' comment where necessary

     Scripted change:

     $ grep -l -x '^[.]TS$' man*/* | sort -u | xargs sed -i -e "1i'\\\\\" t"

     Tested-by: Alejandro Colomar <alx@kernel.org>
     Signed-off-by: Alejandro Colomar <alx@kernel.org>


The diff is huge, so I won't send it, but you can expect what it is by just 
running your own script :)

Would you mind also signing that patch?

Cheers,

Alex

> 
> Hi Jakub,
> 
> Since you contributed the initial rough scripts used in this patch, it's
> fair to say that you co-developed it.  Would you mind signing off the
> patch?
> 
> Cheers,
> 
> Alex
> 
> 
>   INSTALL         |  2 ++
>   Makefile        |  1 +
>   lib/cmd.mk      |  2 ++
>   lib/lint-man.mk | 22 +++++++++++++++++++++-
>   4 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/INSTALL b/INSTALL
> index bcaadf763..ab513f8c4 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -93,7 +93,9 @@ Description
>             -  For linting man(7) source:
>                -  eqn(1)
>                -  grotty(1)
> +             -  head(1)
>                -  mkdir(1)
> +             -  tail(1)
>                -  tbl(1)
>                -  troff(1) >= 1.23.0 - GNU troff is required.
>   
> diff --git a/Makefile b/Makefile
> index e7be1e291..66f02dea8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -71,6 +71,7 @@ help:
>   	$(info	lint-man		Wrapper for lint-man-* targets)
>   	$(info	lint-man-groff		Lint man pages with groff(1))
>   	$(info	lint-man-mandoc		Lint man pages with mandoc(1))
> +	$(info	lint-man-tbl		Lint man pages about '\" t' comment for tbl(1))
>   	$(info	)
>   	$(info	[un]install		Alias for "[un]install-man")
>   	$(info	[un]install-man		Wrapper for [un]install-man* targets)
> diff --git a/lib/cmd.mk b/lib/cmd.mk
> index c9c22d0ae..7d6d9abd9 100644
> --- a/lib/cmd.mk
> +++ b/lib/cmd.mk
> @@ -14,12 +14,14 @@ FIND       := find
>   GIT        := git
>   GREP       := grep
>   GZIP       := gzip
> +HEAD       := head
>   INSTALL    := install
>   LOCALE     := locale
>   PKG-CONFIG := pkg-config
>   SED        := sed
>   SORT       := sort
>   TAC        := tac
> +TAIL       := tail
>   TAR        := tar
>   XARGS      := xargs
>   XZ         := xz
> diff --git a/lib/lint-man.mk b/lib/lint-man.mk
> index 18b71446b..c86f3addd 100644
> --- a/lib/lint-man.mk
> +++ b/lib/lint-man.mk
> @@ -66,9 +66,10 @@ MANDOC              := mandoc
>   
>   _LINT_man_groff :=$(patsubst $(MANDIR)/%,$(_LINTDIR)/%.lint-man.groff.touch,$(LINTMAN))
>   _LINT_man_mandoc:=$(patsubst $(MANDIR)/%,$(_LINTDIR)/%.lint-man.mandoc.touch,$(LINTMAN))
> +_LINT_man_tbl   :=$(patsubst $(MANDIR)/%,$(_LINTDIR)/%.lint-man.tbl.touch,$(LINTMAN))
>   
>   
> -linters_man := groff mandoc
> +linters_man := groff mandoc tbl
>   lint_man    := $(foreach x,$(linters_man),lint-man-$(x))
>   
>   
> @@ -87,6 +88,25 @@ $(_LINT_man_mandoc): $(_LINTDIR)/%.lint-man.mandoc.touch: $(MANDIR)/% | $$(@D)/.
>   	$(MANDOC) $(MANDOCFLAGS) $<
>   	touch $@
>   
> +$(_LINT_man_tbl): $(_LINTDIR)/%.lint-man.tbl.touch: $(MANDIR)/% | $$(@D)/.
> +	$(info LINT (tbl)	$@)
> +	if $(GREP) '^\.TS$$' <$< >/dev/null && ! $(HEAD) -n1 <$< | $(GREP) '\\" t$$' >/dev/null; then \
> +		2>&1 $(ECHO) "$<: 1: missing '\\\" t' comment:"; \
> +		2>&1 $(HEAD) -n1 <$<; \
> +		exit 1; \
> +	fi;
> +	if $(HEAD) -n1 <$< | $(GREP) '\\" t$$' >/dev/null && ! $(GREP) '^\.TS$$' <$< >/dev/null; then \
> +		2>&1 $(ECHO) "$<: 1: spurious '\\\" t' comment:"; \
> +		2>&1 $(HEAD) -n1 <$<; \
> +		exit 1; \
> +	fi;
> +	if $(TAIL) -n+2 <$< | $(GREP) '\\" t$$' >/dev/null ; then \
> +		2>&1 $(ECHO) "$<: spurious '\\\" t' not in first line:"; \
> +		2>&1 $(GREP) -n '\\" t$$' $< /dev/null; \
> +		exit 1; \
> +	fi;
> +	touch $@
> +
>   
>   .PHONY: $(lint_man)
>   $(lint_man): lint-man-%: $$(_LINT_man_%)

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

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

Re: [PATCH] INSTALL, Makefile, cmd.mk, lint-man.mk: Lint about '\" t' comment for tbl(1)

[Alejandro Colomar]

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

On 11/9/22 16:40, Alejandro Colomar wrote:

> The diff is huge, so I won't send it, but you can expect what it is by just 
> running your own script :)

I can show you this, though:

  592 files changed, 592 insertions(+)


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

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

Re: [PATCH] INSTALL, Makefile, cmd.mk, lint-man.mk: Lint about '\" t' comment for tbl(1)

[Alejandro Colomar]

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



On 11/9/22 16:42, Alejandro Colomar wrote:
> On 11/9/22 16:40, Alejandro Colomar wrote:
> 
>> The diff is huge, so I won't send it, but you can expect what it is by just 
>> running your own script :)
> 
> I can show you this, though:
> 
>   592 files changed, 592 insertions(+)

And the first couple of diffs is:

diff --git a/man2/adjtimex.2 b/man2/adjtimex.2
index c106928e9..51ba650a8 100644
--- a/man2/adjtimex.2
+++ b/man2/adjtimex.2
@@ -1,3 +1,4 @@
+'\" t
  .\" Copyright (c) 1995 Michael Chastain (mec@shell.portal.com), 15 April 1995.
  .\" and Copyright (C) 2014, 2016 Michael Kerrisk <mtk.manpages@gmail.com>
  .\"
diff --git a/man2/clock_getres.2 b/man2/clock_getres.2
index 228d21467..91a57bc36 100644
--- a/man2/clock_getres.2
+++ b/man2/clock_getres.2
@@ -1,3 +1,4 @@
+'\" t
  .\" Copyright (c) 2003 Nick Clifford (zaf@nrc.co.nz), Jan 25, 2003
  .\" Copyright (c) 2003 Andries Brouwer (aeb@cwi.nl), Aug 24, 2003
  .\" Copyright (c) 2020 Michael Kerrisk <mtk.manpages@gmail.com>

> 
> 

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

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

Re: [PATCH] INSTALL, Makefile, cmd.mk, lint-man.mk: Lint about '\" t' comment for tbl(1)

[Mike Frysinger]

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

On 09 Nov 2022 16:18, Alejandro Colomar wrote:
> --- a/lib/lint-man.mk
> +++ b/lib/lint-man.mk

i guess not a new issue, but it feels like writing lint logic in Makefiles
is not the best use of time.  this logic is really hairy.

> +$(_LINT_man_tbl): $(_LINTDIR)/%.lint-man.tbl.touch: $(MANDIR)/% | $$(@D)/.
> +	$(info LINT (tbl)	$@)
> +	if $(GREP) '^\.TS$$' <$< >/dev/null && ! $(HEAD) -n1 <$< | $(GREP) '\\" t$$' >/dev/null; then \

POSIX grep has a -q option so you don't have to redirect to /dev/null.
	if $(GREP) -q '^\.TS$$' <$< && ...

also, is there a reason you're using a redirect instead of just passing the
file to grep ?  the redirect works, but it seems to contribute to general
"this code is hard for humans to read".  i don't think you really need to
be concerned with files starting with dashes ...
	if $(GREP) -q '^\.TS$$' $< && ...

or more completely:
	if $(GREP) -q '^\.TS$$' $< && ! $(HEAD) -n1 $< | $(GREP) -q '\\" t$$'; then

> +	fi;

don't need this trailing semi-colon
-mike

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

Re: [PATCH] INSTALL, Makefile, cmd.mk, lint-man.mk: Lint about '\" t' comment for tbl(1)

[Alejandro Colomar]

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

Hi Mike!

On 11/9/22 16:58, Mike Frysinger wrote:
> On 09 Nov 2022 16:18, Alejandro Colomar wrote:
>> --- a/lib/lint-man.mk
>> +++ b/lib/lint-man.mk
> 
> i guess not a new issue, but it feels like writing lint logic in Makefiles
> is not the best use of time.  this logic is really hairy.

Hmm, well, the line is not so clear.  But I'd say this is the second case where 
I add lint logic hardcoded in the makefile.  The first case was for checking 
that rendered output fits in a terminal (80 col):

<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/lib/lint-man.mk#n82>

All other lints are just running the linter, which in some cases is just a 
single command, and in others is a pipeline.

But this one is the most clear example of lint logic in the Makefiles, yes.


Option B would be to write those 3 conditionals in a script under <./scripts/>, 
and call it from the Makefile.  Since I don't expect this (hardcoding lint logic 
in the Makefile) to happen very often, I'm not tempted enough to do that.  For 
now, a few lines of embedded code seem reasonable to me.

But please feel free to propose a different approach.

> 
>> +$(_LINT_man_tbl): $(_LINTDIR)/%.lint-man.tbl.touch: $(MANDIR)/% | $$(@D)/.
>> +	$(info LINT (tbl)	$@)
>> +	if $(GREP) '^\.TS$$' <$< >/dev/null && ! $(HEAD) -n1 <$< | $(GREP) '\\" t$$' >/dev/null; then \
> 
> POSIX grep has a -q option so you don't have to redirect to /dev/null.
> 	if $(GREP) -q '^\.TS$$' <$< && ...

Hmm, I'm usually not a fan of it (I was in the past, but I've learnt to prefer 
pipes and redirects).  But in this case it helps keep the lines shorter, so yes, 
I like it for this code.

> 
> also, is there a reason you're using a redirect instead of just passing the
> file to grep ?  the redirect works, but it seems to contribute to general
> "this code is hard for humans to read".  i don't think you really need to
> be concerned with files starting with dashes ...
> 	if $(GREP) -q '^\.TS$$' $< && ...
> 
> or more completely:
> 	if $(GREP) -q '^\.TS$$' $< && ! $(HEAD) -n1 $< | $(GREP) -q '\\" t$$'; then

I have grown a tendency to use pipes and redirection instead of filenames and 
options.  Especially when writing my own filters.

When I write long scripts where each command goes on a line of its own, I prefer 
that very much.  It helps readability IMO.  As an example, <./scripts/bash_aliases>.

However, for 'if's, it's much more clear when the condition is a one-liner, and 
that changes some things.  Having the line under 80 col helps in readability a 
lot more than redirections.  And having fewer special characters also helps 
parse the complex line.  So, yes, I agree that in this case it's better to make 
it shorter as you propose.  I'll send an updated v2 soon.

> 
>> +	fi;
> 
> don't need this trailing semi-colon

In bash scripts, I tend to always write semicolons.  I like their preciseness. 
In Makefiles, however, I learnt recently that GNU make makes a difference when 
you use them: it can't optimize certain things.  So it's actually better to 
remove them.

I just put them out of inertia from the rest of the embedded script, and agree 
that it's better without them.

> -mike

Thanks!

Cheers,

Alex

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

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