Bug 533919

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

Created attachment 374418 [details]
repo-font-audit data for this package

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

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 :(

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

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

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

Created attachment 395525 [details]
repo-font-audit data for this package

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?

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

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

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.

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

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

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.

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

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.

(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.

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

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?

(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.

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

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).

Thanks you for all help gentlemen.

This package has been import and build, closing this ticket.