Bug 528949 - Review Request: drehatlas-widelands-fonts - Widelands font
Review Request: drehatlas-widelands-fonts - Widelands font
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dave Ludlow
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-14 09:42 EDT by Dave Ludlow
Modified: 2009-11-04 07:14 EST (History)
3 users (show)

See Also:
Fixed In Version: 1.0.2.2-6.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-11-04 07:14:46 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
nicolas.mailhot: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dave Ludlow 2009-10-14 09:42:09 EDT
Spec URL: http://www.adsllc.com/tmp/oflb-Widelands-fonts.spec
SRPM URL: http://www.adsllc.com/tmp/oflb-Widelands-fonts-1.0.2.2-1.fc11.src.rpm
Description:
This font was created by Nasenbaer as a logo for the Widelands project, but
has since been enhanced.  The font is decorative and not really usable for
real texts, but it looks nice for logos, banners or similar stuff.  All normal
latin chars and arabic numbers are included and usable.
Comment 1 Dave Ludlow 2009-10-14 09:43:37 EDT
CC the fedora fonts bugs list
Comment 2 Dave Ludlow 2009-10-14 15:29:29 EDT
Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1746547

$ rpmlint SRPMS/oflb-Widelands-fonts-1.0.2.2-1.fc11.src.rpm RPMS/noarch/oflb-Widelands-fonts-1.0.2.2-1.fc11.noarch.rpm 
2 packages and 0 specfiles checked; 0 errors, 0 warnings.
Comment 3 Nicolas Mailhot 2009-10-14 15:54:28 EDT
Please only use lowercase in the package name. Before F12 that's a packaging guidelines error, starting from F12 it'll also cause the build to fail  (since new code assumes packages respect naming guidelines)

http://fedoraproject.org/wiki/Fonts_packaging_policy#Naming
Comment 4 Nicolas Mailhot 2009-10-14 16:29:13 EDT
That being said, let's do a more thorough review

1. when creating a new file (such as the fontconfig rules one) it's better to use SourceX instead of patch, that produces a simple patch in the vcs people can look at or import. Using patch only adds lots of not-really useful +s before each line

2. likewise for simple operations such as re-wrapping text files it's better to script them instead of creating a trivial patch that will need to be rebased every few releases

I often use something like

for txt in *.txt; do
   fold -s $txt > $txt.new &&\
   sed -i 's/\r//' $txt.new &&\
   touch -r $txt $txt.new &&\
   mv $txt.new $txt
done

(you can remove the sed if you don't need to fix DOS EOLs, and add an iconv if necessary)

3. We sadly do not seem to have a standard script to invoque fontforge. So I can't say if your way is better or worse than others. It's probably a good idea to write a small makefile and get it included upstream so upstream at least will check it's happy with the way we generate the font. Someday we'll need to codify this so all our packages have the same bugs :)

4. you can put multiple files on a single %doc line

