Bug 1015701 - (Amiri) Review Request: amiri-fonts - A classical Arabic font in Naskh style
Review Request: amiri-fonts - A classical Arabic font in Naskh style
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On: 1021164
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-04 17:15 EDT by Mosaab Alzoubi
Modified: 2014-10-20 07:48 EDT (History)
8 users (show)

See Also:
Fixed In Version: amiri-fonts-0.106-9.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-11-15 13:56:52 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Mosaab Alzoubi 2013-10-04 17:15:40 EDT
Spec URL: http://helallinux.com/paste/show.php?id=1225
SRPM URL: http://sourceforge.net/projects/oji/files/srpms/amiri-fonts-0.106-1.fc19.src.rpm
Description: 
مجموعة الخطوط الأميرية ذات المظهر الأنيق و التّراث العتيق .
Amiri font is an open font revival of the Arabic Naskh typeface
designed and first used by Bulaq Press in Cairo (Maṭbaʿtu Būlāq, also known as
Āl-Maṭbaʿtu Āl-Āmīriīaʰ) in the early 2th century.  Amiri's uniqueness comes
from its superb balance between the beauty of Naskh calligraphy and the
requirements of elegant typography.  Amiri is most suitable for running text
and book printing.

Fedora Account System Username: moceap
Comment 1 Mosaab Alzoubi 2013-10-06 07:20:31 EDT
Spec URL: http://helallinux.com/paste/show.php?id=1225&mode=raw
SRPM URL: http://downloads.sourceforge.net/projects/oji/srpms/amiri-fonts-0.106-1.fc19.src.rpm
Description: 
مجموعة الخطوط الأميرية ذات المظهر الأنيق و التّراث العتيق .
Amiri font is an open font revival of the Arabic Naskh typeface
designed and first used by Bulaq Press in Cairo (Maṭbaʿtu Būlāq, also known as
Āl-Maṭbaʿtu Āl-Āmīriīaʰ) in the early 2th century.  Amiri's uniqueness comes
from its superb balance between the beauty of Naskh calligraphy and the
requirements of elegant typography.  Amiri is most suitable for running text
and book printing.

Fedora Account System Username: moceap
Comment 2 Mosaab Alzoubi 2013-10-10 04:42:14 EDT
Hosted at Ojuba server , Alot of fixes after many packages revision :)

Spec : http://ojuba.org/oji/SPECS/amiri-fonts.spec
SRPM : http://ojuba.org/oji/SRPMS/amiri-fonts-0.106-2.oji.fc19.src.rpm
Comment 3 Zbigniew Jędrzejewski-Szmek 2013-10-17 15:32:37 EDT
This is pretty far from the guidelines... See https://fedoraproject.org/wiki/Packaging:FontsPolicy.
Comment 4 Mosaab Alzoubi 2013-10-18 15:25:31 EDT
Ok , rewiting in progress >>>
Comment 5 Mosaab Alzoubi 2013-10-19 17:37:23 EDT
All Done, Rewritten almost from Zero ::

Spec : http://ojuba.org/oji/SPECS/amiri-fonts.spec
SRPM : http://ojuba.org/oji/SRPMS/amiri-fonts-0.106-3.oji.fc19.src.rpm
Comment 6 Zbigniew Jędrzejewski-Szmek 2013-10-19 18:47:07 EDT
Some high-level problems:

1. Guidelines say that fonts must be built from sources, if those are provided. Here they are provided.

2. -web fonts are the same as in the main package with just some metainformation removed to save space. I don't think we normally package that. I think it is normal to just put .woff and other formats in the same -fonts package.

3. Some of your descriptions in Arabic are not the same as in English, afaict. This is supposed to be a translation, not a separate text.

4. -common just contains some documentation. It would be better to call it -docs.
Also, -fonts packages should not depend on it. It is enough to include a copy of the license file in the -fonts packages.

5. License file is not packaged.

6. There's no need to remove the buildroot.

7. You can drop the empty %files sections and the two "meta" packages. What are they useful for?
Comment 7 Mosaab Alzoubi 2013-10-19 19:22:58 EDT
Thanx for reply.

1 - When I build it from source directly, nothing done because the destination of built fonts already busy, Also building of the font requirs gpp command I couldn't find it in Fedora repos.

2 - Did you mean to renaming files of -web?

