https://bugzilla.redhat.com/show_bug.cgi?id=1124070
Bug ID: 1124070 Summary: Review Request: pcaro-hermit-fonts - Monospace fonts for programming Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: rybrown@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://rsb.io/pkg/hermit/pcaro-hermit-fonts-1.21-2.spec SRPM URL: http://rsb.io/pkg/hermit/pcaro-hermit-fonts-1.21-2.fc20.src.rpm
Description: Hermit is a monospace font designed to be clear, pragmatic and very readable. Its creation has been focused on programming. Every glyph was carefully planned and calculated, according to defined principles and rules. For this reason, Hermit is coherent and regular.
Fedora Account System Username: ryansb
Koji Builds: http://koji.fedoraproject.org/koji/taskinfo?taskID=7205300 http://koji.fedoraproject.org/koji/taskinfo?taskID=7205301
This copr repo uses the specfile from above, and I've tested it on F19, F20, and rawhide. https://copr.fedoraproject.org/coprs/ryansb/pcaro-hermit-fonts/
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
David Gay oddshocks@riseup.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |oddshocks@riseup.net
--- Comment #1 from David Gay oddshocks@riseup.net --- Tested and working on Fedora 20 x86_64. Awesome stuff! :)
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
Parag AN(पराग) panemade@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |fonts-bugs@lists.fedoraproj | |ect.org, | |i18n-bugs@lists.fedoraproje | |ct.org, panemade@gmail.com Assignee|nobody@fedoraproject.org |panemade@gmail.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
--- Comment #2 from Parag AN(पराग) panemade@gmail.com --- will review this tomorrow :)
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
--- Comment #3 from Parag AN(पराग) panemade@gmail.com --- Suggestions: 1) spec file name should not contain any version or release. I suppose its a typo in the initial comment for Spec URL.
2) If you are not planning to add this package to EPEL6 then remove following from spec
a) Group tag is now optional and can be removed.
b) "rm -rf %{buildroot}" is not needed anymore as its done automatically by rpmbuild
c) similarly %clean section is also not needed anymore
3) %prep should contain only %setup -q
4) Though there is no compilation or building of binary files happening, you can include empty %build to let rpmlint happy
5) Don't add extra empty spaces in the spec file like one you have line 59
6) I think you can if wanted keep the priority of fontconfig file to 69. Generally 60-69 numbers are used for "generic aliases, map generic->family" and you have the same fontconfig rule.
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
--- Comment #4 from Ryan Brown rybrown@redhat.com --- Parag,
Thank you for your review, I've responded inline.
1) spec file name should not contain any version or release.
The spec file name only contains a version so I can make multiple revisions available from my web site, the actual spec file on my local machine does not have a version in its filename.
2) If you are not planning to add this package to EPEL6 then remove following from spec
a) Group tag is now optional and can be removed. Done
b) "rm -rf %{buildroot}" is not needed anymore as its done automatically by rpmbuild Done
c) similarly %clean section is also not needed anymore Done
3) %prep should contain only %setup -q
For my case %setup -cq is necessary because the upstream tarball extracts files directly into current directory instead of extracting to a folder
4) Though there is no compilation or building of binary files happening, you can include empty %build to let rpmlint happy Done
5) Don't add extra empty spaces in the spec file like one you have line 59 Removed
6) I think you can if wanted keep the priority of fontconfig file to 69. Generally 60-69 numbers are used for "generic aliases, map generic->family" and you have the same fontconfig rule. Done
New SRPM: http://rsb.io/pkg/hermit/pcaro-hermit-fonts-1.21-3.fc20.src.rpm New specfile: http://rsb.io/pkg/hermit/pcaro-hermit-fonts-1.21-3.spec
Thanks very much for your feedback, let me know what you think of the changes.
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
--- Comment #5 from Parag AN(पराग) panemade@gmail.com --- Thanks. Few more changes I will suggest
1) We have fedora-review tool which reads the spec and srpm links. This tool shows issue for spec file name as it will not match to what is packaged inside srpm. You can have multiple revisions of spec along with version at your web location but keep only one latest spec with just pcaro-hermit-fonts.spec name.
2) The upstream also uses spelling as "monospace" and not "mono-space". Please change all such occurrences. Let rpmlint complain it as warning.
3) I think many are familiar with this way to fix DOS line endings http://fedoraproject.org/wiki/Packaging_tricks#Remove_DOS_line_endings
4) with your recent changelog another rpmlint warning has come. When you use character '%' in %changelog section or in any line which is a comment, always use it as twice '%%' character.
Else looks good now.
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
--- Comment #6 from Ryan Brown rybrown@redhat.com --- Ok, I didn't know that fedora-review would fail if the version was in the spec URL.
mono-space -> monospace Done
DOS line endings Done
Changelog % -> %% Done
Spec URL: http://rsb.io/pkg/hermit/pcaro-hermit-fonts.spec SRPM URL: http://rsb.io/pkg/hermit/pcaro-hermit-fonts-1.21-4.fc20.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
Parag AN(पराग) panemade@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #7 from Parag AN(पराग) panemade@gmail.com --- Thanks. Looks good now.
No. The fedora-review tool will not fail but will check such mismatch and raise it an issue.
Package is APPROVED.
Note when you request for Package SCM request, please add InitialCC as InitialCC: fonts-sig i18n-team
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
Ryan Brown rybrown@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-cvs?
--- Comment #8 from Ryan Brown rybrown@redhat.com --- New Package SCM Request ======================= Package Name: pcaro-hermit-fonts Short Description: Hermit monospace fonts Upstream URL: https://pcaro.es/p/hermit Owners: ryansb Branches: f20 f21 master InitialCC: fonts-sig i18n-team
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-cvs? |fedora-cvs+
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
--- Comment #9 from Jon Ciesla limburgher@gmail.com --- Git done (by process-git-requests).
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
Ryan Brown rybrown@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |NEXTRELEASE Last Closed| |2014-07-31 12:43:23
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
--- Comment #10 from Fedora Update System updates@fedoraproject.org --- pcaro-hermit-fonts-1.21-4.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/pcaro-hermit-fonts-1.21-4.fc20
https://bugzilla.redhat.com/show_bug.cgi?id=1124070
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version| |pcaro-hermit-fonts-1.21-4.f | |c20 Resolution|NEXTRELEASE |ERRATA
--- Comment #11 from Fedora Update System updates@fedoraproject.org --- pcaro-hermit-fonts-1.21-4.fc20 has been pushed to the Fedora 20 stable repository.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla