Bug 290581 - Review Request: madan-fonts - Font for Nepali
Review Request: madan-fonts - Font for Nepali
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
: i18n
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-14 06:30 EDT by Rahul Bhalerao
Modified: 2007-11-30 17:12 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-17 03:55:41 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
madan-fonts.spec-1.patch (1.05 KB, patch)
2007-09-28 03:11 EDT, Jens Petersen
no flags Details | Diff

  None (edit)
Description Rahul Bhalerao 2007-09-14 06:30:03 EDT
Spec URL: http://rbhalera.fedorapeople.org/madan-fonts/madan-fonts.spec
SRPM URL: http://rbhalera.fedorapeople.org/madan-fonts/madan-fonts-1.0-1.fc8.src.rpm
Description: The madan-fonts package contains font for the display of
Nepali text.
Comment 1 Parag AN(पराग) 2007-09-19 07:34:52 EDT
We don't need following lines in SPEC
touch $RPM_BUILD_ROOT%{_datadir}/fonts/nepali/fonts.cache-1
%ghost %verify(not md5 size mtime) %{_datadir}/fonts/nepali/fonts.cache-1

Change summary from
Summary: Font for Nepali
to
Summary: Font for Nepali language

Change
cp *.ttf $RPM_BUILD_ROOT%{_datadir}/fonts/nepali
to
cp -p *.ttf $RPM_BUILD_ROOT%{_datadir}/fonts/nepali
This will preserve file timestamp.

Do we need to change license to
http://www.gnu.org/licenses/gpl-faq.html#FontException?
Comment 2 Jens Petersen 2007-09-19 07:45:00 EDT
I don't think we can add a Font Exception without upstream agreeing first.
Feel free to request/ask them about that though.

There is no need to define madan_version just use version instead.
Also -n argument can be dropped from %setup I think.

Please define fontdir to %{_datadir}/fonts/madan say and use that
instead of %{_datadir}/fonts/nepali.

Otherwise it looks ok to me.
Comment 3 Jens Petersen 2007-09-19 07:49:53 EDT
Is there an upstream URL for the source.
Comment 4 Rahul Bhalerao 2007-09-19 08:45:44 EDT
Only upstream URL for source is found to be this:
http://www.nepalinux.org/fonts/Madan.ttf
Comment 5 Rahul Bhalerao 2007-09-19 08:56:02 EDT
Found one more URL to the same file:
http://madanpuraskar.org/detail_guide/fonts/Madan.ttf
But there is no tarball available. Also the license mentioned is GPL only,
nothing about exception.
Comment 6 Jens Petersen 2007-09-19 09:03:24 EDT
Ah ok, I see you generated the tarball yourself.

Probably it is better just to use Madan.ttf as the source then IMHO.

(In reply to comment #5)
> Also the license mentioned is GPL only, nothing about exception.

Then I think it will just have to be plain GPL.
Comment 7 Rahul Bhalerao 2007-09-19 09:13:11 EDT
IMHO, %{_datadir}/fonts/nepali should not be changed to %{_datadir}/fonts/madan.
This may give rise to too many per font directories in /usr/share/fonts. Also
keeping fonts as language wise collection sounds better.
Comment 8 Jens Petersen 2007-09-19 09:50:35 EDT
Hmm I think I tend to disagree, from the point of view of %catalog (I think
you maybe need a symlink for that too?) and fc-cache having distinct directories
is cleaner and avoids problems with sharing of directories.
Comment 9 Rahul Bhalerao 2007-09-20 06:59:21 EDT
Updated spec and srpm available here:
Spec URL: http://rbhalera.fedorapeople.org/madan-fonts/madan-fonts.spec
SRPM URL: http://rbhalera.fedorapeople.org/madan-fonts/madan-fonts-1.0-2.fc8.src.rpm

Please review again. 
Comment 10 Rahul Bhalerao 2007-09-21 06:34:08 EDT
Updated spec again to use LICENSE file properly.
Spec URL: http://rbhalera.fedorapeople.org/madan-fonts/madan-fonts.spec
SRPM URL: http://rbhalera.fedorapeople.org/madan-fonts/madan-fonts-1.0-3.fc8.src.rpm
Comment 11 Parag AN(पराग) 2007-09-21 07:40:39 EDT
I think this package is ready for official review.
Jens,
 Do you see anything missing in this package?
Comment 12 Jens Petersen 2007-09-21 07:57:35 EDT
Some comments:

- I think "Requires(post): fontconfig" should be removed -
see the recent mails from mclasen and mailhot on fedora-packaging.

- please URLs for the .ttf and LICENSE file

- "cp -p %{SOURCE1} ." should be move to %prep

- still wondering if we should add %catalogue but I don't think that is a blocker

Otherwise looks ok to me. :)
Comment 14 Jens Petersen 2007-09-28 03:05:46 EDT
If there is no upstream license file then we can't include it.
Running strings on the .ttf file shows:

