bugGNU roff - Bugs: bug #62150, [afmtodit] should not write full...

 
 

bug #62150: [afmtodit] should not write full file names to "name" directive of font descriptions

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Mon 07 Mar 2022 10:24:38 PM UTC
   
 
Category:  Font devps Severity:  3 - Normal
Item Group:  Incorrect behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 14 Mar 2022 06:07:41 AM UTC, comment #6: 

comment #5:

> Hmm, it now occurs to me: is this script guaranteed to be run on a file system that uses "/" as the directory separator?


I think so.  At some point, MS-DOS support was dropped, and all ports to Windows in the past 20-odd years or so seem to have been done on top of a POSIX emulation layer.

> If it needs to be more general, perl has a basename() function in its standard File::Basename module that is aware of the OS's conventions.


If this turns out to be an issue, I'm happy to adapt it.  But I think we have a lot of other places that assume '/' as a directory separator.

Meanwhile:

commit 9c46247e568d15f7badaed7774af8c2673bdc1ea
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Mar 9 12:09:15 2022 +1100

    [afmtodit]: Fix Savannah #62150.

    * src/utils/afmtodit/afmtodit.pl: Stop writing full file name to the
      generated font description file as the "name" (i.e., don't include
      directory components).

    Fixes <https://savannah.gnu.org/bugs/?62150>.  Thanks to Dave Kemper for
    the discussion and code review.


G. Branden Robinson <gbranden>
Group administrator
Wed 09 Mar 2022 06:58:15 PM UTC, comment #5: 

Hmm, it now occurs to me: is this script guaranteed to be run on a file system that uses "/" as the directory separator?  If it needs to be more general, perl has a basename() function in its standard File::Basename module that is aware of the OS's conventions.

comment #4:

> A fellow student in my high school programming class
> (Pascal-based) discovered for himself the arbitrariness
> of variable names, recognizing that the machine did not
> care what we called them.  So on one occasion he named
> something, I think it was a loop index, "BANANA".


A coding session for one of my advanced college programming classes stretched into the wee hours, and my project partner and I were a little slap-happy.  At one point when he was at the keyboard, we realized we needed a new variable for some flag.  He asked me what we should name it, and I offered "Fred."  He dutifully typed this in, and it remained in the code we eventually submitted.  Moral: Do not listen to me when it comes to naming variables.

Dave <barx>
Group Member
Wed 09 Mar 2022 04:28:08 AM UTC, comment #4: 

Hi Dave,

Here's a fresh version of the patch, as I have committed locally (but not pushed).


