Bug 473583 (wordnet) - Review Request: wordnet - A lexical database for the english language
Summary: Review Request: wordnet - A lexical database for the english language
Keywords:
Status: CLOSED NEXTRELEASE
Alias: wordnet
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tom "spot" Callaway
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-11-29 15:16 UTC by steve
Modified: 2010-07-12 17:12 UTC (History)
7 users (show)

Fixed In Version: 3.0-8.fc11
Clone Of:
Environment:
Last Closed: 2009-06-16 01:47:43 UTC
Type: ---
Embargoed:
tcallawa: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to use system tk headers (2.79 KB, patch)
2009-02-18 19:08 UTC, Tom "spot" Callaway
no flags Details | Diff

Description steve 2008-11-29 15:16:22 UTC
Spec URL: http://lonetwin.net/WordNet.spec
SRPM URL: http://lonetwin.net/WordNet-3.0-1.fc10.src.rpm
Description:
From the WordNet site: http://wordnet.princeton.edu/
WordNet is a large lexical database of English, developed under the direction of George A. Miller. Nouns, verbs, adjectives and adverbs are grouped into sets of cognitive synonyms (synsets), each expressing a distinct concept.

A couple of things about this package:
a. This is my first package submission, and although I have tried to package this to confirm to the Fedora packaging guidelines, please forgive me for any obvious mistakes.

b. The original package is licensed under it's own specific license named 'WordNet 3.0 license' (http://wordnet.princeton.edu/license) which is an open source license. Although I have packaged this application using the upstream license and have included the license file unmodified from the upstream, this license is not listed in the fedora licensing list at http://fedoraproject.org/wiki/Licensing#SoftwareLicenses.

Comment 1 Fabian Affolter 2008-12-01 13:51:08 UTC
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.

Comment 2 Tom "spot" Callaway 2008-12-01 16:08:38 UTC
Use:

License: MIT

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

Lifting FE-Legal.

Comment 3 steve 2008-12-01 16:22:04 UTC
> 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.

Comment 4 Debarshi Ray 2009-01-10 14:54:25 UTC
+ 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.

Comment 5 steve 2009-01-14 00:13:26 UTC
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

Comment 6 Debarshi Ray 2009-01-17 18:15:04 UTC
Did not notice that you need a sponsor. Unassigning.

Comment 7 Debarshi Ray 2009-01-17 18:25:15 UTC
(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.

Comment 8 Debarshi Ray 2009-01-17 18:31:03 UTC
+ 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

Comment 9 steve 2009-01-18 03:00:56 UTC
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

Comment 10 Tom "spot" Callaway 2009-02-18 19:07:59 UTC
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.

Comment 11 Tom "spot" Callaway 2009-02-18 19:08:49 UTC
Created attachment 332434 [details]
Patch to use system tk headers

Comment 12 steve 2009-02-18 20:22:07 UTC
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

Comment 13 Tom "spot" Callaway 2009-02-18 21:54:05 UTC
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.

Comment 14 steve 2009-05-19 15:02:06 UTC
(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

Comment 15 Tom "spot" Callaway 2009-05-19 16:04:17 UTC
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

Comment 16 steve 2009-05-27 11:34:02 UTC
(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

Comment 17 Rahul Sundaram 2009-05-27 15:43:54 UTC
Fixing the title to match the package so that review reports count it right.

Comment 18 Tom "spot" Callaway 2009-05-27 17:08:07 UTC
%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

Comment 19 steve 2009-05-27 17:57:59 UTC
(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

Comment 20 Tom "spot" Callaway 2009-05-27 18:21:26 UTC
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.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. :)

Comment 21 steve 2009-05-27 19:24:01 UTC
> 
> 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

Comment 22 Tom "spot" Callaway 2009-05-27 19:50:05 UTC
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. :)

Comment 23 steve 2009-05-27 21:51:59 UTC
(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

Comment 24 Tom "spot" Callaway 2009-05-28 13:06:13 UTC
== Review ==

Good:

- rpmlint checks return:
wordnet.x86_64: W: shared-lib-calls-exit /usr/lib64/libWN.so.3.0.0 exit.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)!

Comment 25 steve 2009-06-03 22:23:55 UTC
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

Comment 26 steve 2009-06-03 22:27:16 UTC
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

Comment 27 Rahul Sundaram 2009-06-04 06:56:41 UTC
(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.

Comment 28 Jason Tibbitts 2009-06-04 22:23:31 UTC
CVS done.

Comment 29 Fedora Update System 2009-06-05 12:45:53 UTC
wordnet-3.0-8.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/wordnet-3.0-8.fc11

Comment 30 Fedora Update System 2009-06-05 12:46:00 UTC
wordnet-3.0-8.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/wordnet-3.0-8.fc10

Comment 31 Fedora Update System 2009-06-16 01:47:36 UTC
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.

Comment 32 Fedora Update System 2009-06-16 01:57:55 UTC
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.

Comment 33 Susi Lehtola 2009-07-05 20:03:04 UTC
Once you sponsor someone, please remember to remove the FE-NEEDSPONSOR tag to
avoid noise in the blocker bug.

Comment 34 steve 2010-07-08 14:38:49 UTC
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.

Comment 35 Kevin Fenzi 2010-07-09 18:11:50 UTC
Which EPEL branches? there is EL-4, EL-5 and EL-6?

Please repeat your request showing which exact branches you need.

Comment 36 steve 2010-07-09 21:41:50 UTC
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

Comment 37 Kevin Fenzi 2010-07-12 17:12:01 UTC
CVS done (by process-cvs-requests.py).


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