The rest seems fine (but I'll need a package buildable in rawhide to run our full battery of tests

Thank you for submitting this package, it's not perfect but it's also not too bad and you were awfully quick :)
Comment 5 Nicolas Mailhot 2009-10-14 16:42:27 EDT
And lastly (I'm not very efficient today, sorry) it's good style not to repeat the package name (or something similar) in the summary, since GUI and CLI tools already print the package name next to it. So you need to be a bit more creative
Comment 6 Dave Ludlow 2009-10-14 17:30:36 EDT
Renamed, patches replaced with SourceX's, Makefile used for conversion, %docs
combined.  Summary updated.  Waiting to hear back from upstream; will pass the
updates on when I do.

$ rpmlint oflb-widelands-fonts.spec oflb-widelands-fonts-1.0.2.2-2.fc11.src.rpm
oflb-widelands-fonts-1.0.2.2-2.fc11.noarch.rpm
2 packages and 1 specfiles checked; 0 errors, 0 warnings.

SPEC:
http://adsllc.fedorapeople.org/rpmbuild/SPECS/oflb-widelands-fonts.spec

Koji scratch build against f12:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1746690

Thanks for the review and helpful suggestions - I learned a few things.  Any
suggestions on getting a rawhide buildable package?
Comment 7 Dave Ludlow 2009-10-14 17:37:23 EDT
Fuedal->Feudal

SPEC replaced, Koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1746712
Comment 8 Nicolas Mailhot 2009-10-14 17:54:55 EDT
(In reply to comment #6)

> Thanks for the review and helpful suggestions - I learned a few things.  Any
> suggestions on getting a rawhide buildable package?  

lowercasing the package name should have been sufficient, I'll check it tomorrow (my time is up today)

Another point: oflb- in a font context means "a font downloaded from www.openfontlibrary.org" Since that does not seem the case for this package, you need to define another prefix

You have some documented examples here
http://fedoraproject.org/wiki/Fonts_packaging_policy#Naming

We prefix font package names by foundry to group packages with the same origin in package management tools, since an entity that produced good fonts will tend to publish other good fonts, and an entity that produced average fonts unfortunately rarely gets much better (or improves its old fonts at the same time). So this is useful to the user. Other SIGs maye have made similar choices (or not)
Comment 9 Nicolas Mailhot 2009-10-15 18:21:56 EDT
This one is good technically

I'd simplify the summary to
“A Latin typeface inspired by feudal calligraphy”

Also technically "glyph" is a better term than "char"

You didn't keep the touch after the fold, it's used to keep the original file timestamp (but that do not matter as much in recent rpm as it used to before, so this is just FYI)

But this is really nitpicking on my part. Ignore me if you don't want to bother

So the only remaining problem is the naming. Since the author publishes two other fonts, that may be packaged someday, finding a nice prefix to identify him is necessary (unfortunately renaming packages is one thing the Fedora infra is very bad at, so if you make a mistake here you'll be stuck with it quite a long time). pschwanemann- would be consistent with apanov- but a bit long. Feel free to use this or invent a better one (or ask upstream for a better handle)
Comment 10 Dave Ludlow 2009-10-16 09:52:09 EDT
Waiting on a response from upstream.  I would ideally like them to incorporate the font-building Makefile and either update the release on oflb or give their preferred foundry name.
Comment 11 Dave Ludlow 2009-10-16 12:59:23 EDT
Heard from upstream.  This attempt changes the name as suggested, adds the touch, and updates the font conversion to add .ttf output abilities in addition to .otf.  We don't actually use the .ttf stuff, but upstream may want it.

SPEC:
http://adsllc.fedorapeople.org/rpmbuild/SPECS/drehatlas-widelands-fonts.spec

Koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1750409

This is a little premature - I'm waiting on a final confirmation from upstream before calling it official.
Comment 12 Nicolas Mailhot 2009-10-16 16:29:13 EDT
(In reply to comment #11)

> Koji:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1750409
> 
> This is a little premature - I'm waiting on a final confirmation from upstream
> before calling it official.

Well, this one is good

⨒⨒⨒ APPROVED ⨒⨒⨒

(Note that I'm not asking you to push it at once, just confirming this particular version is good enough for Fedora, and that you can push it or something similar when you feel ready. You can still wait for upstream confirmation or ask new questions here. But I don't think I should block you from now on)

When you feel ready, you can continue from
http://fedoraproject.org/wiki/Font_package_lifecycle#3.a

I hope the process was pleasant, and that it will inspire you to package a
other fonts for Fedora. Please do not hesitate to suggest improvements to our
organisation on the fonts mailing list.

Thank you for your contribution to our font package pool.

⇒ REASSIGNING
Comment 13 Dave Ludlow 2009-10-17 11:13:33 EDT
Upstream has confirmed - let's make it official.

New Package CVS Request
=======================
Package Name: drehatlas-widelands-fonts
Short Description: A Latin typeface inspired by feudal calligraphy
Owners: adsllc
Branches: F-10 F-11 F-12
InitialCC: fonts-sig
Comment 14 Nicolas Mailhot 2009-10-17 12:24:08 EDT
If upstream does not fix its “Decorative” buglet please do not change upstream's zip — write a fontforge script to patch the font, or look at
/usr/share/fontconfig/templates/remapping-font-template.*
Comment 15 Kevin Fenzi 2009-10-17 16:40:00 EDT
cvs done.
Comment 16 Fedora Update System 2009-10-17 22:46:06 EDT
drehatlas-widelands-fonts-1.0.2.2-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/drehatlas-widelands-fonts-1.0.2.2-6.fc11
Comment 17 Fedora Update System 2009-10-17 22:46:17 EDT
drehatlas-widelands-fonts-1.0.2.2-6.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/drehatlas-widelands-fonts-1.0.2.2-6.fc12
Comment 18 Fedora Update System 2009-10-17 22:46:26 EDT
drehatlas-widelands-fonts-1.0.2.2-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/drehatlas-widelands-fonts-1.0.2.2-6.fc10
Comment 19 Fedora Update System 2009-10-20 20:39:46 EDT
drehatlas-widelands-fonts-1.0.2.2-6.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update drehatlas-widelands-fonts'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-10589
Comment 20 Fedora Update System 2009-10-20 20:39:58 EDT
drehatlas-widelands-fonts-1.0.2.2-6.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update drehatlas-widelands-fonts'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-10593
Comment 21 Fedora Update System 2009-11-04 07:14:41 EST
drehatlas-widelands-fonts-1.0.2.2-6.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 22 Fedora Update System 2009-11-04 07:14:53 EST
drehatlas-widelands-fonts-1.0.2.2-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.