diff --git a/src/utils/afmtodit/afmtodit.pl b/src/utils/afmtodit/afmtodit.pl
index 73ae7c65f..afaabc219 100644
--- a/src/utils/afmtodit/afmtodit.pl
+++ b/src/utils/afmtodit/afmtodit.pl
@@ -48,8 +48,8 @@ if ($#ARGV != 2) {

 my $afm = $ARGV[0];
 my $map = $ARGV[1];
-my $font = $ARGV[2];
-my $outfile = $opt_o || $font;
+my $fontfile = $ARGV[2];
+my $outfile = $opt_o || $fontfile;
 my $desc = $opt_d || "DESC";
 my $sys_map = $groff_sys_fontdir . "/devps/generate/" . $map;
 my $sys_desc = $groff_sys_fontdir . "/devps/" . $desc;
@@ -445,7 +445,10 @@ if ($opt_c) {

 print("\n");

-print("name $font\n");
+my $name = $fontfile;
+$name =~ s@.*/@@;
+
+print("name $name\n");
 print("internalname $psname\n") if $psname;
 print("special\n") if $opt_s;
 printf("slant %g\n", $italic_angle) if $italic_angle != 0;


comment #3:

> comment #2:
> > I have conflicting goals.
>
> As some internet wag once said, "I don't have a solution, but I admire the problem."


That's a good saying.  :D

> Won't argue against that precept...but "$font" seems a fairly solid variable name to hold a font name.


Yes--but at first it's a file system object.

> > Perl is not the level of programming where we need to emulate
> > a register allocator.
>
> True, but I was thinking more of the human reader.  I read that, and I think, "oh, he's preserving the old value so he can use it again later."  However, I cannot vouch that my interpretation is universal, or even common.


It should be common to an experienced programmer.  It simply isn't the case this time.

A fellow student in my high school programming class (Pascal-based) discovered for himself the arbitrariness of variable names, recognizing that the machine did not care what we called them.  So on one occasion he named something, I think it was a loop index, "BANANA".

He ended up being a lawyer.

G. Branden Robinson <gbranden>
Group administrator
Wed 09 Mar 2022 03:02:53 AM UTC, comment #3: 

comment #2:

> I have conflicting goals.


As some internet wag once said, "I don't have a solution, but I admire the problem."

(And anyway, this part of the patch is off the topic of this ticket.)

> I prefer semantic to misleading variable names.  ;-)


Won't argue against that precept...but "$font" seems a fairly solid variable name to hold a font name.

> Perl is not the level of programming where we need to emulate
> a register allocator.


True, but I was thinking more of the human reader.  I read that, and I think, "oh, he's preserving the old value so he can use it again later."  However, I cannot vouch that my interpretation is universal, or even common.

Dave <barx>
Group Member
Wed 09 Mar 2022 12:59:24 AM UTC, comment #2: 

comment #1:

> Hey, thanks for opening this bug report!
>
> A couple nits in the proposed patch:
>
> > +$short_version =~ s/([0-9.]+)(rc[0-9]+).*/$1$2/;
>
> That regex (equivalently but more simply written as
>
>   /([0-9.]+rc[0-9]+).*/$1/
>
> and more perlishly as
>
>   /([\d.]+rc\d+).*/$1/
>
> ) fails to match, and thus leaves $short_version unchanged, if the version string contains no .rc component, which a development build might not (if I correctly recall from the very-long-ago days when we weren't in a release cycle).


Yes, I shared a highly preliminary and un-fully-baked version of the patch.  :)  I was also dithering over the issue you raised next, of just how much version info to include.
 

> One could craft a regex to strip the git addendum whether or not the version string contains an .rc component.  However, absent such a component, you may want to keep the git-appended portion, as this might then be the only thing distinguishing an official release from a development build.  (If those dusty neurons serve correctly, our pre-rc1 build version strings were of the form 1.22.4.git-stuff; turning this into plain 1.22.4 seems misleading.)  Thus, maybe the limitation I'm noting is intentional.


I have conflicting goals.

1. I wanted to get some of the noise out of the version number for re-generated PostScript font description files that we can ship in a release.
2. I wanted to make it possible to regenerate these files well before making the final 1.23.0 tag.  The idea is to to have all the work essentially done for the release (at whatever point) that happens, and then just make the tags and send an email.  The less stuff that has to be done to finalize the release on the day it happens, the easier it will be to increase the release cadence.

> But then, if you're keeping .git-stuff in some cases, why not keep it in all cases for consistency?  Yeah, it's kinda ugly, but for the most part only developers will see it, and they should be the people least put off by long strings of hex digits.  (And might even occasionally appreciate the extra precision.)


Yeah.  What I had in that diff ended up between two stools.

> > -print("name $font\n");
> > +my $name = $font;
> > +$name =~ s@.*/@@;
> > +
> > +print("name $name\n");
>
> Nothing wrong with this, but I wonder why it introduces a new variable.  $font is never again used after this, so may as well hack it directly.


Because I prefer semantic to misleading variable names.  ;-)  Perl is not the level of programming where we need to emulate a register allocator.  It might help if I renamed $font to $font_file or similar.

Thanks for the review!  Very soon my grand plan to regenerate those font descriptions is going to be gated on finding the most recent versions of the AFM files from Adobe, which I recently learned is discontinuing Type 1 support altogether, and has done so already in some products.

I see that printafm(1) is of ancient vintage, and probably available anywhere GhostScript is.  TrueType handling might therefore be easier than I thought.

G. Branden Robinson <gbranden>
Group administrator
Tue 08 Mar 2022 05:48:36 AM UTC, comment #1: 

Hey, thanks for opening this bug report!

A couple nits in the proposed patch:

> +$short_version =~ s/([0-9.]+)(rc[0-9]+).*/$1$2/;


That regex (equivalently but more simply written as

  /([0-9.]+rc[0-9]+).*/$1/

and more perlishly as

  /([\d.]+rc\d+).*/$1/

) fails to match, and thus leaves $short_version unchanged, if the version string contains no .rc component, which a development build might not (if I correctly recall from the very-long-ago days when we weren't in a release cycle).

One could craft a regex to strip the git addendum whether or not the version string contains an .rc component.  However, absent such a component, you may want to keep the git-appended portion, as this might then be the only thing distinguishing an official release from a development build.  (If those dusty neurons serve correctly, our pre-rc1 build version strings were of the form 1.22.4.git-stuff; turning this into plain 1.22.4 seems misleading.)  Thus, maybe the limitation I'm noting is intentional.

But then, if you're keeping .git-stuff in some cases, why not keep it in all cases for consistency?  Yeah, it's kinda ugly, but for the most part only developers will see it, and they should be the people least put off by long strings of hex digits.  (And might even occasionally appreciate the extra precision.)

> -print("name $font\n");
> +my $name = $font;
> +$name =~ s@.*/@@;
> +
> +print("name $name\n");


Nothing wrong with this, but I wonder why it introduces a new variable.  $font is never again used after this, so may as well hack it directly.

Dave <barx>
Group Member
Mon 07 Mar 2022 10:24:38 PM UTC, original submission:  

Dave observed in bug #61423:

> A recent change -- I'm gonna go out on a limb and accuse September's commit c0d1bb28, though I've not done before/after testing to confirm this -- now prevents these files from loading, troff giving up with the error "font description file name 'JR' does not match 'name' argument '/full/path/to/JR'".


> (In my case, the /full/path/to/JR as specified in the font description file did not match the actual full path of its final resting place; apparently I used some tool to generate the file in a working directory, and this tool encoded the full path of its working directory onto this line.  But even when I updated the path to match the file's current location, it still failed with the same error.)


That bug deals with deeper architectural stuff, but in the meantime, afmtodit should not be writing anything but the "basename" as the name of the font.

Having now looked at the code, I suspect the fact that it wrote the full file name was an oversight.


diff --git a/src/utils/afmtodit/afmtodit.pl b/src/utils/afmtodit/afmtodit.pl
index 73ae7c65f..5886c1ecc 100644
--- a/src/utils/afmtodit/afmtodit.pl
+++ b/src/utils/afmtodit/afmtodit.pl
@@ -421,8 +421,13 @@ foreach my $lig (sort keys %default_ligatures) {
 open(FONT, ">$outfile") || die "$prog: can't open '$outfile' for output: $!\n";
 select(FONT);

-print("# This file has been generated with " .
-      "GNU afmtodit (groff) version @VERSION@\n");
+# Produce a short version number so noise from a Git build doesn't leak
+# into the generated files when we run this tool in "maintainer mode".
+my $short_version = "@VERSION@";
+$short_version =~ s/([0-9.]+)(rc[0-9]+).*/$1$2/;
+
+print("# This file was generated with " .
+      "GNU afmtodit (groff) version $short_version\n");
 print("#\n");
 print("#   $fullname\n") if defined $fullname;
 print("#   $version\n") if defined $version;
@@ -445,7 +450,10 @@ if ($opt_c) {

 print("\n");

-print("name $font\n");
+my $name = $font;
+$name =~ s@.*/@@;
+
+print("name $name\n");
 print("internalname $psname\n") if $psname;
 print("special\n") if $opt_s;
 printf("slant %g\n", $italic_angle) if $italic_angle != 0;


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:
   

No files currently attached

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2022-10-05 gbranden CategoryDriver grops Font devps
    2022-03-14 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2022-03-08 barx Carbon-Copy- Added barx

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code