Open Type rules and Nepali glyphsets developed at Madan Puraskar Pustakalaya and
released under GPL. 
Contacts: 
Madan Puraskar Pustakalaya
Yalamaya Kendra, Patan, NEPAL
GPO Box. 42
www.mpp.org.np
info@mpp.org.np
Font Developers: 
Gaurav Shrestha 
Anjan Ale

Have we confirmed with the authors that it is intended to be GPLv2?
Comment 15 Jens Petersen 2007-09-28 03:11:49 EDT
Created attachment 209621 [details]
madan-fonts.spec-1.patch

some more recommended cleanup
Comment 17 Parag AN(पराग) 2007-10-15 00:07:33 EDT
Review:
+ package builds in mock (development i386).
+ rpmlint is silent for SRPM and for RPM.
+ source files match upstream url
6046ef235c9c9e61b1c4e67e3027298b  madan.ttf
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ font is open source-compatible.
- License text is NOT included in package.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains content.
+ no headers or static libraries.
+ no .pc file present.
+ no -devel subpackage
+ no .la files.
+ no translations are available
+ Does owns the directories it creates.
+ fonts scriptlets present.
+ no duplicates in %files.
+ file permissions are appropriate.
+ Not a GUI App.
APPROVED.
Comment 18 Parag AN(पराग) 2007-10-15 00:14:33 EDT
ohh. Just found that provided SPEC in comment #16 is missing %prep while
reviewing  this package I have it already added on my system so mock build went
fine.

Rahul,
 you missed comment #12
Comment 19 Parag AN(पराग) 2007-10-15 00:36:56 EDT
and similarly SOURCE md5sum is also wrong in my review post. It should be read as
d78a34b1d46dcfa74ff64e78dcc1fc31  Madan.ttf
Comment 20 Rahul Bhalerao 2007-10-15 02:15:53 EDT
Parag,
I see %prep is present in SPEC provided in comment 16, but only empty. The
license file is no more provided with this package since upstream does not
provide the one. What exactly did I miss in comment 12?
Comment 21 Parag AN(पराग) 2007-10-15 02:30:26 EDT
sure.
Mock build gave me
+ install -m 0644 -p '*.ttf'
/var/tmp/madan-fonts-1.0-5.fc8-root-mockbuild/usr/share/fonts/madan
install: cannot stat `*.ttf': No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.47557 (%install)

This mean you missed to add in %prep
cp -p %{SOURCE0} .

Empty %prep mean above install command will not find which *.ttf file to install.

Empty %prep will successfully run rpmbuild on SPEC outside mock. But when built
in mock it failed.
Comment 22 Parag AN(पराग) 2007-10-15 02:35:14 EDT
(In reply to comment #20)
> Parag,
> I see %prep is present in SPEC provided in comment 16, but only empty. The
> license file is no more provided with this package since upstream does not
> provide the one. What exactly did I miss in comment 12?

I missed to say that replace SOURCE1 with SOURCE0 and add that line in %prep.
Comment 24 Parag AN(पराग) 2007-10-15 07:03:00 EDT
Re-APPROVED above SRPM.
you can now request CVS branches.
Comment 25 Rahul Bhalerao 2007-10-15 07:07:33 EDT
New Package CVS Request
=======================
Package Name: madan-fonts
Short Description: Font for Nepali language
Owners: rbhalera
Branches: devel
InitialCC: petersen
Cvsextras Commits: yes

Comment 26 Kevin Fenzi 2007-10-15 11:35:50 EDT
cvs done.
Comment 27 Jens Petersen 2007-10-15 22:20:04 EDT
I think strictly the License should be noted as GPL (as mentioned in comment 6)
since no version information is provided in the distribution.
So better to fix that before importing to cvs, IMHO.

I hope upstream will include a license file in a future release.

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