bugGNU roff - Bugs: bug #63757, [gropdf] doesn't handle...

 
 

bug #63757: [gropdf] doesn't handle "papersize" directive as other drivers do

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Sat 04 Feb 2023 11:36:20 AM UTC
   
 
Category:  Driver gropdf Severity:  3 - Normal
Item Group:  Incorrect behaviour Status:  Fixed
Privacy:  Public Assigned to:  deri
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 09 Feb 2023 04:36:03 PM UTC, comment #6: 

Thanks, Deri!  I'll add the whole commit message as is my wont, even if you covered the issue thoroughly in comment #4.


commit 7fb6e3ffd06c0ad8058e5de2325eef55f178ef67
Author: Deri James <deri@chuzzlewit.myzen.co.uk>
Date:   Thu Feb 9 15:27:56 2023 +0000

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

    * src/devices/gropdf/gropdf.pl: Parse papersize string for
    possible multiple (space separated) entries. First valid entry
    wins.

    [gropdf] Parse multiple entries in 'papersize' as specified in
    the groff_font man page. Reported by Ben Wong and fix based on
    his patch, thanks.


G. Branden Robinson <gbranden>
Group administrator
Thu 09 Feb 2023 03:43:09 PM UTC, comment #5: 

commit 7fb6e3ffd06c0ad8058e5de2325eef55f178ef67

This fixes the gropdf bug. I suggest the gropdf.am patch is rejected because the reference to grops.am adding /etc/papersize is wrong, ours does not, so it must be a debian patch. This should be a patch applied by debian.

With the fix to gropdf debian can add /etc/papersize to DESC and get the desired result.

Deri James <deri>
Group Member
Wed 08 Feb 2023 10:22:57 PM UTC, comment #4: 

Both patches seem fine but the gropdf one will need a slight massage. Since groff_font specifies that the papersize entry in the DESC file may contain multiple entries of 3 types (paper name, file path or x,y dimension). The current code only expects one entry, which can any of the 3 types. The patch changes the behaviour to loop over all the entries and selecting the first which is valid. It uses white space to separate entries.

Since file paths may contain spaces, this would cause issues, but given that its intention is to include /etc/papersize (which does not contain spaces) this "problem" is moot. This problem is probably in font.cpp as well.

Since I suspect this patch was created before a specific warning was added about an unrecognised paper size I need to add some code fo this. Given that another change by the patch is that a failed entry is allowed because it continues to look at following entries, but no error if none are valid. Also, it uses the papersize given in the -p flag as the first entry to check, so if that fails it will look at the entry in the DESC file which will probably be valid so the incorrect -p papersize will not be reported. I think this behaviour is undesirable, so I will change it so that if the -p flag papersize is given any DESC paper sizes are not checked, so that a warning will be given for an invalid paper size on the command line.

Given papersize.tmac is now included to make it easier to tell troff the intended document size with -dpaper=, wouldn't it make sense to pass this information on to whichever device driver is chosen. It would avoid commands such as -dpaper=letter -P-pletter, you would only need the -dpaper=... We already have an API for this, \X'papersize=x,y' so I have attached a patch which works for grodvi and gropdf. If only we had an environment variable GROFF_OPTIONS which users could set to "-dpaper=letter". I don't know if grops supports this API but it should be very easy to add since it would use the same code as it uses to parse the -p flag.

(file #54340)

Deri James <deri>
Group Member
Sat 04 Feb 2023 11:07:32 PM UTC, comment #3: 

  2) (should be 4) I forgot to mention that the formatter (gtroff)
needs to know for what paper format it should process the input, so add
option "-dpaper=$PAGE".

Bjarni Ingi Gislason <bjarniig>
Sat 04 Feb 2023 03:42:47 PM UTC, comment #2: 

  I do not see a report about a bug in the DESC file.

1) The file "configure" consults "/etc/papersize" among others to set
"PAGE".

  The only defect I see, is that "locale -k LC_PAPER" is not consulted
before "/etc/resolv.conf".

2) "papersize(5)" (Debian) states that the first valid string is used
(by the libpaper library), meaning: only one paper size is used.

There can only be one default paper format !

2) The "devpdf.am" only checks the value "$PAGE" for "A4" instead of
using its value, if defined and not empty.

2) I do not recommend, that users redefine the content of
"/etc/papersize".
They should use the "-P-p..." option for "groff" for their own
personal choices.

3) The need to include "/etc/papersize" for "gropdf" (in gropdf.am) is
not explained.

  The report looks line a proposal to change something affecting every
user for the sake of a single person, which should instead use an
available resource (solution) for a special case.

Bjarni Ingi Gislason <bjarniig>
Sat 04 Feb 2023 11:42:02 AM UTC, comment #1: 

I'm not sure about the change to gropdf's DESC file; grops doesn't ship with "/etc/papersize" in it either.

I'm not sure how standardized /etc/papersize actually is.  The papersize(5) man page on my Debian system refers to its format as being the province of the libpaper library, but groff does not link against that library, and I don't know if libpaper is as portable as groff is.  I'm unwilling to impose a default that will break on some systems.

Distributors are well-positions to know if their systems support /etc/papersize.

The other point (and patch) is well-taken, from my perspective.  gropdf should indeed behave like other output drivers, and should process the "papersize" directive in the same way.

I've assigned this ticket to gropdf author Deri James and am interested to hear his feedback.

G. Branden Robinson <gbranden>
Group administrator
Sat 04 Feb 2023 11:36:20 AM UTC, original submission:  

Over at the Debian Bug Tracking System, Ben Wong filed the following report.


