[Bug 487913] Review Request: culmus-fancy-fonts - Fancy fonts for Hebrew

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=487913


Nicolas Mailhot <nicolas.mailhot at laposte.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |fedora-fonts-bugs-list at redh
                   |                            |at.com
             Blocks|                            |477387
         Depends on|                            |173897
         AssignedTo|nobody at fedoraproject.org    |nicolas.mailhot at laposte.net
               Flag|                            |fedora-review?,
                   |                            |needinfo?(danken at cs.technio
                   |                            |n.ac.il)




--- Comment #2 from Nicolas Mailhot <nicolas.mailhot at laposte.net>  2009-03-02 18:43:20 EDT ---
First review pass (builds in mock, good)

1. various rpmlint warnings, of which only the following need to be fixed
culmus-fancy-fonts.src: E: invalid-spec-name

culmus-fancy-fonts.src:84: E: files-attr-not-set
A file or a directory entry in a %files section does not have attributes set
which may result in security issues in the resulting binary package depending
on the system where the package is built.  Add default attributes using
%defattr before it in the %files section, or use per line %attr's.

culmus-fancy-fonts.src:157: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

culmus-fancy-fonts.src:158: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

culmus-fancy-fonts.src:159: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

culmus-fancy-fonts.src:160: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

culmus-fancy-fonts.src:161: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

culmus-fancy-fonts.src:162: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

culmus-fancy-fonts.src:163: W: setup-not-quiet
Use the -q option to the %setup macro to avoid useless build output from
unpacking the sources.

culmus-fancy-fonts.src: W: no-%build-section
The spec file does not contain a %build section.  Even if some packages don't
directly need it, section markers may be overridden in rpm's configuration to
provide additional "under the hood" functionality, such as injection of
automatic -debuginfo subpackages.  Add the section, even if empty.

fonts-hebrew-fancy-compat.noarch: W: summary-ended-with-dot Compatibility files
of Culmus fancy font families.
Summary ends with a dot.

2. please use OTF over Type1 whenever possible

3. 
 a. we prefer for fonts released in different archives to be packaged
separately (different rpms and srpms).
 b. also, I don't think Legal would appreciate the way you drop every licensing
file but one.
 c. lastly, the different fonts actually have different timestamps so your
version is misleading

However there is a tolerance for fonts that used to be packaged in a single
srpm so you may avail yourself of it if you really want to. Still, I don't
think that's a good idea. 7 simple packages can be easier to manage than a
monster one (and are actually quicker to review)

4. It would be a good idea to contact upstream and make it add the FSF font
exception to their licensing
http://fedoraproject.org/wiki/Legal_considerations_for_fonts#Good_font_licenses_allow_embedding

5. the rpm in rawhide allows you to drop the duplicate Group declarations in
subpackages

6. FPC and FESCO have decided %define-s should be replaced by %global-s (cf
fontpackages-devel 1.20)

7. You do not need this
Obsoletes:         %{fontname}-fonts-common < %{version}-%{release}
Obsoletes:         %{fontname}-comix-no2-fonts < %{version}-%{release}
Obsoletes:         %{fontname}-dorian-fonts < %{version}-%{release}
Obsoletes:         %{fontname}-gan-fonts < %{version}-%{release}
Obsoletes:         %{fontname}-gladia-fonts < %{version}-%{release}
Obsoletes:         %{fontname}-ktav-yad-fonts < %{version}-%{release}
Obsoletes:         %{fontname}-ozrad-fonts < %{version}-%{release}
Obsoletes:         %{fontname}-anka-fonts < %{version}-%{release}

8. You should not need this
Provides:          fonts-hebrew-fancy = %{version}-%{release}

9. Please only obsolete the last version of fonts-hebrew-fancy  built in koji +
1
https://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages

10. Your compat subpackage need not require common, it'll be pulled in by the
others

11. You're supposed to add something specific to each font subpackage under 
%common_desc references. Likewise your summaries are all identical, that's not
user-friendly

12. You do not need the 
%dir %{_fontdir}
line anymore

13. since your fontconfig files define substitution rules, it's probably a good
idea to use
/usr/share/fontconfig/templates/substitution-font-template.conf
as template

14. the spec is slightly easier to review if you keep the same line order as
the template so diffs are minimal

⇒ need a little more work

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.