bugGNU roff - Bugs: bug #63133, [PATCH] [pdfroff] throws warning...

 
 

bug #63133: [PATCH] [pdfroff] throws warning on any document if -ww given

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Thu 29 Sep 2022 12:22:27 PM UTC
   
 
Category:  General Severity:  3 - Normal
Item Group:  Warning/Suspicious behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.24.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 10 Jul 2023 09:04:07 AM UTC, comment #13: 


commit 7efb987ef7d10147a6b991009e242e31d7956d83
Author: Keith Marshall <keith.d.marshall@ntlworld.com>
Date:   Sat Feb 25 00:30:47 2023 +0000

    [pdfmark]: Don't emit redundant pdfhref Z records.

    * contrib/pdfmark/pdfroff.sh [grohtml-info] (pdfhref Z): Adapt awk
      script, to emit...

      (pdfhref Z 0 0 0): ...this conditionally, only if at least one prior
      record has been emitted; delete unconditional emission, which caused
      the anomaly reported as groff issue #63133.

    Fixes <https://savannah.gnu.org/bugs/?63133>.

    {merged from OSDN hg patch by GBR, 2023-04-07}


G. Branden Robinson <gbranden>
Group administrator
Sat 08 Apr 2023 12:06:02 AM UTC, comment #12: 


commit 510694f995a8b7bba754da3586a22787bf9856f4
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Apr 7 19:03:04 2023 -0500

    [pdfmark]: Don't emit redundant pdfhref Z records.

    [pdfmark]: Do not emit redundant 'pdfhref Z' records.

    * pdfroff.sh [grohtml-info] (pdfhref Z): Adapt awk script, to emit...

      (pdfhref Z 0 0 0): ...this conditionally, only if at least one prior
      record has been emitted; delete unconditional emission, which caused
      the anomaly reported as groff issue #63133.

    Fixes <https://savannah.gnu.org/bugs/?63133>.

    {merged from OSDN hg patch by GBR, 2023-04-07}


G. Branden Robinson <gbranden>
Group administrator
Fri 07 Apr 2023 10:36:53 PM UTC, comment #11: 

Attaching patch from Keith's hg repo.

