Bug 533919 - Review Request: mplus-fonts - The M+ family of fonts designed by Coji Morishita
Summary: Review Request: mplus-fonts - The M+ family of fonts designed by Coji Morishita
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-11-09 19:08 UTC by Igshaan Mesias
Modified: 2011-01-11 18:32 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-01-11 18:32:14 UTC
nicolas.mailhot: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)
repo-font-audit data for this package (16.25 KB, application/x-xz)
2009-11-28 15:02 UTC, Nicolas Mailhot
no flags Details
repo-font-audit data for this package (14.85 KB, application/x-xz)
2010-02-22 17:50 UTC, Nicolas Mailhot
no flags Details

Description Igshaan Mesias 2009-11-09 19:08:56 UTC
Spec URL: http://www.box.net/shared/y6divetbvo
SRPM URL: http://www.box.net/shared/n6hgdrds4v
Description: 
The M+ outline fonts is a superfamily of fonts designed by Coji Morishita.
There are 7 families of which, 4 are combinations of proportinal font families.

This is my very first submission please be gentle ;-)

Comment 1 Nicolas Mailhot 2009-11-28 14:57:38 UTC
Here is my package review. Sorry it took longer than usual, was busy hacking a packager checker script (and given m+ is composed of 43 files, I'm so glad I took the time to finish the script before looking at it; could not have done the same level of review manually)

This is an awesome font set, thank you very much for packaging it!

Anyway, the review:

1. (not blocking) you don't include any fontconfig files, they would make your user's life a lot easier (however, CJK is quite a mess in fontconfig, so maybe it's better that way for now. Let's see what CJK users think of it)

2. (not blocking) rpmlint complains at the licensing string. I know the MPlus license has been approved by spot, so it's not a legal problem. However I fear all the repo-checking scripts will complain at you because of that. Please open a fedora rpmlint bug so the license gets added to its list. (however upstream would have made our life easier by using a standard font license such as the OFL or the GPL with font exception)

3. (not blocking) rpmlint complains some of the lines you used in descriptions are too long. Please break them in smaller (<80) bits

4. (blocking) same problem for at least one for your summaries

5. (not blocking) it seems rpmlint grew a spell-checker in fedora-devel. Please fix the spelling mistakes it has identified (ie everything which is not a human name)

6. (blocking) please make sure your Source0 is a full URL pointing to the source file. That will make your future packager life a lot easier (we have many sf-hosted projects in Fedora, so it can be done)

7. (not blocking) you don't need to repeat the "Group:    User Interface/X" line in Fedora releases with a recent rpm

8. (not blocking) it would be nice if each sub-package had a different summary. What is the difference between 1c and 2c ?

9. (blocking) m++ipa.pe is a build script, it has nothing to do in %doc

10. (not blocking) you don't need to specify %dir %{_fontdir} in the common subpackage, it will be added automatically to each font subpackage

11. (not blocking) it's mostly cosmetic, but it would be nice if the font files cased the style names they declare (ie "Bold" instead of "bold"). Apps are not always smart enough to correct this

12. (not blocking) a few font files declare an "heavy" weight. The standard qualifier for "heavy" is "Black". That means weight selection won't work as expected in apps not smart enough to try every possible "Black" legacy alias. Please ask upstream to change this (also please have upstream check they did mean "Black" and not some other weight). Standard style qualifiers and their meaning have been described by Microsoft in the following whitepaper:
http://blogs.msdn.com/text/attachment/2249036.ashx

13. (not blocking) the fonts fall just short of complete coverage for several languages and unicode blocks (we only test if a language/block needs less than 10 glyphs to be finished). Please relay upstream so it gets a chance to complete them (for example, ab is incomplete because the Unicode consortium added two glyphs to it this year IIRC)

14. (not blocking) it would be nice if upstream included the text of their license in the fonts copyright field, and not just "Copyright(c) 2009 M+ FONTS PROJECT" without licensing info

15. (not blocking) it would be nice if upstream released a source archive with source files and build scripts, so we can re-build in Fedora using our own fontforge version (that helps finding bugs in fontforge, and as our fontforge improves, so do the fonts we build with it)

Please fix whatever you can and relay the requests to upstream when appropriate. I'll attach the output of latest repo-font-audit for your package so you have a copy of rpmlint errors, font naming warnings, and font coverage analysis.

There are a lot of requests here but your package is quite complex. Given the number of packaged font files both you and upstream did a very good job, I've seen much worse in packages that only included a couple font files.

NEEDINFO for now

Comment 2 Nicolas Mailhot 2009-11-28 15:02:31 UTC
Created attachment 374418 [details]
repo-font-audit data for this package

Comment 3 Igshaan Mesias 2009-12-16 20:47:15 UTC
Hi Nicolas,

Thanks for your package review and I will shortly tackle some of the above mentioned tasks, I do however, detect it being a slow process as I'm a bit pressed for time. 

For now I'll try and look at some of the rpmlint errors and file a bug for the license bit. Thanks again.

Ciao,
Igshaan

Comment 4 Nicolas Mailhot 2009-12-17 08:55:50 UTC
Hi Igshaan,

It's perfectly ok to take time do do things right, as long as you actually do it. I only dislike people who make you spend a lot of time on package reviews, and then never actually finish their package :(

Comment 5 Igshaan Mesias 2010-01-12 11:44:24 UTC
Hi Nicolas,

Upstream released a new version. I'm packaging that version and have completely re-written the spec file, though, still adhering to the points below.

> Anyway, the review:
> 
> 1. (not blocking) you don't include any fontconfig files, they would make your
> user's life a lot easier (however, CJK is quite a mess in fontconfig, so maybe
> it's better that way for now. Let's see what CJK users think of it)

