Bug 1187337 - Review Request: oflb-coval-fonts - Derivation of other free of charge fonts
Summary: Review Request: oflb-coval-fonts - Derivation of other free of charge fonts
Keywords:
Status: CLOSED DUPLICATE of bug 1198246
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-01-29 19:25 UTC by Carlos Morel-Riquelme
Modified: 2015-07-21 12:58 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2015-03-03 16:57:34 UTC
Type: ---
Embargoed:
panemade: fedora-review?


Attachments (Terms of Use)

Description Carlos Morel-Riquelme 2015-01-29 19:25:08 UTC
Spec URL: https://empateinfinito.fedorapeople.org/font/sil-coval-fonts/sil-coval-fonts.spec

SRPM URL: https://empateinfinito.fedorapeople.org/font/sil-coval-fonts/sil-coval-fonts-0.1-1.fc21.src.rpm

Fedora-review: https://empateinfinito.fedorapeople.org/font/sil-coval-fonts/review.txt


Description: Coval font is a derivated of sans serif with OFL license
Fedora Account System Username: empateinfinito

FE-NEEDSPONSOR

Comment 1 Parag AN(पराग) 2015-01-31 14:22:58 UTC
Hi Carlos,
   We have this process http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group to get sponsored into the packager group. Can you either submit few more packages and/or some full detailed package reviews using fedora-review tool? This is needed to make sure package submitter understands the rpm packaging well and follows the fedora packaging guidelines.

Please go through the following links
1) http://fedoraproject.org/wiki/Package_Review_Process

2) https://fedoraproject.org/wiki/PackagingGuidelines

3) To find the packages already submitted for review, check http://fedoraproject.org/PackageReviewStatus/

4) http://fedoraproject.org/wiki/Packaging:ReviewGuidelines is useful while doing package reviews.

5) https://fedorahosted.org/FedoraReview/ this is fedora-review tool to help review packages in fedora. You need to use this and do un-official package reviews of packages submitted by other contributors. While doing so mention "This is un-official review of the package." at top of your review comment.

More about reviewer process is given at http://fedoraproject.org/wiki/Package_Review_Process#Reviewer

Good to review packages listed in http://fedoraproject.org/PackageReviewStatus/NEW.html


If you got any questions please ask :)

Comment 2 Carlos Morel-Riquelme 2015-01-31 18:40:05 UTC
Thanks for the reply Parag, well i read all guidelines and follow your recommendations, about fedora review tool when i run 
fedora-review --mock-config fedora-rawhide-x86_64 -n sil-coval-fonts

i have the error mock 127, this is reported here https://bugzilla.redhat.com/show_bug.cgi?id=1185565 , but the review finish fine. here are some output.

review.txt

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

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



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

Generic:
[ ]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[ ]: 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.
[ ]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. No licenses
     found. Please check the source files for licenses manually.
[ ]: Package contains no bundled libraries without FPC exception.
[ ]: Changelog in prescribed format.
[ ]: Sources contain only permissible code or content.
[ ]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[ ]: Development files must be in a -devel package
[ ]: Package uses nothing in %doc for runtime.
[ ]: Package consistently uses macros (instead of hard-coded directory names).
[ ]: Package is named according to the Package Naming Guidelines.
[ ]: Package does not generate any conflict.
[ ]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: Requires correct, justified where necessary.
[ ]: Spec file is legible and written in American English.
[ ]: Package is not known to require an ExcludeArch tag.
     Note: Test run failed
[ ]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Test run failed
[ ]: Packages must not store files under /srv, /opt or /usr/local
     Note: Test run failed
[ ]: 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]: %config files are marked noreplace or the reason is justified.
[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]: No %config files under /usr.
[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.

===== 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.
[ ]: Final provides and requires are sane (see attachments).
[ ]: Package functions as described.
[ ]: Latest version is packaged.
[ ]: Package does not include license text files separate from upstream.
[ ]: Scriptlets must be sane, if used.
[ ]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
[ ]: 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]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

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

fonts:
[ ]: This text should never be shown.
     Note: Test run failed
[ ]: Run repo-font-audit on all fonts in package.
     Note: Test run failed
[ ]: Run ttname on all fonts in package.
     Note: Test run failed

Rpmlint
-------
Checking: sil-coval-fonts-0.1-1.fc22.noarch.rpm
          sil-coval-fonts-0.1-1.fc22.src.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:

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