(file #54582)

G. Branden Robinson <gbranden>
Group administrator
Thu 06 Apr 2023 10:47:40 AM UTC, comment #10: 

comment #9:

> comment #8:
> > The underlying issue, with pdfroff, was fixed by
> > commit 5d88cef1407d5bb625a1c23a888976f6efa80fab:


> > 2023-02-24  Keith Marshall  <address@hidden>
> >
> >     Do not emit redundant 'pdfhref Z' records.
> >
> >     * pdfroff.sh [grohtml-info] (pdfhref Z): Adapt awk script, to emit...
> >     (pdfhref Z 0 0 0): ...this conditionally, only if at least one prior
> >     record has been emitted; delete unconditional emission, which caused
> >     the anomaly reported as groff issue #63133.


> >
> > Okay to close?
>
> Hi Keith,
>
> I'd prefer not to close it here since it still affects groff's
> pdfroff and therefore likely the 1.23.0 release.
>
> Unless we cherry-pick the fix...


Okay.  You might consider just copying my latest pdfroff.sh, accompanying pdfroff.1.man, and everything in my tmac/ subtree, except s.tmac, (which is just a verbatim copy of yours, at the time I forked groff-pdfmark, and yours has evolved since then), but I'll leave it to your discretion.

> BTW, are you sure about that commit link?


Oops! It looks like I copied, and pasted the wrong link; it should have been commit #69c99bee; (I should have noticed that the hash wasn't as I expected).

> It seems to go to this change:


> Clean up Z-shell initialization logic.
>
> * pdfroff.sh [ZSH_VERSION]: Tidy initialization code for...
> (NULLCMD, emulate sh): ...these; bring it more into alignment with
> contemporary GNU autoconf usage.


Indeed.  That's the immediate parent of the intended commit.

> In any case I'll take responsibility for it, on the assumption I'll
> be able to cherry-pick, and mark it "Confirmed" for want of a status
> that means "fix available elsewhere".


Okay; I'll leave it with you.

Keith Marshall <keithmarshall>
Group Member
Wed 05 Apr 2023 11:41:00 PM UTC, comment #9: 

comment #8:

> The underlying issue, with pdfroff, was fixed by commit 5d88cef1407d5bb625a1c23a888976f6efa80fab:


> 2023-02-24  Keith Marshall  <address@hidden>
>
>     Do not emit redundant 'pdfhref Z' records.
>
>     * pdfroff.sh [grohtml-info] (pdfhref Z): Adapt awk script, to emit...
>     (pdfhref Z 0 0 0): ...this conditionally, only if at least one prior
>     record has been emitted; delete unconditional emission, which caused
>     the anomaly reported as groff issue #63133.


>
> Okay to close?


Hi Keith,

I'd prefer not to close it here since it still affects groff's pdfroff and therefore likely the 1.23.0 release.

Unless we cherry-pick the fix...

BTW, are you sure about that commit link?  It seems to go to this change:


Clean up Z-shell initialization logic.

* pdfroff.sh [ZSH_VERSION]: Tidy initialization code for...
(NULLCMD, emulate sh): ...these; bring it more into alignment with
contemporary GNU autoconf usage.


In any case I'll take responsibility for it, on the assumption I'll be able to cherry-pick, and mark it "Confirmed" for want of a status that means "fix available elsewhere".

G. Branden Robinson <gbranden>
Group administrator
Wed 05 Apr 2023 11:23:29 PM UTC, comment #8: 

The underlying issue, with pdfroff, was fixed by commit 5d88cef1407d5bb625a1c23a888976f6efa80fab:

2023-02-24  Keith Marshall  <address@hidden>

    Do not emit redundant 'pdfhref Z' records.

    * pdfroff.sh [grohtml-info] (pdfhref Z): Adapt awk script, to emit...
    (pdfhref Z 0 0 0): ...this conditionally, only if at least one prior
    record has been emitted; delete unconditional emission, which caused
    the anomaly reported as groff issue #63133.


Okay to close?

Keith Marshall <keithmarshall>
Group Member
Thu 23 Feb 2023 04:11:51 PM UTC, comment #7: 

Not a resolver, just an updater.


commit 948ccc55e6feb4f6db235488e485eb4386dc9b6a
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Tue Feb 21 14:23:02 2023 -0600

    pdfroff(1): Note new upstream development site.

    See <https://savannah.gnu.org/bugs/?63133>.


G. Branden Robinson <gbranden>
Group administrator
Thu 23 Feb 2023 11:14:07 AM UTC, comment #6: 

comment #5:

> Bug #63827 is now open to track the removal of pdfmark from the
> groff git repository.  The following is more relevant to that bug,

And I will follow up there.

> but I'm starting it here because it's a reply to a comment here,
> and Keith is already cc:ed here.

Indeed, but I'm also subscribed to bug #63827.

Keith Marshall <keithmarshall>
Group Member
Wed 22 Feb 2023 06:30:01 AM UTC, comment #5: 

Bug #63827 is now open to track the removal of pdfmark from the groff git repository.  The following is more relevant to that bug, but I'm starting it here because it's a reply to a comment here, and Keith is already cc:ed here.

comment #3:

> Agreed.  I have, indeed, added significant new content to
> pdfmark.pdf, (via pdfmark.ms), and some pdfroff enhancements,
> which are not now included in groff's contrib/pdfmark tree.


There have also been 12 commits that have touched groff's contrib/pdfmark tree since the 2021-11-13 creation of the OSDN pdfmark, according to:

git log --since 2021-11-13 contrib/pdfmark

I presume these haven't been reflected in OSDN.  Many of them are part of larger changes to the groff code base, and may or may not be necessary to keep the two interoperating happily, but someone familiar with the code will have to make that call for each change.

(This presumes that the OSDN repository was created from the latest groff git contrib/pdfmark at the time; if it was spawned from another source, there might be more divergence.)

Dave <barx>
Group Member
Tue 21 Feb 2023 06:10:13 PM UTC, comment #4: 

comment #3:

> comment #2:
> > Long story short, the `.C` register didn't work for some of the use cases people had for it, including the one seen above.
> Yep.  The interaction between compatibility mode and '.C' is kind of tricky ... when compatibility mode is on, it needs to be interpolated using the '\n(.C' syntax, because the '\n[.C]' syntax isn't "compatible":


> $ printf '.cp 0\n.tm .C = \\n[.C]\n' | groff -ww > /dev/null
> .C = 0
> $ printf '.cp 1\n.tm .C = \\n[.C]\n' | groff -ww > /dev/null
> .C = troff: <standard input>:2: warning: number register '[' not defined
> 0.C]
> $ printf '.cp 1\n.tm .C = \\n(.C\n' | groff -ww > /dev/null
> .C = 1