I have yet to come up with a decent fontconfig, but for now I'll be omitting it. 

> 2. (not blocking) rpmlint complains at the licensing string. I know the MPlus
> license has been approved by spot, so it's not a legal problem. However I fear
> all the repo-checking scripts will complain at you because of that. Please open
> a fedora rpmlint bug so the license gets added to its list. (however upstream
> would have made our life easier by using a standard font license such as the
> OFL or the GPL with font exception)

I have filed a bug against rpmlint, and it seems the issue has now been resolved ie, I no longer get rpmlint errors about the licensing at version 0.91.

> 3. (not blocking) rpmlint complains some of the lines you used in descriptions
> are too long. Please break them in smaller (<80) bits
> 
> 4. (blocking) same problem for at least one for your summaries
>

I've made sure all the lines in the summary and descriptions are less than 80 characters.

 
> 5. (not blocking) it seems rpmlint grew a spell-checker in fedora-devel. Please
> fix the spelling mistakes it has identified (ie everything which is not a human
> name)

For someone who's native language should be english, I hope I fixed all the spelling mistakes.
 
> 6. (blocking) please make sure your Source0 is a full URL pointing to the
> source file. That will make your future packager life a lot easier (we have
> many sf-hosted projects in Fedora, so it can be done)

I have placed a url pointing to the tarball in the Source tag.

> 7. (not blocking) you don't need to repeat the "Group:    User Interface/X"
> line in Fedora releases with a recent rpm

I have removed the unnecessary Group tags from subpackages.
 
> 8. (not blocking) it would be nice if each sub-package had a different summary.
> What is the difference between 1c and 2c ?
the one is Type 1 the other is Type 2.

> 9. (blocking) m++ipa.pe is a build script, it has nothing to do in %doc

I am excluding the build script for now.
 
> 10. (not blocking) you don't need to specify %dir %{_fontdir} in the common
> subpackage, it will be added automatically to each font subpackage

removed.

> 11. (not blocking) it's mostly cosmetic, but it would be nice if the font files
> cased the style names they declare (ie "Bold" instead of "bold"). Apps are not
> always smart enough to correct this

I will try and fix, with some help as I am still newbie when it comes to fonts.

> 12. (not blocking) a few font files declare an "heavy" weight. The standard
> qualifier for "heavy" is "Black". That means weight selection won't work as
> expected in apps not smart enough to try every possible "Black" legacy alias.
> Please ask upstream to change this (also please have upstream check they did
> mean "Black" and not some other weight). Standard style qualifiers and their
> meaning have been described by Microsoft in the following whitepaper:
> http://blogs.msdn.com/text/attachment/2249036.ashx