Package: groff
Version: 1.22.4-9
Severity: normal
Tags: patch
X-Debbugs-Cc: bugs.debian.org@wongs.net

Dear Maintainer,

The gropdf Perl script has a few bugs.

1. It does not include /etc/papersize as the first place to check for
   the default paper size. The problem is that the "papersize"
   directive in fonts/devpdf/DESC only lists "a4" (or "letter").

        papersize a4

   Instead it should look like this:

        papersize /etc/papersize a4

2. It does not search each possible papersize listed in DESC, which is
   the documented behaviour in the groff_font manual. The fix for that
   requires editing the Perl script and splitting the papersizes on
   the space character.


To reproduce the problem, try this:

    man -Tpdf groff > groff.pdf  &&  pdfinfo groff.pdf | grep size


I have attached two patches, one for each bug. The first changes the
devpdf.am file so that it prepends "/etc/papersize" to the search. The
code was copied from devps.am.

The second patch adds a loop around the check of the $papersz variable
in gropdf.pl. Because another level of indentation was added, the diff
makes it look like more of the code changed than actually did.

Thank you.


Here are the suggested patches.


--- font/devpdf/devpdf.am.orig        2023-01-30 19:12:42.600754792 -0800
+++ font/devpdf/devpdf.am        2023-01-30 19:15:00.829075259 -0800
@@ -1,9 +1,9 @@
-# Copyright (C) 2011-2018 Free Software Foundation, Inc.
+# Copyright (C) 2011-2022 Free Software Foundation, Inc.
 #      Original Makefile.sub written
 #      by Deri James <deri@chuzzlewit.demon.co.uk>
 #      Automake migration by Bertrand Garrigues
 #
-# Last update: 2017-11-02
+# Last update: 2022-01-30
 #
 # This file is part of groff.
 #
@@ -95,9 +95,9 @@
         && $(RM) $(top_builddir)/font/devpdf/DESC \
         && cat $(devpdf_srcdir)/DESC.in >$(top_builddir)/font/devpdf/DESC \
         && if test "$(PAGE)" = A4; then \
-             echo "papersize a4" >>$(top_builddir)/font/devpdf/DESC; \
+             echo "papersize /etc/papersize a4" >>$(top_builddir)/font/devpdf/DESC; \
            else \
-             echo "papersize letter" >>$(top_builddir)/font/devpdf/DESC; \
+             echo "papersize /etc/papersize letter" >>$(top_builddir)/font/devpdf/DESC; \
            fi

 font/devpdf/Foundry:



--- src/devices/gropdf/gropdf.pl.orig        2023-01-30 17:23:40.938310920 -0800
+++ src/devices/gropdf/gropdf.pl        2023-01-30 18:55:24.859580795 -0800
@@ -248,39 +248,49 @@
 LoadDesc();

 my $unitwidth=$desc{unitwidth};
-my $papersz=$desc{papersize};
-$papersz=lc($fpsz) if $fpsz;
-
 $env{FontHT}=0;
 $env{FontSlant}=0;
 MakeMatrix();

-if (substr($papersz,0,1) eq '/' and -r $papersz)
+my $possiblesizes = $desc{papersize};
+$possiblesizes = $fpsz . " " . $possiblesizes if $fpsz;
+my $papersz;
+for $papersz ( split(" ", lc($possiblesizes)) )
 {
-    if (open(P,"<$papersz"))
+    # Check for "/etc/papersize"
+    if (substr($papersz,0,1) eq '/' and -r $papersz)
     {
-        while (<P>)
+        if (open(P,"<$papersz"))
         {
-            chomp;
-            s/# .*//;
-            next if $_ eq '';
-            $papersz=$_;
-            last
+            while (<P>)
+            {
+                chomp;
+                s/# .*//;
+                next if $_ eq '';
+                $papersz=$_;
+                last;
+            }
+            close(P);
         }
+    }

-        close(P);
+    # Allow height,width specified directly in centimeters, inches, or points.
+    if ($papersz=~m/([\d.]+)([cipP]),([\d.]+)([cipP])/)
+    {
+        @defaultmb=@mediabox=(0,0,ToPoints($3,$4),ToPoints($1,$2));
+        last;
+    }
+    # Look $papersz up as a name such as "a4" or "letter".
+    elsif (exists($ppsz{$papersz}))
+    {
+        @defaultmb=@mediabox=(0,0,$ppsz{$papersz}->[0],$ppsz{$papersz}->[1]);
+        last;
     }
-}

-if ($papersz=~m/([\d.]+)([cipP]),([\d.]+)([cipP])/)
-{
-    @defaultmb=@mediabox=(0,0,ToPoints($3,$4),ToPoints($1,$2));
-}
-elsif (exists($ppsz{$papersz}))
-{
-    @defaultmb=@mediabox=(0,0,$ppsz{$papersz}->[0],$ppsz{$papersz}->[1]);
+    # If we get here, $papersz was invalid, so try the next one.
 }

+
 my (@dt)=gmtime($ENV{SOURCE_DATE_EPOCH} || time);
 my $dt=PDFDate(\@dt);


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
file #54340:  papersize.patch added by deri (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by deri (Updated the item)
  • -email is unavailable- added by bjarniig (Posted a comment)
  • -email is unavailable- added by gbranden (Submitted the item)
  • -email is unavailable- added by gbranden (original reporter)
  •  

    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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-02-09 deri StatusConfirmed Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2023-02-08 deri Attached File- Added papersize.patch, #54340
    2023-02-04 gbranden Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.12.
    Corresponding source code