> Interaction with the '.do' request compounds the trickiness:


> $ printf '.cp 1\n.do tm .C = \\n[.C]\n' | groff -ww > /dev/null
> .C = 0
> $ printf '.cp 1\n.do tm .C = \\n(.C\n' | groff -ww > /dev/null
> .C = 0


> It appears that the '.do' takes effect before the remainder of the request line, (and in particular, the '\n(.C' evaluation), is parsed.  Thus, within the scope of '.do', '\n(.C' must always evaluate to zero.  This isn't made particularly clear, in the relevant groff documentation.


I have tried to clarify it for groff 1.23.

https://git.savannah.gnu.org/cgit/groff.git/tree/man/groff_diff.7.man?h=1.23.0.rc3#n5079

https://git.savannah.gnu.org/cgit/groff.git/tree/man/groff.7.man?h=1.23.0.rc3#n5510

https://git.savannah.gnu.org/cgit/groff.git/tree/man/groff.7.man?h=1.23.0.rc3#n5541

> > This went unnoticed for a long time, I think, because relatively few people tried to render many groff man pages in series using a single invocation of the formatter.
> Or ... perhaps more likely ... few people use groff directly, to render manpages, and even those who may have done so didn't think to add the '-ww' option?


The reason I noticed it is because when batch-rendering man pages, some got formatted incorrectly because the incorrect saved value of compatibility mode was used.  That would generally be much more noticeable than a warning which is off by default and even if enabled by the user, is typically discarded by man(1), as you note.

[...]

> > In bug #63590, you expressed disinterest in maintaining pdfmark in the groff source tree.
> I no longer feel comfortable committing directly to the groff repo, following the debacle when you deleted a published branch ...


For which I apologized, but if you object to published branches being deleted even with notice, then, yes, it seems your development preferences and the groff project's have diverged and will do so again.  (After about a year, I deleted the "tmp-mail-fail" branch that I created to work around groff-commit reflector problems.)

> > Would you prefer to make your OSDN the official pdfmark resource?
> so yes, I would prefer to keep control of my own development tree.
> > I don't think it would do anyone any good for contrib/pdfmark to bit-rot in groff's contrib.
> Agreed.  I have, indeed, added significant new content to pdfmark.pdf, (via pdfmark.ms), and some pdfroff enhancements, which are not now included in groff's contrib/pdfmark tree.


Okay.  I will update groff's copy of pdfroff(1) shortly to point to your site, advising users of the move.  I will also withdraw the contrib/pdfmark directory itself in the next development cycle--I feel it is too disruptive a change at present (on our 3rd release candidate for 1.23.0.)  (The man page update can go in before final, of course.)

Thank you for your contributions.