3 - Arabic and English texts isn't matching 100% , but as whole they serve one idea.

4 - I taked (common) name from complex formal font spec, and I doesn't have problems to call it (docs), also the requires line.

5 - Ok ^_^ , (License alredy included in ttf files ^_^),

I'll package it :)

6 - OK

7 - OK

--------------

Thank you Zbigniew , this is longest spec I wrote ever, so I wrote it slowly.
Comment 8 Mosaab Alzoubi 2013-10-19 19:30:43 EDT
For point 7 , I think (amiri-fonts) meta package make download of the fonts as group more easy .

I think this way must be formal in Fedora ^_^
Comment 10 Zbigniew Jędrzejewski-Szmek 2013-10-19 22:44:27 EDT
(In reply to Mosaab Alzoubi from comment #7)
> Thanx for reply.
> 
> 1 - When I build it from source directly, nothing done because the
> destination of built fonts already busy, Also building of the font requirs
> gpp command I couldn't find it in Fedora repos.
You can do 'make clean' to get rid of the generated files.

gpp is now at #1021164.

> 2 - Did you mean to renaming files of -web?
No, I meant to remove (not install) the files in -web.

> 3 - Arabic and English texts isn't matching 100% , but as whole they serve
> one idea.
The Arabic should be a translation of the English one...

> 4 - I taked (common) name from complex formal font spec, and I doesn't have
> problems to call it (docs), also the requires line.
The example from the wiki doesn't really fit here, so some adjustments must be made.

> 5 - Ok ^_^ , (License alredy included in ttf files ^_^),
> 
> I'll package it :)
> 
> 6 - OK
> 
> 7 - OK
> 
> --------------
> 
> Thank you Zbigniew , this is longest spec I wrote ever, so I wrote it slowly.
Current spec looks nice. Let's see if is viable to build from source.
Comment 11 Zbigniew Jędrzejewski-Szmek 2013-10-22 01:26:49 EDT
sfntly has been packaged: #912686, I think it is the same thing as sfnttool.
gpp is in #1021164.
fntsample, pdfoutline, latexmk are available.

So I think it is viable to build from source.

I'm not a sponsor, and I cannot sponsor you into the packager group. You'll have to find someone to do that. It would certainly be nice to polish this package so that it satisfies all guidelines, I'll by happy to help by doing further (informal) reviews of this package. It also *does* help if you do informal reviews of other packages, before you are sponsored.
Comment 12 Parag AN(पराग) 2013-10-22 01:42:53 EDT
I can sponsor the reporter provided he will show package reviews done on other package review bugs where he can show he has helped in finding issues and providing fix for them.
Comment 13 Mosaab Alzoubi 2013-10-22 19:04:09 EDT
Zbigniew ::
This font couldn't built by FontForge, It built by fork of it found here:
https://bitbucket.org/sortsmill/sortsmill-tools
That what Khaled Hosny (maintainer of Amiri complex font) said.

So I tried to build sortsmill, but it need gnulib to build. I surprised that gnulib isn't found in Fedora repo !!

So I worked hard to package it #1022283

-----------

Parag ::
Thank you for your attention, I did 7 package reveiw bugs ::

https://bugzilla.redhat.com/buglist.cgi?component=Package%20Review&email1=moceap%40hotmail.com&emailreporter1=1&emailtype1=substring&list_id=1829241&query_format=advanced

GnuLib is the last one I did to package sortsmill to package Amiri fonts :)

Really I want your sponsor to push these packages (and others) into Fedora.

-----------

Kind Regards
Mosaab
Comment 14 Parag AN(पराग) 2013-10-23 03:52:49 EDT
Thanks for submitting 7 packages for review but you need to do package review of other people's submitted packages reviews.
Comment 15 Mosaab Alzoubi 2013-10-23 06:41:09 EDT
Ok . I read 4 package review new bugs and wrote my notes:

#1021721 #1022407 #1021749

Thank You again Parag.
Comment 16 Zbigniew Jędrzejewski-Szmek 2013-10-23 16:08:47 EDT
(Strong suggestion:) Please install fedora-review, and use it on your *own* review requests. It will point out obvious things, letting the reviewers concentrate on subtler things and reduce the review time.