Provides
--------
sil-coval-fonts:
    config(sil-coval-fonts)
    font(:lang=aa)
    font(:lang=ab)
    font(:lang=ak)
    font(:lang=an)
    font(:lang=ast)
    font(:lang=av)
    font(:lang=ay)
    font(:lang=az-az)
    font(:lang=ba)
    font(:lang=be)
    font(:lang=bg)
    font(:lang=bi)
    font(:lang=bin)
    font(:lang=br)
    font(:lang=bs)
    font(:lang=bua)
    font(:lang=ce)
    font(:lang=ch)
    font(:lang=chm)
    font(:lang=co)
    font(:lang=crh)
    font(:lang=cs)
    font(:lang=csb)
    font(:lang=cu)
    font(:lang=cv)
    font(:lang=cy)
    font(:lang=da)
    font(:lang=de)
    font(:lang=el)
    font(:lang=en)
    font(:lang=eo)
    font(:lang=es)
    font(:lang=et)
    font(:lang=eu)
    font(:lang=fat)
    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=gn)
    font(:lang=gv)
    font(:lang=haw)
    font(:lang=ho)
    font(:lang=hr)
    font(:lang=hsb)
    font(:lang=ht)
    font(:lang=hu)
    font(:lang=hz)
    font(:lang=ia)
    font(:lang=id)
    font(:lang=ie)
    font(:lang=ig)
    font(:lang=ik)
    font(:lang=io)
    font(:lang=is)
    font(:lang=it)
    font(:lang=jv)
    font(:lang=kaa)
    font(:lang=ki)
    font(:lang=kj)
    font(:lang=kk)
    font(:lang=ku-tr)
    font(:lang=kum)
    font(:lang=kv)
    font(:lang=kw)
    font(:lang=kwm)
    font(:lang=ky)
    font(:lang=la)
    font(:lang=lb)
    font(:lang=lez)
    font(:lang=lg)
    font(:lang=li)
    font(:lang=ln)
    font(:lang=mg)
    font(:lang=mi)
    font(:lang=mk)
    font(:lang=mn-mn)
    font(:lang=mo)
    font(:lang=ms)
    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=os)
    font(:lang=pap-an)
    font(:lang=pap-aw)
    font(:lang=pl)
    font(:lang=pt)
    font(:lang=qu)
    font(:lang=quz)
    font(:lang=rm)
    font(:lang=rn)
    font(:lang=ro)
    font(:lang=ru)
    font(:lang=rw)
    font(:lang=sah)
    font(:lang=sc)
    font(:lang=sel)
    font(:lang=sg)
    font(:lang=sh)
    font(:lang=shs)
    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=sr)
    font(:lang=ss)
    font(:lang=st)
    font(:lang=su)
    font(:lang=sv)
    font(:lang=sw)
    font(:lang=tg)
    font(:lang=tk)
    font(:lang=tl)
    font(:lang=tn)
    font(:lang=to)
    font(:lang=tr)
    font(:lang=ts)
    font(:lang=tt)
    font(:lang=tw)
    font(:lang=ty)
    font(:lang=tyv)
    font(:lang=uk)
    font(:lang=uz)
    font(:lang=ve)
    font(:lang=vo)
    font(:lang=vot)
    font(:lang=wa)
    font(:lang=wen)
    font(:lang=wo)
    font(:lang=xh)
    font(:lang=yap)
    font(:lang=yo)
    font(:lang=za)
    font(:lang=zu)
    font(coval)
    sil-coval-fonts

Source checksums
----------------
http://openfontlibrary.org/assets/downloads/bretan/79f39f3224dd7db0aae8fca528dac1a4/bretan.zip :
  CHECKSUM(SHA256) this package     : d1441cca556f93ed46e7d9e83cef687a2f4f2f59be70f8676be39f3445eb0753
  CHECKSUM(SHA256) upstream package : d1441cca556f93ed46e7d9e83cef687a2f4f2f59be70f8676be39f3445eb0753


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review --mock-config fedora-rawhide-x86_64 -n sil-coval-fonts
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, fonts, Shell-api
Disabled plugins: Java, C/C++, Python, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG


finally i don't know if i need submit other file ? 

well, i really appreciate any suggestion :)

thanks parag

Comment 3 Parag AN(पराग) 2015-02-01 01:10:49 UTC
Quick comment: Font packages need some time to get reviewed for things which fedora-review tool not provide as checklist. I will review this in next 2 days. Meantime do some package reviews.

I think easiest way to use fedora-review tool is to get that package from latest comment where package links exists from bugzilla using

fedora-review -b 1187337 --mock-config fedora-rawhide-x86_64

You may want to fill all the markings to each checklist item for every review you post to other packages also. See the Legend at top of review text.