G. Branden Robinson <gbranden>
Group administrator
Tue 21 Feb 2023 12:06:49 PM UTC, comment #3: 

comment #2:

> Long story short, the `.C` register didn't work for some of the use cases people had for it, including the one seen above.

Yep.  The interaction between compatibility mode and '.C' is kind of tricky ... when compatibility mode is on, it needs to be interpolated using the '\n(.C' syntax, because the '\n[.C]' syntax isn't "compatible":

$ printf '.cp 0\n.tm .C = \\n[.C]\n' | groff -ww > /dev/null
.C = 0
$ printf '.cp 1\n.tm .C = \\n[.C]\n' | groff -ww > /dev/null
.C = troff: <standard input>:2: warning: number register '[' not defined
0.C]
$ printf '.cp 1\n.tm .C = \\n(.C\n' | groff -ww > /dev/null
.C = 1

Interaction with the '.do' request compounds the trickiness:

$ printf '.cp 1\n.do tm .C = \\n[.C]\n' | groff -ww > /dev/null
.C = 0
$ printf '.cp 1\n.do tm .C = \\n(.C\n' | groff -ww > /dev/null
.C = 0

It appears that the '.do' takes effect before the remainder of the request line, (and in particular, the '\n(.C' evaluation), is parsed.  Thus, within the scope of '.do', '\n(.C' must always evaluate to zero.  This isn't made particularly clear, in the relevant groff documentation.

> This went unnoticed for a long time, I think, because relatively few people tried to render many groff man pages in series using a single invocation of the formatter.

Or ... perhaps more likely ... few people use groff directly, to render manpages, and even those who may have done so didn't think to add the '-ww' option?

> > Please note that this wider issue is not even related to pdfroff:


> > $ groff -ww -man src/roff/groff/groff.1 > /dev/null
> > troff: src/roff/groff/groff.1:25: warning: number register '.cp' not defined


> > Does this merit its own bug report?
>
> I don't think so.  It was a deliberate feature change.  I infer that you're running the above commands using groff 1.22.4

Yes; it's what is installed by default, by the Manjaro derivative of Arch Linux, (which is my current distro of choice).