Also, use it on *other* people's review requests, even pasting the whole review.txt with your own comments notes into comments on the tickets. This will show your sponsor that you can review other packages.
Comment 17 Parag AN(पराग) 2013-10-25 03:01:26 EDT
Mosaab,
  Do some full reviews of new packages not just one or two comments. I will be happy to sponsor then.
Comment 18 Mosaab Alzoubi 2013-10-25 03:26:50 EDT
Ok I'll search. These was latest 3 new review bugs !!
Comment 19 Mosaab Alzoubi 2013-10-26 18:47:46 EDT
First full-reviesion : bug 1023619
Comment 20 Zbigniew Jędrzejewski-Szmek 2013-10-27 12:43:53 EDT
I learnt in another review that .woffs are not supposed to be packaged: http://fedoraproject.org/wiki/Packaging:Web_Assets#Fonts.
Comment 21 Parag AN(पराग) 2013-10-27 13:14:31 EDT
That's right. Whichever format upstream is providing fonts, try to package ttf or otf only if available that format.

For fonts packages its recommended to create a font package information on Fedora wiki page with In_progress category page which when font package review gets approved moved to packaged font category.
See https://fedoraproject.org/wiki/Category:In-progress_fonts
Comment 22 Mosaab Alzoubi 2013-10-27 19:48:06 EDT
* Well, I'll remove web fonts.
* Font already now at the wiki.
* CC Amiri Maintainer.
Comment 23 Mosaab Alzoubi 2013-10-27 20:59:05 EDT
- Dropped .woff fonts.
- Update description by official one.
- Make this package ready for building if it possible later.

Spec : http://ojuba.org/oji/SPECS/amiri-fonts.spec
SRPM : http://ojuba.org/oji/SRPMS/amiri-fonts-0.106-5.oji.fc19.src.rpm
Comment 24 Zbigniew Jędrzejewski-Szmek 2013-10-27 21:35:46 EDT
Oh, one more issue:
1. change %define to %global
Comment 26 Zbigniew Jędrzejewski-Szmek 2013-10-28 18:33:07 EDT
Informal review:

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
OFL.

