Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 227811 - Review Request: hunspell-af - Afrikaans hunspell dictionary
Summary: Review Request: hunspell-af - Afrikaans hunspell dictionary
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: manuel wolfshant
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2007-02-08 11:50 UTC by Caolan McNamara
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-14 09:26:39 UTC
Type: ---
Embargoed:
wolfy: fedora-review+
notting: fedora-cvs+


Attachments (Terms of Use)

Description Caolan McNamara 2007-02-08 11:50:29 UTC
Spec URL: http://people.redhat.com/caolanm/hunspell/hunspell-af.spec
SRPM URL: http://people.redhat.com/caolanm/hunspell/hunspell-af-0.20060117-1.src.rpm
Description: Afrikaans hunspell dictionary


1) http://wiki.services.openoffice.org/wiki/Dictionaries#Afrikaans_.28South_Africa.29
2) LGPL
3) splits this dictionary out of OOo to becomes a standalone package which can be independently updated and reused by other applications, e.g. firefox when it moves to hunspell

Comment 1 manuel wolfshant 2007-02-08 18:50:10 UTC
Caolan: I am willing to work with you through all dictionaries.

Quick review for this package:
- unzip is not needed as BR because it's on the exception list (see 
http://fedoraproject.org/wiki/Extras/FullExceptionList)
- adding -q to the %setup line and an empty %build would silence rpmlint (see below)

Everything else seems fine
- package meets naming guidelines
- package meets packaging guidelines 
- spec file legible, in am. english
- source matches upstream , sha1sum fa0dfbe45efd7eeba04e069f2e5987b721ebae71 
af_ZA-pack.zip
- the package builds in mock, devel/x86_64, generates a noarch (which is
consistent with the fact that basically it includes only 3 text files)
- the license LGPL stated in the tag is the same as the web site says
- there are only 2 files (word lists) + a short doc with instructions and
license clearance, so no .la, .pc, static files
- no missing BR
- MINOR: unneeded BR: unzip
- no locales
- not relocatable
- owns all files/directories that it creates, does not take ownership of other
files/dirs
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- rpmlint output:
on source:
W: hunspell-af setup-not-quiet
W: hunspell-af no-%build-section
on binary: silent
- code, not content
- no need for -docs as the only doc is just a 31K text file
- nothing in %doc affects runtime
- no need for .desktop file 

The package is APPROVED but before comitting to CVS please fix the three small
problems mentioned in the first paragraph.


Comment 2 Mamoru TASAKA 2007-02-08 19:05:10 UTC
Well,

* Timestamps
  - Keep timestamps. i.e. Use "cp -p" 

* By the way, some reason you don't want to use %?dist tag?

Comment 3 manuel wolfshant 2007-02-08 19:08:50 UTC
Damn, I missed those :(
Mamoru, thank you.


Comment 4 Michael Schwendt 2007-02-08 19:37:36 UTC
* Missing empty %build section.

* Added value in the %install loop would be to %lang'ify the files
via a file-list and by copying the Locale identifier from the
file names, like "%lang(af_ZA) foobar".

That depends a bit on the future of %lang in RPM and in related
package tools, however.

I've seen it in several packages, e.g. kphotoalbum.


Comment 5 Caolan McNamara 2007-02-08 20:37:28 UTC
updated spec and srpm with setup -q, cp -p, %?dist, removed BR unzip and added
an empty %build.

I'd rather not set %lang on the files as they're dictionaries rather than
documentation or translations, I'm not comfortable that it fits perfectly
conceptually to mark dicts that way as well.

Comment 6 Ralf Corsepius 2007-02-09 03:01:57 UTC
- Source0 is not an absolute URL.

Comment 7 Caolan McNamara 2007-02-09 09:13:40 UTC
oky doky, I don't see a section in our guidelines about this, but not a problem.
Absolute URL for Source: in the current version

Comment 8 Ralf Corsepius 2007-02-09 09:40:17 UTC
(In reply to comment #7)
> oky doky, I don't see a section in our guidelines about this, but not a problem.
> Absolute URL for Source: in the current version
Where have you been throughout the years Fedora exists?
This rule had even been enforced during Fedora.us.

But I realize, the Guidelines are a bit vague on this.

The rationale behind this: Without an absolute URL, it's unnecessary hard to
* verify if the source tarball matches upstream or if it has been compromised. 
* to track upstream activities (Is the tarball still there, has a new version
been released etc.)

You will also want to have a look at /usr/bin/spectool (from package
rpmdevtools, available from FE).

Comment 9 manuel wolfshant 2007-02-09 10:38:25 UTC
  The wiki says "- MUST: The sources used to build the package must match the
upstream source, as provided in the spec URL. Reviewers should use md5sum for
this task." Maybe we could rephrase that in a form which makes clear that full
URL is mandatory (and sha1sum is also allowed as alternative) ? Since this
version is, just as you said, a bit vague, I was never picky about full URL, as
long as the URL + %name + %version led to a valid result.


Comment 10 manuel wolfshant 2007-02-10 03:17:30 UTC
Caolan, it looks like you have fixed all recommendations that were suggested and
that no one has anything else to comment.
I suggest considering this package as approved and using this spec as template
for the others. I will be glad to review them all and I hope that other eyes
will keep verifying, just like before. I also hope I won't miss anything :)

PS: in the future, please increase the release tag and add an entry in the
%Changelog when you modify the spec.

Comment 11 Caolan McNamara 2007-02-14 09:26:39 UTC
 27535 (hunspell-af): Build on target fedora-development-extras succeeded.


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