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

PATCH Superseded Alejandro Colomar 2 participants 6 messages Nov 9, 2022

Subsystems (3)

Kbuild Library code Man-pages: manual pages for Linux -- Sections 2, 3, 4, 5, and 7

lore

Share & tools

lore

Alejandro Colomar

author

2022-11-09 15:20 UTC #0 lore raw

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

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

Quoted 14 lines · Jakub Wilk

* Michael Kerrisk <[email protected]>, 2020-07-24 12:13:
Quoted 4 lines
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.

Quoted 5 lines

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/[email protected]/T/#mcf36c8a387fd5ff4f800dc220e3dbdd229b556bd> open here

Cc Mike Frysinger <[email protected]>

Cc "G. Branden Robinson" <[email protected]>

Cc Michael Kerrisk <[email protected]>

Cc Stefan Puiu <[email protected]>

Reported-by Jakub Wilk <[email protected]>

Co-developed-by Jakub Wilk <[email protected]>

Signed-off-by Alejandro Colomar <[email protected]>

---

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

INSTALL +2 -0

index bcaadf763..ab513f8c4 100644
@@ -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.

Makefile +1 -0

index e7be1e291..66f02dea8 100644
@@ -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)

lib/cmd.mk +2 -0

index c9c22d0ae..7d6d9abd9 100644
@@ -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

lib/lint-man.mk +21 -1

index 18b71446b..c86f3addd 100644
@@ -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_%)

Signature · 1 line

2.38.1

Alejandro Colomar

author Tested-by on v1 1/1

2022-11-09 15:40 UTC #1 lore raw

Hi Jakub!

Quoted 36 lines · Alejandro Colomar

This is to make sure that we have correct \" t comments in the pages, which are necessary for the Debian package checker:
Quoted 14 lines · Jakub Wilk
* Michael Kerrisk <[email protected]>, 2020-07-24 12:13:
Quoted 4 lines
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.
Quoted 5 lines
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/[email protected]/T/#mcf36c8a387fd5ff4f800dc220e3dbdd229b556bd> open here
Cc Mike Frysinger <[email protected]>
Cc Mike Frysinger <[email protected]>
Cc "G. Branden Robinson" <[email protected]>
Cc Michael Kerrisk <[email protected]>
Cc Stefan Puiu <[email protected]>
Reported-by Jakub Wilk <[email protected]>
Co-developed-by Jakub Wilk <[email protected]>
Signed-off-by Alejandro Colomar <[email protected]>

Quoted 1 line

---

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 <[email protected]>
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 <[email protected]>

Signed-off-by Alejandro Colomar <[email protected]>

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

Quoted 105 lines

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

INSTALL +2 -0

index bcaadf763..ab513f8c4 100644
@@ -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.

Makefile +1 -0

index e7be1e291..66f02dea8 100644
@@ -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)

lib/cmd.mk +2 -0

index c9c22d0ae..7d6d9abd9 100644
@@ -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

lib/lint-man.mk +21 -1

index 18b71446b..c86f3addd 100644
@@ -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_%)

Signature · 1 line

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

Alejandro Colomar

author

2022-11-09 15:42 UTC #2 lore raw

Quoted 1 line · Alejandro Colomar

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

Signature · 1 line

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

Alejandro Colomar

author

2022-11-09 15:53 UTC #3 lore raw

Quoted 7 lines · Alejandro Colomar

Quoted 1 line · Alejandro Colomar
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:

man2/adjtimex.2 +1 -0

index c106928e9..51ba650a8 100644
@@ -1,3 +1,4 @@
+'\" t
 .\" Copyright (c) 1995 Michael Chastain ([email protected]), 15 April 1995.
 .\" and Copyright (C) 2014, 2016 Michael Kerrisk <[email protected]>
 .\"

man2/clock_getres.2 +1 -0

index 228d21467..91a57bc36 100644
@@ -1,3 +1,4 @@
+'\" t
 .\" Copyright (c) 2003 Nick Clifford ([email protected]), Jan 25, 2003
 .\" Copyright (c) 2003 Andries Brouwer ([email protected]), Aug 24, 2003
 .\" Copyright (c) 2020 Michael Kerrisk <[email protected]>

Quoted 2 lines

Signature · 1 line

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

Mike Frysinger

2022-11-09 15:58 UTC #4 lore raw

In reply to Alejandro Colomar

lib/lint-man.mk

Quoted 2 lines · Alejandro Colomar

lib/lint-man.mk +0 -0

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.

lib/lint-man.mk $(_LINT_man_mandoc): $(_LINTDIR)/%.lint-man.mandoc.touch: $(MANDIR)/% | $$(@D)/. · line 93

Quoted 3 lines

+$(_LINT_man_tbl): $(_LINTDIR)/%.lint-man.tbl.touch: $(MANDIR)/% | $$(@D)/.
lib/lint-man.mk +2 -0
+	$(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

lib/lint-man.mk

Quoted 1 line

lib/lint-man.mk +1 -0
+	fi;

don't need this trailing semi-colon
-mike

Alejandro Colomar

author

2022-11-09 17:06 UTC #5 lore raw

Hi Mike!

Quoted 6 lines · Mike Frysinger

lib/lint-man.mk
Quoted 2 lines · Alejandro Colomar
lib/lint-man.mk +0 -0
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.

Quoted 7 lines

lib/lint-man.mk $(_LINT_man_mandoc): $(_LINTDIR)/%.lint-man.mandoc.touch: $(MANDIR)/% | $$(@D)/. · line 93
Quoted 4 lines
+$(_LINT_man_tbl): $(_LINTDIR)/%.lint-man.tbl.touch: $(MANDIR)/% | $$(@D)/.
lib/lint-man.mk +2 -0
+	$(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.

Quoted 9 lines

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.

Quoted 4 lines

lib/lint-man.mk
Quoted 2 lines
lib/lint-man.mk +1 -0
+	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.

Quoted 1 line

-mike

Thanks!

Cheers,

Alex

Signature · 1 line

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