[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 2 files have unknown license. Detailed output of
     licensecheck in /home/zbyszek/fedora/1015701-amiri-fonts/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 460800 bytes in 10 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: No rpmlint messages.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in amiri-
     fonts-docs , amiri-quran-fonts , amiri-naskh-fonts
Not needed.

[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: amiri-fonts-docs-0.106-6.fc19.noarch.rpm
          amiri-quran-fonts-0.106-6.fc19.noarch.rpm
          amiri-naskh-fonts-0.106-6.fc19.noarch.rpm
          amiri-fonts-0.106-6.fc19.src.rpm
amiri-fonts-docs.noarch: I: enchant-dictionary-not-found ar
4 packages and 0 specfiles checked; 0 errors, 0 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint amiri-naskh-fonts amiri-fonts-docs amiri-quran-fonts
amiri-naskh-fonts.noarch: I: enchant-dictionary-not-found ar
3 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'



Requires
--------
amiri-naskh-fonts (rpmlib, GLIBC filtered):
    /bin/sh
    config(amiri-naskh-fonts)
    fontpackages-filesystem

amiri-fonts-docs (rpmlib, GLIBC filtered):

amiri-quran-fonts (rpmlib, GLIBC filtered):
    /bin/sh
    config(amiri-quran-fonts)
    fontpackages-filesystem



Provides
--------
amiri-naskh-fonts:
    amiri-naskh-fonts
    config(amiri-naskh-fonts)
    font(:lang=aa)
    font(:lang=af)
    font(:lang=an)
    font(:lang=ar)
    font(:lang=ay)
    font(:lang=az-ir)
    font(:lang=bi)
    font(:lang=br)
    font(:lang=bs)
    font(:lang=ca)
    font(:lang=ch)
    font(:lang=co)
    font(:lang=crh)
    font(:lang=cs)
    font(:lang=csb)
    font(:lang=cy)
    font(:lang=da)
    font(:lang=de)
    font(:lang=en)
    font(:lang=eo)
    font(:lang=es)
    font(:lang=et)
    font(:lang=eu)
    font(:lang=fa)
    font(:lang=fi)
    font(:lang=fil)
    font(:lang=fj)
    font(:lang=fo)
    font(:lang=fr)
    font(:lang=fur)
    font(:lang=fy)
    font(:lang=ga)
    font(:lang=gd)
    font(:lang=gl)
    font(:lang=gv)
    font(:lang=haw)
    font(:lang=ho)
    font(:lang=hr)
    font(:lang=hsb)
    font(:lang=ht)
    font(:lang=hu)
    font(:lang=ia)
    font(:lang=id)
    font(:lang=ie)
    font(:lang=io)
    font(:lang=is)
    font(:lang=it)
    font(:lang=jv)
    font(:lang=ki)
    font(:lang=kj)
    font(:lang=kl)
    font(:lang=ks)
    font(:lang=ku-iq)
    font(:lang=ku-ir)
    font(:lang=ku-tr)
    font(:lang=kwm)
    font(:lang=la)
    font(:lang=lah)
    font(:lang=lb)
    font(:lang=lg)
    font(:lang=li)
    font(:lang=lt)
    font(:lang=lv)
    font(:lang=mg)
    font(:lang=mh)
    font(:lang=ms)
    font(:lang=mt)
    font(:lang=na)
    font(:lang=nb)
    font(:lang=nds)
    font(:lang=ng)
    font(:lang=nl)
    font(:lang=nn)
    font(:lang=no)
    font(:lang=nr)
    font(:lang=nso)
    font(:lang=ny)
    font(:lang=oc)
    font(:lang=om)
    font(:lang=ota)
    font(:lang=pa-pk)
    font(:lang=pap-an)
    font(:lang=pap-aw)
    font(:lang=pl)
    font(:lang=ps-af)
    font(:lang=ps-pk)
    font(:lang=pt)
    font(:lang=rm)
    font(:lang=rn)
    font(:lang=rw)
    font(:lang=sc)
    font(:lang=sd)
    font(:lang=se)
    font(:lang=sg)
    font(:lang=sk)
    font(:lang=sl)
    font(:lang=sm)
    font(:lang=sma)
    font(:lang=smj)
    font(:lang=smn)
    font(:lang=sn)
    font(:lang=so)
    font(:lang=sq)
    font(:lang=ss)
    font(:lang=st)
    font(:lang=su)
    font(:lang=sv)
    font(:lang=sw)
    font(:lang=tk)
    font(:lang=tl)
    font(:lang=tn)
    font(:lang=to)
    font(:lang=tr)
    font(:lang=ts)
    font(:lang=ty)
    font(:lang=ug)
    font(:lang=ur)
    font(:lang=uz)
    font(:lang=vo)
    font(:lang=vot)
    font(:lang=wa)
    font(:lang=wen)
    font(:lang=wo)
    font(:lang=xh)
    font(:lang=yap)
    font(:lang=za)
    font(:lang=zu)
    font(amiri)

amiri-fonts-docs:
    amiri-fonts-docs

amiri-quran-fonts:
    amiri-quran-fonts
    config(amiri-quran-fonts)
    font(:lang=ar)
    font(amiriquran)



Source checksums
----------------
http://sourceforge.net/projects/amiri/files/amiri-0.106.tar.bz2 :
  CHECKSUM(SHA256) this package     : 4d7ba7dff5cfcaf2b8d93c6afc5874c8048e4d600d698aa911f7181723d4bc2
  CHECKSUM(SHA256) upstream package : 4d7ba7dff5cfcaf2b8d93c6afc5874c8048e4d600d698aa911f7181723d4bc2

Looks good.
Comment 27 Zbigniew Jędrzejewski-Szmek 2013-10-28 18:36:50 EDT
In my opinion, package is good. Un-assigning, since I'm not a sponsor.

Note, link to wiki: https://fedoraproject.org/wiki/Amiri_fonts
Comment 28 Parag AN(पराग) 2013-11-04 00:53:45 EST
I see that you have started reviewing other new packages. Please do 2 more full reviews and show that you understand packaging and can find issues in packages and can provide fixes to them.

I am taking this for official review but will do so once I see your reviews done on other packages.
Comment 29 Mosaab Alzoubi 2013-11-05 20:09:41 EST
Thank you Parag for your attention, Done: #1025601 #1025052
Comment 30 Parag AN(पराग) 2013-11-09 10:00:09 EST
Thanks. I will resume this review on Monday and will sponsor you.
Comment 31 Parag AN(पराग) 2013-11-11 00:39:06 EST
suggestions:
1) your source download link should be
 http://downloads.sourceforge.net/project/amiri/%{fontname}-%{version}.tar.bz2

2) good to always truncate your long lines with 80 characters per line
# Sorts Mill tools (and others) which necessary to rebuild Amiri fonts isn't
# released in Fedora yet. Enable the disabled-lines when rebuilding of Amiri
# fonts possible.

