I would suggest that you first check with Fedora Legal ( https://www.redhat.com/mailman/listinfo/fedora-legal-list ) about the license.  This will avoid that a reviewer will invest time and then Legal drop the package.

Use:

License: MIT

I've added this as another MIT variant here:
https://fedoraproject.org/wiki/Licensing/MIT#WordNet_Variant

Lifting FE-Legal.

> I've added this as another MIT variant here:
> https://fedoraproject.org/wiki/Licensing/MIT#WordNet_Variant

Thanks Tom ! 

I've changed the License string, rebuilt the package and uploaded it to the location mentioned in the description.

+ CVE-2008-2149 (patch in Debian), CVE-2008-3908 (http://www.ocert.org/analysis/2008-014/wordnet.patch.2). Debian carries a split version of the patch for CVE-2008-3908.

+ Please have look at the patches used in the Debian package. Some of them are useful to have in Fedora. eg., the manual page fixes.

+ 'wordnet' might be a better name for the package because that is the name some other distributions (eg., Debian, Ubuntu) are using. Having consistency in naming across distributions is a good thing.

Debarshi, thanks for you comments.

(In reply to comment #4)
> + CVE-2008-2149 (patch in Debian), CVE-2008-3908
> (http://www.ocert.org/analysis/2008-014/wordnet.patch.2). Debian carries a
> split version of the patch for CVE-2008-3908.
> 
> + Please have look at the patches used in the Debian package. Some of them are
> useful to have in Fedora. eg., the manual page fixes.

I took a look at the debain patches and as far as I could tell, the man page that you pointed out and the security patch mentioned above were the only ones that I think were relevant to this Fedora package.

Both have been included.

> 
> + 'wordnet' might be a better name for the package because that is the name
> some other distributions (eg., Debian, Ubuntu) are using. Having consistency in
> naming across distributions is a good thing.

Umm, although i agree that naming across distributions is a good thing, I would say naming the package as the way the original (upstream) package, is a better thing.

There are a couple of other reasons:
a. 'wordnet' is a common noun where as 'WordNet' refers to the actual package from princeton ...don't believe me ? Ask WordNet :) ...

[steve@laptop ~]$ wn wordnet -over

Overview of noun wordnet

The noun wordnet has 2 senses (no senses from tagged texts)

1. wordnet -- (any of the machine-readable lexical databases modeled after the Princeton WordNet)
2. WordNet, Princeton WordNet -- (a machine-readable lexical database organized by meanings; developed at Princeton University)
[steve@laptop ~]$

b. Changing the %{name} in the spec file, implies that i'd have to change the name of the included tarball, but then the "Source:" tag would not be correct (actually, i did try to change all occurrences of WordNet to wordnet in the spec, but got some errors while building, which i could not understand ...not that i didn't spent too much time on investigation).

I could still rename the package if you still think it is a good thing.

The new spec and source files are at:
http://lonetwin.net/WordNet.spec
http://lonetwin.net/WordNet-3.0-2.fc10.src.rpm

- steve

Did not notice that you need a sponsor. Unassigning.

(In reply to comment #5)

> I took a look at the debain patches and as far as I could tell, the man page
> that you pointed out and the security patch mentioned above were the only ones
> that I think were relevant to this Fedora package.
> 
> Both have been included.

The manual pages append "WN" to the section numbers. There is a Debian patch to fix this. It will be good to have in our package too.

> Umm, although i agree that naming across distributions is a good thing, I would
> say naming the package as the way the original (upstream) package, is a better
> thing.

Not really. https://fedoraproject.org/wiki/Packaging/NamingGuidelines#General_Naming lays equal, if not more stress, on having consistency across distributions.
 
> There are a couple of other reasons:
> a. 'wordnet' is a common noun where as 'WordNet' refers to the actual package
> from princeton ...don't believe me ? Ask WordNet :) ...

That is not a problem.

> b. Changing the %{name} in the spec file, implies that i'd have to change the
> name of the included tarball

Not at all.

> I could still rename the package if you still think it is a good thing.

Finally it is going to be yours and your sponsor's call, but I think "wordnet" is better.

+ The package fails to build in a chroot: http://koji.fedoraproject.org/koji/taskinfo?taskID=1062215 To fix this add 'BuildRequires: libXft-devel'.

+ Header and lib*.so should not be a part of the main package. Instead they should be put in a -devel sub-pacakge. See: https://fedoraproject.org/wiki/Packaging/Guidelines#Devel_Packages

Debarshi,

Thanks for the through review and all your comments !

(In reply to comment #7)
> The manual pages append "WN" to the section numbers. There is a Debian patch to
> fix this. It will be good to have in our package too.

Fixed. I also noticed that although the Debian patch fixed the names of the man pages, it didn't fix the references with the pages itself. I've done those too.

> > I could still rename the package if you still think it is a good thing.
> 
> Finally it is going to be yours and your sponsor's call, but I think "wordnet"
> is better.

Fixed.

(In reply to comment #8)
> + The package fails to build in a chroot:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1062215 To fix this add
> 'BuildRequires: libXft-devel'.

Fixed.

> + Header and lib*.so should not be a part of the main package. Instead they
> should be put in a -devel sub-pacakge. See:
> https://fedoraproject.org/wiki/Packaging/Guidelines#Devel_Packages

Fixed.

The new srpm and spec file are at:
http://lonetwin.net/wordnet.spec
http://lonetwin.net/wordnet-3.0-3.fc10.src.rpm

- steve

Two issues here:

1. please use %{_datadir} instead of /usr/share.
2. These files shouldn't be in the package. 
/usr/share/wordnet-3.0/include/tk
/usr/share/wordnet-3.0/include/tk/tk.h
/usr/share/wordnet-3.0/include/tk/tkDecls.h

In fact, upstream really shouldn't be shipping these files as part of wordnet, they should rely on the system tk-devel (they're using the system libtk, but providing mismatched tk headers?).

I'll attach a patch to remove it from the Fedora package. You'd need to apply this patch and also delete the include/tk dir in %prep.

Show me a fixed package and I'll finish this review.

Created attachment 332434 [details]
Patch to use system tk headers

Thanks for the review spot.

(In reply to comment #10)
> Two issues here:
> 
> 1. please use %{_datadir} instead of /usr/share.

Done.

> 2. These files shouldn't be in the package. 
> /usr/share/wordnet-3.0/include/tk
> /usr/share/wordnet-3.0/include/tk/tk.h
> /usr/share/wordnet-3.0/include/tk/tkDecls.h
...
...
> I'll attach a patch to remove it from the Fedora package. You'd need to apply
> this patch and also delete the include/tk dir in %prep.

Applied you patch, and made the change in %prep.

> 
> Show me a fixed package and I'll finish this review.

The new package & spec is at:
http://www.lonetwin.net/wordnet.spec
http://lonetwin.net/wordnet-3.0-4.fc10.src.rpm

- steve

Bad:

1. The version in the changelog, should be 3.0-4, not 3.0.4.
2. The sums don't match to upstream:
Upstream SHA1: aeb7887cb4935756cf77deb1ea86973dff0e32fb
Your tarball's SHA1: fb2476bf83cbd14f2030c7c66b7485e49e36671c
3. There is a static lib in the -devel package. Unless we have a good reason, we don't package static libs:
https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries
4. Devel packages should require the main package (Requires: %{name} = %{version}-%{release})

Good:

- rpmlint checks return:
wordnet.x86_64: W: incoherent-version-in-changelog 3.0.4 ['3.0-4.fc11', '3.0-4']
- package meets naming guidelines
- package meets packaging guidelines
- license (MIT) OK, text in %doc, matches source
- spec file legible, in am. english
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- no .la files

I'd also prefer if you didn't wildcard everything so broadly in %files. That approach leads to extra files getting packaged upon updates without noticing it.
Clean up the bad items, and I'll give this another pass.

(In reply to comment #13)
> Bad:
> 
> 1. The version in the changelog, should be 3.0-4, not 3.0.4.

Done. Also, updated version to 3.0-5 due to the changes below.

> 2. The sums don't match to upstream:
> Upstream SHA1: aeb7887cb4935756cf77deb1ea86973dff0e32fb
> Your tarball's SHA1: fb2476bf83cbd14f2030c7c66b7485e49e36671c

Fixed. (huh, don't quite remember why there was a difference in the first place)

> 3. There is a static lib in the -devel package. Unless we have a good reason,
> we don't package static libs:
> https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries

Done. Although note that the dynamic lib is included in the wordnet package (rather than the -devel) because the wordnet binaries create a dependency on it.

> 4. Devel packages should require the main package (Requires: %{name} =
> %{version}-%{release})
> 

Done.

> I'd also prefer if you didn't wildcard everything so broadly in %files. That
> approach leads to extra files getting packaged upon updates without noticing
> it.
Done. The binaries are no longer selected with a wildcard, and all the other files are grouped into reasonably generic wildcards.

> Clean up the bad items, and I'll give this another pass.


The newer spec and src rpm are at:
http://www.lonetwin.net/wordnet.spec
http://lonetwin.net/wordnet-3.0-5.fc10.src.rpm

Thanks for your time.

cheers,
- steve

Well, if you're going to use a shared library here, you should do it properly, as a versioned .so, with proper packaging.

libWN_la_LDFLAGS = -version-number 3:0:0

will give you what you want, although, you might talk to upstream to be sure they're okay with the .so numbering starting there.

Don't forget to delete the .la and .a file, your -devel wildcard is catching them. See why wildcards are tricky! :)

Also, you need to change a few things to ensure that the autotooling happens cleanly, specifically:

* BuildRequires: libtool
* autoreconf -i instead of just autoreconf

(In reply to comment #15)
Thanks for your time and comments spot. Sorry about the slip-ups, this is the first time I played around with autotools and have learned a lot in the process.

> Well, if you're going to use a shared library here, you should do it properly,
> as a versioned .so, with proper packaging.
> 
> libWN_la_LDFLAGS = -version-number 3:0:0
> 
> will give you what you want, although, you might talk to upstream to be sure
> they're okay with the .so numbering starting there.
> 
I did mail upstream about this but got no response back. I also checked other distributions (ubuntu and mandriva) and they seem to be using version 3.x.x for the .so ...so, i went with that.

> Don't forget to delete the .la and .a file, your -devel wildcard is catching
> them. See why wildcards are tricky! :)

Yep ! ...and Done
 
> Also, you need to change a few things to ensure that the autotooling happens
> cleanly, specifically:
> 
> * BuildRequires: libtool
> * autoreconf -i instead of just autoreconf  
Done

The new spec and srpm are at:
http://www.lonetwin.net/wordnet.spec
http://lonetwin.net/wordnet-3.0-6.fc10.src.rpm

cheers,
- steve

Fixing the title to match the package so that review reports count it right.

%files
...
%{_libdir}/libWN.so*

%files devel
...
%{_libdir}/libWN.so*

You're double-packaging those files. The .so.* go in the main package, and the .so goes into the -devel package. It should be:

%files
...
%{_libdir}/libWN.so.*

%files devel
...
%{_libdir}/libWN.so

You're also missing the necessary %post and %postun invocations for packages with shared libraries, see:

https://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

(In reply to comment #18)
> 
> You're double-packaging those files. The .so.* go in the main package, and the
> .so goes into the -devel package. It should be:
> ...

Done.
 
> 
> You're also missing the necessary %post and %postun invocations for packages
> with shared libraries, see:
> 
> https://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries  

Yep ! Sorry about that ...something I forgot. Done.

As always, thanks for your time.

The new spec and srpm are at:
http://www.lonetwin.net/wordnet.spec
http://lonetwin.net/wordnet-3.0-7.fc10.src.rpm

cheers,
- steve

rpmlint says:

wordnet.src:107: W: macro-in-%changelog files
wordnet.src:109: W: macro-in-%changelog pre

(it also says:
wordnet.x86_64: W: shared-lib-calls-exit /usr/lib64/libWN.so.3.0.0 exit@GLIBC_2.2.5
but that is reasonably safe to ignore)

Just use %%macro instead of %macro when you mention things in the changelog. Oh, and you should also get in the habit of running rpmlint on your packages after you build them. :)

> 
> Just use %%macro instead of %macro when you mention things in the changelog.

Done. Although I didn't change the version for this trivial change. Hope that's ok.

> Oh, and you should also get in the habit of running rpmlint on your packages
> after you build them. :)  

Yep ! Will do.

Hope that all. The latest spec and src rpm are at:
http://www.lonetwin.net/wordnet.spec
http://lonetwin.net/wordnet-3.0-7.fc10.src.rpm

cheers,
- steve

Last thing, sorry for not noticing it earlier:

You've currently got:

%files
%defattr(-,root,root,-)
%doc AUTHORS COPYING INSTALL ChangeLog README doc/{html,ps,pdf}
...
%{_datadir}/%{name}-%{version}/dict/*
%{_datadir}/%{name}-%{version}/doc/html/*
%{_datadir}/%{name}-%{version}/doc/pdf/*
%{_datadir}/%{name}-%{version}/doc/ps/*
%{_datadir}/%{name}-%{version}/lib/wnres/*

There are two problems here.

1. You're not owning the %{_datadir}/%{name}-%{version}/ dir, just the directories beneath it.
You can solve this problem by replacing the subdirs with wildcards with simply:
%{_datadir}/%{name}-%{version}/

That translates into "own this directory and all files and directories underneath it".

2. You're packaging up duplicate copies of the same html,ps,pdf docs, in two different location. The ones in %{_datadir}/%{name}-%{version}/doc/ don't have %doc attribution, so they should just be manually removed after make install is done. 

# Remove duplicate copies of docs installed by make install
rm -rf $RPM_BUILD_ROOT%{_datadir}/%{name}-%{version}/doc 

Show me a new spec with both issues fixed and I'll finish this review off and sponsor you. :)

(In reply to comment #22)
> Last thing, sorry for not noticing it earlier:

Thanks, you've been very patient. This is my first submission but still that ought not to be an excuse for some of the basic things i've overlooked :).

Anyways, ...

> There are two problems here.
> 
> 1. You're not owning the %{_datadir}/%{name}-%{version}/ dir, just the
> directories beneath it.
> You can solve this problem by replacing the subdirs with wildcards with simply:
> %{_datadir}/%{name}-%{version}/
> 
> That translates into "own this directory and all files and directories
> underneath it".
Done.

> # Remove duplicate copies of docs installed by make install
> rm -rf $RPM_BUILD_ROOT%{_datadir}/%{name}-%{version}/doc 
Done.

> 
> Show me a new spec with both issues fixed and I'll finish this review off and
> sponsor you. :)  

Here goes:
http://www.lonetwin.net/wordnet.spec
http://lonetwin.net/wordnet-3.0-8.fc10.src.rpm


cheers,
- steve

== Review ==

Good:

- rpmlint checks return:
wordnet.x86_64: W: shared-lib-calls-exit /usr/lib64/libWN.so.3.0.0 exit@GLIBC_2.2.5 [SAFE TO IGNORE]
- package meets naming guidelines
- package meets packaging guidelines
- license (MIT) OK, text in %doc (source code does not include licensing, just blanket attribution, please ask upstream to include per file licensing)
- spec file legible, in am. english
- source matches upstream (aeb7887cb4935756cf77deb1ea86973dff0e32fb)
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file (tk application is just wish shell, does not meet requirements for desktop file inclusion)
- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

APPROVED.

Pick up the steps here:
http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored

Also, please don't forget about wmfire (478744)!

Hello spot,

(In reply to comment #24)
> == Review ==
>
> Good:
> ...
> APPROVED.
>
Thanks !

> Pick up the steps here:
> http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored
>
Thanks for sponsoring me too !

> Also, please don't forget about wmfire (478744)!
I've updated the BZ with a newer spec and srpm.

In relation to this bug, I have a question about sponsorship -- after being sponsored, does one have to wait for one's packages to be reviewed by another person (I would assume yes, since peer review is always a good thing), before submitting a CVS request ?

cheers,
- steve

New Package CVS Request
=======================
Package Name: wordnet
Short Description: A lexical database for the english language
Owners: lonetwin
Branches: F-10 F-11
InitialCC: lonetwin sundaram

(In reply to comment #25)

> In relation to this bug, I have a question about sponsorship -- after being
> sponsored, does one have to wait for one's packages to be reviewed by another
> person (I would assume yes, since peer review is always a good thing), before
> submitting a CVS request ?

For updates to an existing package, you have direct commit access. For new packages, you have to follow the review process as usual. You can even review others packages now except when they are a completely new packager and waiting on sponsorship.

CVS done.

wordnet-3.0-8.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/wordnet-3.0-8.fc11

wordnet-3.0-8.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/wordnet-3.0-8.fc10

wordnet-3.0-8.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

wordnet-3.0-8.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Once you sponsor someone, please remember to remove the FE-NEEDSPONSOR tag to
avoid noise in the blocker bug.

Package Change Request
======================
Package Name: wordnet
New Branches: EPEL
Owners: lonetwin sundaram
InitialCC: lonetwin sundaram

Need to add this package to EPEL because roshansingh the maintainer of artha, which depends on wordnet wants to push that package to EPEL.

Which EPEL branches? there is EL-4, EL-5 and EL-6?

Please repeat your request showing which exact branches you need.

Package Change Request
======================
Package Name: wordnet
New Branches: EL-4 EL-5 EL-6
Owners: lonetwin sundaram
InitialCC: lonetwin sundaram

Need to add this package to EPEL because roshansingh the maintainer of artha,
which depends on wordnet wants to push that package to EPEL.

(In reply to comment #35)
> Which EPEL branches? there is EL-4, EL-5 and EL-6?
> 
> Please repeat your request showing which exact branches you need.    

Well sorry, this was my first EPEL request and I was simply following the instructions at https://fedoraproject.org/wiki/EPEL/FAQ#Contributing_to_EPEL verbatim. I'll update the page to avoid confusion.

cheers,
- steve

CVS done (by process-cvs-requests.py).