Should I speak to upstream about this?

> 13. (not blocking) the fonts fall just short of complete coverage for several
> languages and unicode blocks (we only test if a language/block needs less than
> 10 glyphs to be finished). Please relay upstream so it gets a chance to
> complete them (for example, ab is incomplete because the Unicode consortium
> added two glyphs to it this year IIRC)

AFAIK the font is not complete as of yet.

> 14. (not blocking) it would be nice if upstream included the text of their
> license in the fonts copyright field, and not just "Copyright(c) 2009 M+ FONTS
> PROJECT" without licensing info

I have spoke to upstream regarding this, he seems unwilling to change the license.

> 15. (not blocking) it would be nice if upstream released a source archive with
> source files and build scripts, so we can re-build in Fedora using our own
> fontforge version (that helps finding bugs in fontforge, and as our fontforge
> improves, so do the fonts we build with it)

I have yet to ask upstream.


Would you kindly confirm whether the above is OK before I post the links to the new srpms and spec files.

Thanks,
Igshaan

Comment 6 Igshaan Mesias 2010-01-19 13:15:33 UTC
Hi,


Package has been updated to 028,

Here's the link to the new src, and spec file.
http://github.com/imesias/Mplus-fonts.git

Kindly review, and post feedback.

Thanks,
Igshaan


(In reply to comment #5)
> Hi Nicolas,
> 
> Upstream released a new version. I'm packaging that version and have completely
> re-written the spec file, though, still adhering to the points below.
> 
> > Anyway, the review:
> > 
> > 1. (not blocking) you don't include any fontconfig files, they would make your
> > user's life a lot easier (however, CJK is quite a mess in fontconfig, so maybe
> > it's better that way for now. Let's see what CJK users think of it)
> 
> I have yet to come up with a decent fontconfig, but for now I'll be omitting
> it. 
> 
> > 2. (not blocking) rpmlint complains at the licensing string. I know the MPlus
> > license has been approved by spot, so it's not a legal problem. However I fear
> > all the repo-checking scripts will complain at you because of that. Please open
> > a fedora rpmlint bug so the license gets added to its list. (however upstream
> > would have made our life easier by using a standard font license such as the
> > OFL or the GPL with font exception)
> 
> I have filed a bug against rpmlint, and it seems the issue has now been
> resolved ie, I no longer get rpmlint errors about the licensing at version
> 0.91.
> 
> > 3. (not blocking) rpmlint complains some of the lines you used in descriptions
> > are too long. Please break them in smaller (<80) bits
> > 
> > 4. (blocking) same problem for at least one for your summaries
> >
> 
> I've made sure all the lines in the summary and descriptions are less than 80
> characters.
> 
> 
> > 5. (not blocking) it seems rpmlint grew a spell-checker in fedora-devel. Please
> > fix the spelling mistakes it has identified (ie everything which is not a human
> > name)
> 
> For someone who's native language should be english, I hope I fixed all the
> spelling mistakes.
> 
> > 6. (blocking) please make sure your Source0 is a full URL pointing to the
> > source file. That will make your future packager life a lot easier (we have
> > many sf-hosted projects in Fedora, so it can be done)
> 
> I have placed a url pointing to the tarball in the Source tag.
> 
> > 7. (not blocking) you don't need to repeat the "Group:    User Interface/X"
> > line in Fedora releases with a recent rpm
> 
> I have removed the unnecessary Group tags from subpackages.
> 
> > 8. (not blocking) it would be nice if each sub-package had a different summary.
> > What is the difference between 1c and 2c ?
> the one is Type 1 the other is Type 2.
> 
> > 9. (blocking) m++ipa.pe is a build script, it has nothing to do in %doc
> 
> I am excluding the build script for now.
> 
> > 10. (not blocking) you don't need to specify %dir %{_fontdir} in the common
> > subpackage, it will be added automatically to each font subpackage
> 
> removed.
> 
> > 11. (not blocking) it's mostly cosmetic, but it would be nice if the font files
> > cased the style names they declare (ie "Bold" instead of "bold"). Apps are not
> > always smart enough to correct this
> 
> I will try and fix, with some help as I am still newbie when it comes to fonts.
> 
> > 12. (not blocking) a few font files declare an "heavy" weight. The standard
> > qualifier for "heavy" is "Black". That means weight selection won't work as
> > expected in apps not smart enough to try every possible "Black" legacy alias.
> > Please ask upstream to change this (also please have upstream check they did
> > mean "Black" and not some other weight). Standard style qualifiers and their
> > meaning have been described by Microsoft in the following whitepaper:
> > http://blogs.msdn.com/text/attachment/2249036.ashx
> 
> Should I speak to upstream about this?
> 
> > 13. (not blocking) the fonts fall just short of complete coverage for several
> > languages and unicode blocks (we only test if a language/block needs less than
> > 10 glyphs to be finished). Please relay upstream so it gets a chance to
> > complete them (for example, ab is incomplete because the Unicode consortium
> > added two glyphs to it this year IIRC)
> 
> AFAIK the font is not complete as of yet.
> 
> > 14. (not blocking) it would be nice if upstream included the text of their
> > license in the fonts copyright field, and not just "Copyright(c) 2009 M+ FONTS
> > PROJECT" without licensing info
> 
> I have spoke to upstream regarding this, he seems unwilling to change the
> license.
> 
> > 15. (not blocking) it would be nice if upstream released a source archive with
> > source files and build scripts, so we can re-build in Fedora using our own
> > fontforge version (that helps finding bugs in fontforge, and as our fontforge
> > improves, so do the fonts we build with it)
> 
> I have yet to ask upstream.
> 
> 
> Would you kindly confirm whether the above is OK before I post the links to the
> new srpms and spec files.
> 
> Thanks,
> Igshaan