but I will suggest you better clean the spec and let not include comments and sorts mill tools information.

We have many font packages which are not built from source so not a hard requirement. Just install binary files for now.

3) your fontconfig files should look neat as
a)amiri-fontconfig.conf
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE fontconfig SYSTEM "../fonts.dtd">
<fontconfig>
        <alias>
          <family>sans-serif</family>
            <prefer>
              <family>Amiri</family>
            </prefer>
        </alias>
        <alias>
          <family>Amiri</family>
            <default>
              <family>sans-serif</family>
            </default>
        </alias>
</fontconfig>

b)amiri-quran-fontconfig.conf
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE fontconfig SYSTEM "../fonts.dtd">
<fontconfig>
        <alias>
          <family>sans-serif</family>
            <prefer>
              <family>Amiri Quran</family>
            </prefer>
        </alias>
        <alias>
          <family>Amiri Quran</family>
            <default>
              <family>sans-serif</family>
            </default>
        </alias>
</fontconfig>

4) font package name should be [foundryname-]projectname[-fontfamilyname]-fonts, in lowercase. 

so you should install amiri-boldslanted.ttf, amiri-bold.ttf, amiri-regular.ttf,
amiri-slanted.ttf from main amiri-fonts package
and
amiri-quran.ttf font from amiri-quran-fonts package

5) I don't think we need this font to be default Arabic font in Fedora. I will suggest to use 67 or 68 fontconfig priority like other fonts are using. Good to keep 67 priority as we have one example as 67-paktype-naskh-basic.conf

6) you should add -common package and not -doc package. Also, all other documentation files to -common package.

7) then, this -common be added to amiri-fonts and amiri-quran-fonts as
Requires: amiri-fonts-common

Feel free to ask me if you got any doubts in above.

Provide new srpm that fixes above issues.
Comment 32 Khaled Hosny 2013-11-11 02:50:37 EST
(In reply to Parag AN(पराग) from comment #31) 
> 3) your fontconfig files should look neat as
> a)amiri-fontconfig.conf
> <?xml version="1.0" encoding="UTF-8"?>
> <!DOCTYPE fontconfig SYSTEM "../fonts.dtd">
> <fontconfig>
>         <alias>
>           <family>sans-serif</family>
>             <prefer>
>               <family>Amiri</family>
>             </prefer>
>         </alias>
>         <alias>
>           <family>Amiri</family>
>             <default>
>               <family>sans-serif</family>
>             </default>
>         </alias>
> </fontconfig>

Amiri is a serif, not a sans-serif font.

> b)amiri-quran-fontconfig.conf
> <?xml version="1.0" encoding="UTF-8"?>
> <!DOCTYPE fontconfig SYSTEM "../fonts.dtd">
> <fontconfig>
>         <alias>
>           <family>sans-serif</family>
>             <prefer>
>               <family>Amiri Quran</family>
>             </prefer>
>         </alias>
>         <alias>
>           <family>Amiri Quran</family>
>             <default>
>               <family>sans-serif</family>
>             </default>
>         </alias>
> </fontconfig>

Amiri Quran is a special font for typesetting Quran (it has a very high line hight, only support characters used in Quran, some typographic choices not generally suitable for regular text), so I don’t think such an alias is a good idea.
Comment 33 Parag AN(पराग) 2013-11-11 03:23:32 EST
Ouch! you are right. I forgot to change it to serif as it was just copy paste from some other fontconfig file.

a)amiri-fontconfig.conf
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE fontconfig SYSTEM "../fonts.dtd">
<fontconfig>
        <alias>
          <family>serif</family>
            <prefer>
              <family>Amiri</family>
            </prefer>
        </alias>
        <alias>
          <family>Amiri</family>
            <default>
              <family>serif</family>
            </default>
        </alias>
</fontconfig>