> (else you wouldn't get the diagnostic message, which also hasn't been worded that way since before commit 31e8ff9daf on 30 June 2021.

Fair enough.  I noticed it when formatting up-to-date pdfroff.1.man with similarly up-to-date pdfroff, and with '-ww' in effect, per this ticket, but underpinned by the system-native groff installation.

> On another subject:
>
> > in my own groff-pdfmark development fork.
>
> In bug #63590, you expressed disinterest in maintaining pdfmark in the groff source tree.

I no longer feel comfortable committing directly to the groff repo, following the debacle when you deleted a published branch ...

> Would you prefer to make your OSDN the official pdfmark resource?

so yes, I would prefer to keep control of my own development tree.

> I don't think it would do anyone any good for contrib/pdfmark to bit-rot in groff's contrib.

Agreed.  I have, indeed, added significant new content to pdfmark.pdf, (via pdfmark.ms), and some pdfroff enhancements, which are not now included in groff's contrib/pdfmark tree.

Keith Marshall <keithmarshall>
Group Member
Tue 21 Feb 2023 12:01:12 AM UTC, comment #2: 

Hi Keith,

comment #1:

> Obviously, since your test case doesn't load any PDF related macro sets, pdfhref is not going to be defined.


It's not quite obvious to me; I think macros that bind tightly to output driver features should probably be declared in a macro file corresponding to the output device.

That is what troffrc has expected for a long time.

https://git.savannah.gnu.org/cgit/groff.git/tree/tmac/troffrc?h=1.22.3.real#n32

I'll grant that the absence of a 'pdf' out device for a long time in groff made such a coupling less straightforward.  For a workflow which uses groff to produce another output format (PostScript) and only converts to PDF after groff has finished with a document, I think your requirement of some pdfhref-providing macro file is more easily motivated.
 

> I do appreciate the minimalism of your test case; with the addition of the '--keep-temporary-files' option, it becomes trivially easy to identify the source of the spurious 'pdfhref' reference.  However, since it runs pdfroff, with no attempt to incorporate any PDF features, does it really provide a compelling incentive to pursue the issue?  I hardly think so, but I can find such an incentive in the '%.pdf:%.man' Makefile.in rule, in my own groff-pdfmark development fork.  Specifically, (if I edit the generated Makefile, to remove the '--no-reference-dictionary' and '--no-toc-relocation' options, the former of which prevents the issue from arising, but then requires the latter to avoid duplication of output), when I invoke this, I see:


> $ make -C wip/build/ pdfroff.1.pdf MAN2PDF_FLAGS=-ww
> make: Entering directory '/home/keith/projects/groff-pdfmark/wip/build'
> /usr/bin/sed -e 's!@VERSION@!1.23.0!' -e 's!@MDATE@!20 February 2023!' -e 's!@PDFDOCDIR@!/usr/local/share/doc/!' -e 's!@MAN\([1-9]\)EXT@!\1!g' ../../pdfroff.1.man | GROFF_TMAC_PATH=../../tmac /bin/sh ./pdfroff -man -ww > pdfroff.1.pdf
> troff: ./pdfroff-jfeZdKTQvb/pdf19949.cmp:1: warning: macro 'pdfhref' not defined
> troff: <standard input>:30: warning: number register '.cp' not defined
> make: Leaving directory '/home/keith/projects/groff-pdfmark/wip/build'


> This does, indeed, reproduce the issue which you report, (and I do have a practical solution, without requiring the '--no-reference-dictionary' option).


Cool!

>  However, it also reveals a more insidious issue, (viz. the reference to undefined '.cp' number register), which appears to be endemic among the entire corpus of manpage sources, throughout the groff code base; it appears to have been introduced by:


> $ hg annotate src/roff/groff/groff.1.man
> ...
> 3963: .\" Save and disable compatibility mode (for, e.g., Solaris 10/11).
> 3963: .do nr *groff_groff_1_man_C \n[.cp]
> 3963: .cp 0
> ...
>
> $ hg log -r 3963
> changeset:   3963:01c8e28fa4bd
> user:        G. Branden Robinson <g.branden.robinson@gmail.com>
> date:        Sun Oct 18 22:56:32 2020 +1100
> summary:     man pages: Make preambles consistent.


Yes, that relates to one of the items in the NEWS file.


o A new read-only register `.cp` is implemented.  Within a `do` request,
  "\n[.cp]" holds the saved value of compatibility mode.  See
  groff_diff(7) or the groff Texinfo manual for rationale, use case, and
  example.


Long story short, the `.C` register didn't work for some of the use cases people had for it, including the one seen above.  This went unnoticed for a long time, I think, because relatively few people tried to render many groff man pages in series using a single invocation of the formatter.

> Please note that this wider issue is not even related to pdfroff:


> $ groff -ww -man src/roff/groff/groff.1 > /dev/null
> troff: src/roff/groff/groff.1:25: warning: number register '.cp' not defined


> Does this merit its own bug report?


I don't think so.  It was a deliberate feature change.  I infer that you're running the above commands using groff 1.22.4 (else you wouldn't get the diagnostic message, which also hasn't been worded that way since before commit 31e8ff9daf on 30 June 2021.

On another subject:

> in my own groff-pdfmark development fork.


In bug #63590, you expressed disinterest in maintaining pdfmark in the groff source tree.

Would you prefer to make your OSDN the official pdfmark resource?  I don't think it would do anyone any good for contrib/pdfmark to bit-rot in groff's contrib.

Regards,
Branden

G. Branden Robinson <gbranden>
Group administrator
Mon 20 Feb 2023 11:14:56 PM UTC, comment #1: 

original submission:

> Affects groff 1.22.4.
>


> $ echo | pdfroff -ww >/dev/null
> troff: /tmp/branden/pdfroff-ehedRan1l8/pdf41844.cmp:1: warning: macro 'pdfhref' not defined


Obviously, since your test case doesn't load any PDF related macro sets, pdfhref is not going to be defined.

I do appreciate the minimalism of your test case; with the addition of the '--keep-temporary-files' option, it becomes trivially easy to identify the source of the spurious 'pdfhref' reference.  However, since it runs pdfroff, with no attempt to incorporate any PDF features, does it really provide a compelling incentive to pursue the issue?  I hardly think so, but I can find such an incentive in the '%.pdf:%.man' Makefile.in rule, in my own groff-pdfmark development fork.  Specifically, (if I edit the generated Makefile, to remove the '--no-reference-dictionary' and '--no-toc-relocation' options, the former of which prevents the issue from arising, but then requires the latter to avoid duplication of output), when I invoke this, I see:

$ make -C wip/build/ pdfroff.1.pdf MAN2PDF_FLAGS=-ww
make: Entering directory '/home/keith/projects/groff-pdfmark/wip/build'
/usr/bin/sed -e 's!@VERSION@!1.23.0!' -e 's!@MDATE@!20 February 2023!' -e 's!@PDFDOCDIR@!/usr/local/share/doc/!' -e 's!@MAN\([1-9]\)EXT@!\1!g' ../../pdfroff.1.man | GROFF_TMAC_PATH=../../tmac /bin/sh ./pdfroff -man -ww > pdfroff.1.pdf
troff: ./pdfroff-jfeZdKTQvb/pdf19949.cmp:1: warning: macro 'pdfhref' not defined
troff: <standard input>:30: warning: number register '.cp' not defined
make: Leaving directory '/home/keith/projects/groff-pdfmark/wip/build'

This does, indeed, reproduce the issue which you report, (and I do have a practical solution, without requiring the '--no-reference-dictionary' option).  However, it also reveals a more insidious issue, (viz. the reference to undefined '.cp' number register), which appears to be endemic among the entire corpus of manpage sources, throughout the groff code base; it appears to have been introduced by:

$ hg annotate src/roff/groff/groff.1.man
...
3963: .\" Save and disable compatibility mode (for, e.g., Solaris 10/11).
3963: .do nr *groff_groff_1_man_C \n[.cp]
3963: .cp 0
...

$ hg log -r 3963
changeset:   3963:01c8e28fa4bd
user:        G. Branden Robinson <g.branden.robinson@gmail.com>
date:        Sun Oct 18 22:56:32 2020 +1100
summary:     man pages: Make preambles consistent.

Please note that this wider issue is not even related to pdfroff:

$ groff -ww -man src/roff/groff/groff.1 > /dev/null
troff: src/roff/groff/groff.1:25: warning: number register '.cp' not defined

Does this merit its own bug report?

Keith Marshall <keithmarshall>
Group Member
Thu 29 Sep 2022 12:22:27 PM UTC, original submission:  

Affects groff 1.22.4.


$ echo | pdfroff -ww >/dev/null
troff: /tmp/branden/pdfroff-ehedRan1l8/pdf41844.cmp:1: warning: macro 'pdfhref' not defined



G. Branden Robinson <gbranden>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by keithmarshall (Posted a comment)
  • -email is unavailable- added by gbranden (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-07-10 gbranden StatusReady for Merge Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.24.0
    2023-04-28 gbranden StatusIn Progress Ready for Merge
    2023-04-08 gbranden StatusConfirmed In Progress
    2023-04-07 gbranden Attached File- Added 69c99bee3ce93ddaae89026d61cea9bc75ecf453.diff, #54582
        Summary[pdfroff] throws warning on any document if -ww given [PATCH] [pdfroff] throws warning on any document if -ww given
    2023-04-05 gbranden StatusNone Confirmed
        Assigned toNone gbranden

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code