Comment 7 Nicolas Mailhot 2010-02-22 17:49:45 UTC
This is awesome packaging, thank you for your work. I'm sure it was at least as scary to make as it was to review, you'll probably find other font sets easier to package in the future (superfamilies are not that common)

Anyway

1. (not blocking) add some fontconfig rules someday

2. (not blocking) spell-checking fixes:

They are x weights ⇒ Includes x weights
emphasize the balance ⇒ emphasizes the balance

But for such a complex package that's nothing


ㆫㆫㆫ APPROVED ㆫㆫㆫ

Attached the a repo-font-audit check of your last packages. Naming and coverage problems only, to relay upstream

⇒ RE-ASSIGING

Comment 8 Nicolas Mailhot 2010-02-22 17:50:16 UTC
Created attachment 395525 [details]
repo-font-audit data for this package

Comment 9 Paul Flo Williams 2010-09-21 18:27:53 UTC
Just a reminder that Igshaan will need sponsoring before he can submit an SCM request for this package. Can someone advise whether he needs to do more work?

Comment 10 Hans de Goede 2010-11-03 12:22:34 UTC
Hi,

I'm not sure what the status of this review request is. But I see that an awesome amount of work has been done, both by the submitter and the reviewer. I would be more then happy to sponsor Igshaan if he still needs a sponsor.

Regards,

Hans

Comment 11 Igshaan Mesias 2010-11-04 09:22:50 UTC
Hi Hans,