b) Khaled you suggest not to include any fontconfig file for Amiri Quran font?
Comment 34 Khaled Hosny 2013-11-11 03:32:12 EST
(In reply to Parag AN(पराग) from comment #33)
> b) Khaled you suggest not to include any fontconfig file for Amiri Quran
> font?

I think it is better not to provide one.
Comment 35 Mosaab Alzoubi 2013-11-11 04:25:14 EST
Thank You for attention (and for invitation) Parag
Thank You for your notes Khaled (maintainer of Amiri)

- Fix Sourceforg link in Source0.
- Decrease instructions to rebuild Amiri from the source.
- Replace -docs by -common.
- Change font priority to 67.
- Improve font config.
- The fonts in one family so it united into 1 main package instead of 2.
- -common to be main package require.

-------

Spec : http://ojuba.org/oji/SPECS/amiri-fonts.spec
SRPM : http://ojuba.org/oji/SRPMS/amiri-fonts-0.106-7.oji.fc19.src.rpm
Comment 36 Parag AN(पराग) 2013-11-11 04:38:44 EST
1) You need subpackage for Amiri Quran. Don't remove it. We add fonts (sub-)packages per family. Here, Amiri Quran is one family for amiri-quran.ttf and all other fonts under family name Amiri.

so as said before, amiri-fonts installs  amiri-boldslanted.ttf, amiri-bold.ttf, amiri-regular.ttf, amiri-slanted.ttf 
And
amiri-quran-fonts installs  amiri-quran.ttf

and then -common is needed by amiri-quran-fonts subpackage also.

2) you can remove following 
%doc 
and instead make all docs files be owned/installed by -common subpackage

