Bug 227811 - Review Request: hunspell-af - Afrikaans hunspell dictionary
Review Request: hunspell-af - Afrikaans hunspell dictionary
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2007-02-08 06:50 EST by Caolan McNamara
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-02-14 04:26:39 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+
notting: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Caolan McNamara 2007-02-08 06:50:29 EST
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
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 13:50:10 EST
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 
- 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 
- 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
- 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 14:05:10 EST

* 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 14:08:50 EST
Damn, I missed those :(
Mamoru, thank you.
Comment 4 Michael Schwendt 2007-02-08 14:37:36 EST
* 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 15:37:28 EST
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-08 22:01:57 EST
- Source0 is not an absolute URL.
Comment 7 Caolan McNamara 2007-02-09 04:13:40 EST
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 04:40:17 EST
(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 05:38:25 EST
  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-09 22:17:30 EST
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 04:26:39 EST
 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.