(In reply to comment #10)
> Hi,
> 
> I'm not sure what the status of this review request is. But I see that an
> awesome amount of work has been done, both by the submitter and the reviewer. I
> would be more then happy to sponsor Igshaan if he still needs a sponsor.
> 
> Regards,
> 
> Hans

Nicolas Mailhot has approved the package, Thank you for your assistance in sponsorship. It's greatly appreciated. I'm quite new to the process and I assume that I will need to be added to the packager group before submitting the SCM request? Could someone advise?

Regards,
Igshaan

Comment 12 Paul Flo Williams 2010-11-04 11:15:17 UTC
You're quite correct. After you're sponsored, you'll be able to set the Flags at the top of the bug -- you'll see the fedora-cvs flag there. Be patient, it may take a day.

Comment 13 Hans de Goede 2010-11-04 16:05:00 UTC
Hi,

I've done quick review of the package , which thanks to Nicolas very thorough review of course is fine (you first package must be reviewed by a sponsor hence the re-review, a mere formality in this case).

I've added you to the packager group in the account system and sponsored you, so you should be able to proceed with the next steps in the process:
http://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Note that it may take up to an hour for the change in the account system to make its way into bugzilla, and before that you cannot set the fedora-cvs flag.

Welcome as a Fedora packager!

Regards,

Hans

Comment 14 Igshaan Mesias 2010-11-18 21:51:05 UTC
New Package SCM Request
=======================
Package Name: mplus-fonts
Short Description: The Mplus fonts is a superfamily of fonts designed by Coji Morishita
Owners: imesias
Branches: f12 f13 f14 el6
InitialCC: imesias fonts-sig

Comment 15 Jason Tibbitts 2010-11-18 22:01:43 UTC
Some notes:

It is too late to request f12 branches.
The package name you request must match what is in the ticket summary.

I urge you to follow the information in 
http://fedoraproject.org/wiki/Package_SCM_admin_requests carefully; otherwise, your request will not be processed.

Comment 16 Igshaan Mesias 2010-11-19 05:54:00 UTC
New Package SCM Request
=======================
Package Name: mplus fonts
Short Description: The Mplus fonts is a superfamily of fonts designed by Coji
Morishita
Owners: imesias
Branches: f13 f14 el6
InitialCC: imesias fonts-sig

Comment 17 Paul Flo Williams 2010-11-19 07:15:13 UTC
Igshaan, you need to edit the bug title to make a valid package name, because they can't contain spaces. When you login to Bugzilla, you'll see a little "(edit)" link alongside the title.

Change this title to say "Review Request: mplus-fonts ..." instead of "mplus fonts", and add another SCM Request.

Comment 18 Igshaan Mesias 2010-11-19 08:35:08 UTC
(In reply to comment #17)
> Igshaan, you need to edit the bug title to make a valid package name, because
> they can't contain spaces. When you login to Bugzilla, you'll see a little
> "(edit)" link alongside the title.
> 
> Change this title to say "Review Request: mplus-fonts ..." instead of "mplus
> fonts", and add another SCM Request.

My apologies, Thanks for all your help, my brain is still doing POST... it hasn't booted up yet.

Comment 19 Igshaan Mesias 2010-11-19 08:36:16 UTC
New Package SCM Request
=======================
Package Name: mplus-fonts
Short Description: The Mplus fonts is a superfamily of fonts designed by Coji
Morishita
Owners: imesias
Branches: f13 f14 el6
InitialCC: imesias fonts-sig

Comment 20 Jason Tibbitts 2010-11-22 13:50:43 UTC
For some reason this ticket is assigned to the submitter.  It needs to be
assigned to the person who did the review, and you can't review your own
packages.  What's happened here?

Comment 21 Hans de Goede 2010-11-22 13:53:30 UTC
(In reply to comment #20)
> For some reason this ticket is assigned to the submitter.  It needs to be
> assigned to the person who did the review, and you can't review your own
> packages.  What's happened here?

My bad, fixed now.

Comment 22 Hans de Goede 2010-11-22 13:58:13 UTC
Hmm,

Maybe this needs some more explanation. Nicolas did a very thorough review of this package as he is always keeping an eye on font packages and then set the fedora-review flag to +. But the submitter needed a sponsor, so I re-reviewed the package and sponsored Igshaan, so I'm the reviewer (as is properly shown by the assigned to field now), but the fedora-review + flag was set by Nicolas, and never changed by me, so that still has Nicolas' name on it.

Sorry about the confusion.

Regards,

Hans

Comment 23 Jason Tibbitts 2010-11-22 14:10:11 UTC
Thanks for the explanation.  We see tickets in all sorts of bizarre states, and
can't always figure out how they got that way.

Git done (by process-git-requests).

Comment 24 Igshaan Mesias 2010-11-23 12:44:03 UTC
Thanks you for all help gentlemen.

Comment 25 Hans de Goede 2011-01-11 18:32:14 UTC
This package has been import and build, closing this ticket.


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