Comment 4 Parag AN(पराग) 2015-02-03 15:25:22 UTC
Some suggestions:

1) Drop the sil prefix. This font does not look to be published on http://scripts.sil.org website so "sil" as a foundry cannot be used.

2) As fonts do get updates without getting their metadata updated. I suggest always to use maximum for versioning like if font provides version number then use it. Also, use the date on which you downloaded source. So consider this as a post-release snapshot package as given in https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

If you look at http://openfontlibrary.org/en/font/bretan page, you can clearly see last updated date given as well as history also shows same zip archive name is used in past.

So, your release tag can be either
Release: 1.20150122%{?dist}
or today's date as you downloaded source today
Release: 1.20150203%{?dist}

But, considering all these facts I will say for now use "1.20150122" as updated date is specified on website. If it was not there we should use current date.

3) fontconfig file should say "sans-serif" and not "serif"

Comment 6 Parag AN(पराग) 2015-02-05 08:12:56 UTC
Hi Carlos,
  Can you start doing un-official package reviews of other packages? Also, you need to submit more packages in Fedora for review to get sponsored.

Comment 7 Carlos Morel-Riquelme 2015-02-05 08:48:05 UTC
hello parag, well i'm not sure if i have the knowledge for start package reviews of other packages. what do you thinks ?

about submit for packages don't worry i will submit more fonts.

finally i hope that the spec and srpm work fine.

regards and thank for all your help parag

:)

Comment 8 Parag AN(पराग) 2015-02-05 08:59:22 UTC
I see you know how to use fedora-review tool. Just use it on other packages. Fill the required markings like 
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

for all the review items and post the review to those review bugs.

Take a reference of following information
http://fedoraproject.org/wiki/Package_Review_Process
http://fedoraproject.org/wiki/Packaging:ReviewGuidelines
http://fedoraproject.org/wiki/Packaging_tricks

Note: If you will not do other's package reviews, its difficult to get sponsorship in packager group. The more you do package reviews, more we can know about your packaging knowledge. 

When you do any package review post that bugzilla link here so that I can have a look at it.

If you don't understand any packaging guidelines, feel free to ask on #fedora-devel or devel mailing list or directly here.

Comment 9 Carlos Morel-Riquelme 2015-02-06 07:42:38 UTC
Hello parag today i made to package review, this is the links, any correction please tell me :)

packages review

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

Also i added 2 news packages

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


Regards my friend :)

Comment 10 Parag AN(पराग) 2015-02-06 16:02:38 UTC
Review:

+ Mock build is successful for F22(x86_64)

+ rpmlint output on all generated rpm looks good
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

+ Source verified with upstream as (sha356sum)
upstream source: d1441cca556f93ed46e7d9e83cef687a2f4f2f59be70f8676be39f3445eb0753
srpm source: d1441cca556f93ed46e7d9e83cef687a2f4f2f59be70f8676be39f3445eb0753

+ License "OFL" is valid and included in source binary file

+ fontconfig file looks good

+ font metadata information is present

+ rest looks following packaging guidelines

Suggestions:
1) "appstream-util validate coval.metainfo.xml" gave some output from which 2 lines of output need to be fixed.
• style-invalid         : <summary> requires sentence case
• style-invalid         : <p> cannot contain a hyperlink

2) I am not a fan of using "oflb-" prefix as a foundry name but let's use it as its given in the guidelines. See http://fedoraproject.org/wiki/Packaging:FontsPolicy#Clarifications
=> your package name will be oflb-coval-fonts

3) Not mandatory but good to have font information added to Fedora wiki. See e.g. existing pages http://fedoraproject.org/wiki/Category:Packaged_fonts

When package underreview you need to set wiki page category to like http://fedoraproject.org/wiki/Category:In-progress_fonts

you will find this information in http://fedoraproject.org/wiki/Font_package_lifecycle

Comment 11 Parag AN(पराग) 2015-02-06 17:16:53 UTC
Just realized this font fits in fontconfig priority level 61-64. Other existing oflb LGC fonts are at 63 level. Choose 61 or higher maybe 63.

│ 61-64  │ Low priority LGC fonts                                             │

Comment 12 Carlos Morel-Riquelme 2015-02-07 23:49:29 UTC
Hi parag i've update and the spec file and i've followed your suggestions (1,2) about the step 3 , i can't edit the font wiki, maybe i need special privileges.

thank for all help, the appstream-util validate is very useful :)

spec: https://empateinfinito.fedorapeople.org/font/oflb-coval-fonts/oflb-coval-fonts.spec 