%files common
%doc documentation/*  OFL.txt OFL-FAQ.txt
Comment 37 Mosaab Alzoubi 2013-11-11 05:14:23 EST
Then, is amiri-quran-fontconfig.conf still needed?
Comment 38 Parag AN(पराग) 2013-11-11 06:10:56 EST
generally we add fontconfig files per family per subpackage. I think if font is not supposed to be used as a desktop font but only for writing text then we can drop fontconfig file.

Nicolas,
  If you are reading this package review emails then we need your input here.
Comment 39 Nicolas Mailhot 2013-11-11 09:06:38 EST
(In reply to Parag AN(पराग) from comment #38)
> generally we add fontconfig files per family per subpackage. I think if font
> is not supposed to be used as a desktop font but only for writing text then
> we can drop fontconfig file.
> 
> Nicolas,
>   If you are reading this package review emails then we need your input here.

The guidelines intent is to treat all fonts the same and not assume users will only use them in a particular way. Packages have a long life and it's always worthwhile not to restrict hteir use beforehand.
Comment 40 Parag AN(पराग) 2013-11-11 10:45:28 EST
Mosaab,

1)Add the fontconfig amiri-quran-fonts.conf file and submit new srpm for further review. Note this is also serif font so add accordingly in fontconfig file.

2) I saw your recent spec. You should write -common as

--------------------------------------------------------------------
%package common
Summary:        Common files for %{name}
Requires:       fontpackages-filesystem

%description common
%common_desc

This package consists of files used by other %{name} packages.
--------------------------------------------------------------------

3) your main package should drop
Requires:       fontpackages-filesystem
as this is already added in -common

4) I tried to find some recent examples for font packages but could not find spec as per updated guidelines but here are some that you should see.
http://pkgs.fedoraproject.org/cgit/ecolier-court-fonts.git/plain/ecolier-court-fonts.spec
http://pkgs.fedoraproject.org/cgit/sj-fonts.git/plain/sj-fonts.spec
http://pkgs.fedoraproject.org/cgit/linux-libertine-fonts.git/plain/linux-libertine-fonts.spec
Comment 41 Mosaab Alzoubi 2013-11-11 12:21:20 EST
- Re-split into main and Quran fonts.
- Improve Amiri Quran font config.
- Add license files to -common, dropped from others.
- Drop fontpackages-filesystem requires from main package.

--------------


Spec : http://ojuba.org/oji/SPECS/amiri-fonts.spec
SRPM : http://ojuba.org/oji/SRPMS/amiri-fonts-0.106-8.oji.fc19.src.rpm
Comment 42 Khaled Hosny 2013-11-11 13:04:55 EST
(In reply to Nicolas Mailhot from comment #39)
> (In reply to Parag AN(पराग) from comment #38)
> > generally we add fontconfig files per family per subpackage. I think if font
> > is not supposed to be used as a desktop font but only for writing text then
> > we can drop fontconfig file.
> > 
> > Nicolas,
> >   If you are reading this package review emails then we need your input here.
> 
> The guidelines intent is to treat all fonts the same and not assume users
> will only use them in a particular way. Packages have a long life and it's
> always worthwhile not to restrict hteir use beforehand.

I’m not sure I follow here, but Amiri Quran is just a modified version of Amiri with reduced character set and defaults suitable only for typesetting Quran.
Comment 43 Parag AN(पराग) 2013-11-11 23:34:49 EST
Though Amiri font is not yet in Fedora and Amiri Quran is modified version of Amiri font, Let's take reference of https://fedoraproject.org/wiki/Shipping_fonts_in_Fedora_%28FAQ%29#What_if_my_package_bundles_a_modified_version_of_an_existing_Fedora_font.3F

Then the last submitted srpm is ready for final review.
Comment 44 Parag AN(पराग) 2013-11-13 10:44:28 EST
I had a look at the last submitted srpm and looks good except summary which should be =>
A classical Arabic font in Naskh style

and ?? in spec should be changed to 67

I have also sponsored you. Make sure you follow current guidelines while doing any package review or writing spec files. When in doubt email me or ask on fedora devel mailing list for any required help.

submit another srpm fixing above issues and I will approve this package.
Comment 45 Mosaab Alzoubi 2013-11-13 12:21:58 EST
- Change variable font priority to 67 in font_pkg line.
- Reform Summary.

-----------

Spec : http://ojuba.org/oji/SPECS/amiri-fonts.spec
SRPM : http://ojuba.org/oji/SRPMS/amiri-fonts-0.106-9.oji.fc19.src.rpm
Comment 46 Mosaab Alzoubi 2013-11-13 12:22:55 EST
Thanks alot for sponsor :)
Comment 47 Parag AN(पराग) 2013-11-13 12:34:33 EST
Thanks. 
Package is APPROVED.

Please go through now step 7 onwards as given on http://fedoraproject.org/wiki/New_package_process_for_existing_contributors
Comment 48 Mosaab Alzoubi 2013-11-13 13:34:19 EST
Thank You

New Package SCM Request
=======================
Package Name: amiri-fonts
Short Description: A classical Arabic font in Naskh style
Owners: moceap
Branches: f18 f19 f20 el5 el6
InitialCC:
Comment 49 Jon Ciesla 2013-11-13 13:58:51 EST
Git done (by process-git-requests).
Comment 50 Fedora Update System 2013-11-13 16:47:00 EST
amiri-fonts-0.106-9.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/amiri-fonts-0.106-9.fc20
Comment 51 Fedora Update System 2013-11-13 16:54:57 EST
amiri-fonts-0.106-9.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/amiri-fonts-0.106-9.fc19
Comment 52 Fedora Update System 2013-11-13 16:56:35 EST
amiri-fonts-0.106-9.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/amiri-fonts-0.106-9.fc18
Comment 53 Fedora Update System 2013-11-13 17:08:12 EST
amiri-fonts-0.106-9.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/amiri-fonts-0.106-9.el6
Comment 54 Fedora Update System 2013-11-14 14:23:33 EST
amiri-fonts-0.106-9.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 55 Fedora Update System 2013-11-15 13:56:52 EST
amiri-fonts-0.106-9.el6 has been pushed to the Fedora EPEL 6 stable repository.
Comment 56 Fedora Update System 2013-11-15 15:32:12 EST
amiri-fonts-0.106-9.fc18 has been pushed to the Fedora 18 stable repository.
Comment 57 Fedora Update System 2013-11-15 15:34:56 EST
amiri-fonts-0.106-9.fc19 has been pushed to the Fedora 19 stable repository.
Comment 58 Fedora Update System 2013-11-16 02:07:24 EST
amiri-fonts-0.106-9.fc20 has been pushed to the Fedora 20 stable repository.
Comment 59 Mosaab Alzoubi 2014-09-16 19:16:21 EDT
Package Change Request
======================
Package Name: amiri-fonts
New Branches: epel7
Owners: moceap
Comment 60 Jon Ciesla 2014-10-20 07:48:02 EDT
Git done (by process-git-requests).

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