srpm: https://empateinfinito.fedorapeople.org/font/oflb-coval-fonts/oflb-coval-fonts-0.4-1.20150122.fc21.src.rpm

Comment 13 Parag AN(पराग) 2015-02-08 09:46:05 UTC
Once you get sponsorship in packager group you can add the wiki pages later on.

You need to keep the package name and summary same in this bug review summary also. I have changed it now.

Some issues with new update
1) You changed the summary. This is only one font being packaged and not set of fonts. So, you cannot say its font family. Better to keep summary as
Summary:  Derivation of other free of charge fonts

2) I see you removed the description from metainfo file and just put the summary. You should add more information there or same information that is in %description of spec file. Just see appstream-util command output also says
• style-invalid         : <p> is too short

You can use there
  <description>
    <p>
     Coval font is a free-to-use non-commercial font.
     Language Support: Basic Cyrillic, Basic Latin, Euro.
    </p>
  </description>

2) Another thing I see that you are changing version number which is not good. if you look into font metadata then there is version number specified as "1.000". You need to use that. Check metadata using command "ttname Coval.otf".

You need to keep same version that upstream provides and based on that version your changes happens with releases in Release tag. So, your Changelog should look like this

* Wed Feb 04 2015 Carlos Morel-Riquelme <empateinfinito> - 1.000-3.20150122
- Update the priority LGC font
- Fix the metainfo.xml

* Wed Feb 04 2015 Carlos Morel-Riquelme <empateinfinito> - 1.000-2.20150122
- Update the spec file
- Fix the metainfo.xml

* Wed Feb 04 2015 Carlos Morel-Riquelme <empateinfinito> - 1.000-1.20150122
- Fix the spec file
- Fix the fontconfig.conf

* Thu Jan 29 2015 Carlos Morel-Riquelme <empateinfinito> - 0.1-1
- Initial version
- Add metainfo file to show this font in gnome-software

Comment 14 Carlos Morel-Riquelme 2015-02-08 10:18:32 UTC
Thank so much parag, i really appreciate all your corrections. About the change of summary and %description well this happen because i see some of your specs file ( in bodhy ) but it's very good learn of my errors. 

well i update finally the files, it's here 

spec: https://empateinfinito.fedorapeople.org/font/oflb-coval-fonts/coval-fonts.spec

srpm : https://empateinfinito.fedorapeople.org/font/oflb-coval-fonts/oflb-coval-fonts-1.000-5.20150122.fc21.src.rpm

i've packed 10 fonts, so all this information will be very useful for fix all my errors.


Thanks !!!

Comment 15 Parag AN(पराग) 2015-02-08 11:10:04 UTC
Hey your release update should be -4 and not -5 and you removed spec file prefix "oflb-". Provide correct package.

Comment 17 Parag AN(पराग) 2015-02-09 03:50:55 UTC
Package looks good to me now :)

Now I can sponsor you based on your reviews to other packages which will show that you have found some problems and suggested fixes to those packages in your review comment. Once I see enough good package reviews, I will sponsor you.

Use fedora-review tool, mark all the review items.

Comment 18 Carlos Morel-Riquelme 2015-02-09 10:11:09 UTC
Thanks parag, i will update my others reviews. today i made a new review of other package, if you wish you can read here :

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

this is a older review
https://bugzilla.redhat.com/show_bug.cgi?id=1189171

any correction please tell me.


regards :)

Comment 19 Parag AN(पराग) 2015-02-13 15:09:42 UTC
Carlos,
   Have you got chance to do any more Full package reviews using fedora-review tool? you still need to show that you have reviewed packages and if found any issues suggested fixes there.

Also, I can do your other package reviews any time but sponsorship process needs you to do package reviews ;-)

Comment 20 Carlos Morel-Riquelme 2015-02-13 17:13:11 UTC
Thanks Parag, and yes i've made some reviews, please check this

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

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

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

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

Any correction or suggestion please tell me.


Regards Parag :)

Comment 21 Parag AN(पराग) 2015-02-15 14:18:48 UTC
Sorry to say but your above reviews are not enough. You are still not running fedora-review tool on these packages and not giving full package review.

Comment 22 Parag AN(पराग) 2015-03-03 16:23:00 UTC
*** Bug 1198246 has been marked as a duplicate of this bug. ***

Comment 23 Parag AN(पराग) 2015-03-03 16:57:34 UTC
FAS user empateinfinito moved to new FAS account name iddnna

Comment 24 Parag AN(पराग) 2015-03-03 16:58:51 UTC

*** This bug has been marked as a duplicate of bug 1198246 ***


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