Bug 225286 - Merge Review: aspell
Merge Review: aspell
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Roman Rakus
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-29 16:06 EST by Nobody's working on this, feel free to take it
Modified: 2014-01-12 19:06 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-14 09:21:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rrakus: fedora‑review+


Attachments (Terms of Use)
spec file diff of doom. This has all the changes that need to be made to pass the merge review (2.33 KB, patch)
2007-02-03 16:46 EST, Jef Spaleta
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-29 16:06:16 EST
Fedora Merge Review: aspell

http://cvs.fedora.redhat.com/viewcvs/devel/aspell/
Comment 1 Enrico Scholz 2007-01-31 16:56:12 EST
BLOCKER:

* dependency bloat by adding huge perl requirement due to a small utility 
  (aspell-import) which is not needed for core functionality
Comment 2 Robert Scheck 2007-02-03 11:00:24 EST
Which huge perl requirement?

> rpm -q --requires aspell | grep perl
/usr/bin/perl
> 
Comment 3 Enrico Scholz 2007-02-03 12:03:48 EST
perl package itself is 30MB, not counting its deps.

But this does not matter; problem is, that 'aspell-import' (which is the only
part which requires perl) is not needed for core functionality.

See http://www.redhat.com/archives/fedora-devel-list/2006-August/msg00735.html too
Comment 4 Jef Spaleta 2007-02-03 16:46:00 EST
Okay doing the merge review.

Summary:
There are things which need to be changed to meet the established review
guidelines.  Which I will go over in bloody detail below. 
However let me first say that I certaintly in agreement with Enrico concerning
the perl dep issue and I would very much recommend that the perl script be moved
to a subpackage or pushed into the docs section..whatever it takes to keep perl
from being a hard requirement of the base package.  Though the perl issue is
outside of my strict mandate to for the merge review.

I have attached a spec file diff which incorporate an attempt to fix the issues
I list as blockers below.  The package maintainer needs to review each of the
spec changes and make sure they are valid, the work, and aren't contentious.
If the package owner has a problem with anything I suggest, they need to report
back into this bug so we can talk about it.

Okay so on with the review

aspell
Checklist:  + GOOD  - BAD
+ rpmlint... see the notes at the end. I've rolled in changes into the spec from
the rpmlint log
+ packagename is fine
+ specfile name is fine
+ license check 
	LGPL in spec tag matches COPYING file in upstream source and COPYING file
included in doc section
+ spec is english-ish
+ md5sum check of sources
17fd8acac6293336bcef44391b71e337  aspell-0.60.5.tar.gz from SOURCE URL
17fd8acac6293336bcef44391b71e337  rpmbuild/SOURCES/aspell-0.60.5.tar.gz from SRPM
+ mock build  as done by matt

+ no buildrequires look good.

+ shared libs look fine
 ldconfig is being called in post preun as expected 
+ not designed to be relocatable
+ no duplicates in the files section
+ file permissions look okay to me
-  locales.. not so good	
+ headers in devel subpackage no static libs
+ docs section looks fine
+ no gui apps
+ no obvious duplicate file/directory ownership

BAD:
MUSTFIX: The spec file MUST handle locales properly. This is done by using the
%find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
MUSTFIX: Need to rm -rf $RPM_BUILD_ROOT  in the install section.
MUSTFIX: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability).
MUSTFIX: remove Prereq and use Requires(x) syntax for scriptlets

rpmlint run from matt/dell:
... notes from reviewer inline

rpmlint on ./aspell-debuginfo-0.60.5-2.fc7.i386.rpm
rpmlint on ./aspell-devel-0.60.5-2.fc7.i386.rpm
W: aspell-devel summary-ended-with-dot Static libraries and header files for
Aspell development.
... fixed in spec diff
rpmlint on ./aspell-0.60.5-2.fc7.i386.rpm
E: aspell obsolete-not-provided ispell
... looks like a valid obsolete

E: aspell obsolete-not-provided aspell-de
E: aspell obsolete-not-provided aspell-fr
E: aspell obsolete-not-provided aspell-ca
E: aspell obsolete-not-provided aspell-da
E: aspell obsolete-not-provided aspell-es
E: aspell obsolete-not-provided aspell-it
E: aspell obsolete-not-provided aspell-nl
E: aspell obsolete-not-provided aspell-no
E: aspell obsolete-not-provided aspell-sv
... since these are versioned obsoletes which have newer versions
in the package tree these are completely bogus error messages.
In fact, does aspell need to keep these obsoletes at all?

E: aspell obsolete-not-provided aspell-pt_BR
... this doesnt have a current package which provides this. 
This is most likely a valid obsoletes, and a dead-end.. so no error.

E: aspell obsolete-not-provided aspell-config
... this is no longer provided by anything.  Is this a valid obsoletes?
Is the pspell-config binary equivalent? If so can an aspell-config symlink
be added and a Provides put in place? I'm honestly not sure about this error.
Package owner will have to provide some backstory concerning this obsoletes.  

W: aspell file-not-utf8 /usr/share/info/aspell.info.gz
... ewww uhm this is an upstream issue. I dont think we can just run iconv
on a gzipped info file... perhaps rpmlint is just being silly here.

rpmlint on ./aspell-0.60.5-2.fc7.src.rpm
W: aspell prereq-use /sbin/install-info
... fixed with Requires(post) and Requires(preun) in spec diff

W: aspell unversioned-explicit-provides pspell
W: aspell unversioned-explicit-obsoletes ispell
W: aspell unversioned-explicit-obsoletes pspell
W: aspell unversioned-explicit-obsoletes aspell-pt_BR
W: aspell unversioned-explicit-obsoletes aspell-config
W: aspell unversioned-explicit-provides pspell-devel
W: aspell unversioned-explicit-obsoletes pspell-devel
W: aspell macro-in-%changelog doc
... reworded the changelog entry spec diff
W: aspell macro-in-%changelog serial
... reworded the changelog entry in spec diff
Comment 5 Jef Spaleta 2007-02-03 16:46:59 EST
Created attachment 147283 [details]
spec file diff of doom. This has all the changes that need to be made to pass the merge review

spec file diff of doom. This has all the changes that need to be made to pass
the merge review
Comment 6 Jef Spaleta 2007-02-04 13:04:50 EST
NOTE FOR DISCUSSION:

Right now all the individual dictionary aspell packages are technically noarch
payloads.. but they need to be arched packages because they want to drop content
down in {_libdir}/aspell-0.60 which parses into /usr/lib or /usr/lib64 and thus
is arch specific.

Does it make sense to change this so that the dictionary's are instead placed in
/usr/share? which is not arch specific?  Obviously this would be a win in a
multilib world. I can't see a long term down-side to rearranging the bits so
that aspell dictionary packages could be treated as noarch packages.

Am I missing something? Are the dictionary payloads arch specific and I'm just
blind?

-jef
Comment 7 Michał Bentkowski 2007-02-04 13:18:44 EST
(In reply to comment #6)
> Does it make sense to change this so that the dictionary's are instead placed 
in
> /usr/share? which is not arch specific?  Obviously this would be a win in a
> multilib world. I can't see a long term down-side to rearranging the bits so
> that aspell dictionary packages could be treated as noarch packages.

I was thinking about that too, when I was fixing an aspell-pl package. Putting 
files in %{_libdir} also makes ugly rpmlint error in aspell-* packages..
Comment 8 Patrice Dumas 2007-02-04 17:37:01 EST
(In reply to comment #6)
> NOTE FOR DISCUSSION:
> 
> Right now all the individual dictionary aspell packages are technically noarch
> payloads.. but they need to be arched packages because they want to drop content
> down in {_libdir}/aspell-0.60 which parses into /usr/lib or /usr/lib64 and thus
> is arch specific.
> 
> Does it make sense to change this so that the dictionary's are instead placed in
> /usr/share? which is not arch specific?

To me it is a blocker if it isn't done, since it goes against the
FHS, and sane packaging practices. 

Comment 9 Warren Togami 2007-02-07 16:16:50 EST
Initial Owner: varekova@redhat.com
Comment 10 Patrice Dumas 2007-02-07 16:21:36 EST
(In reply to comment #8)

> To me it is a blocker if it isn't done, since it goes against the
> FHS, and sane packaging practices. 

It was said on the list that the files are arch-dependent, so
may comment is irrelevant.
Comment 11 Ivana Varekova 2007-02-08 09:01:55 EST
Thanks for all comments - problems from review in comment 4 are fixed in
aspell-0.60.5-3.fc7. 
In this version aspell-import has changed permissions - so the perl is not required.
aspell dictationaries are arch dependent - so there is no problem in their
location in {_libdir}/aspell-0.60.
Comment 12 Jef Spaleta 2007-02-10 17:58:18 EST
Can you ping this bug when this build reaches the development tree? It hasn't
landed yet afaict, and I'd like to finish the review as soon as I can.

-jef
Comment 13 Ivana Varekova 2007-02-13 04:42:58 EST
aspell-0.60.5-3.fc7 is in development branch now - please Jef could you look at it. 
Comment 14 Jef Spaleta 2007-02-13 18:30:45 EST
uhm just dropping the executable bit from /usr/bin/aspell-import is inappropriate.

If you are going to drop the executable perms, then you should make it a doc
file.  Its acceptable to include non-critical helper scripts or example scripts
as part of the documentation as non-executable files.

If you want to leave it in the executable path, leave the script with executable
permissions, and put it in its own subpackage named something aspell-utils or
aspell-import or the like. Enrico's comment seems to suggest to me that he
prefers the subpackage approach.


-jef
Comment 15 Enrico Scholz 2007-02-13 19:24:35 EST
I would even prefer a yet more radical splitting:

* a -lib package with content of /usr/lib/aspell-0.60, the libs and
  the .mo files

* move the precat, preunzip, prezip, prezip-bin and word-list-compress
  binaries either into -devel (afais, they are only used to create
  dictionaries), or into a new -utils subpackage

* move 'aspell-import' into -utils

* keep aspell, ispell, run-with-aspell, spell and related man pages in
  main package. 'aspell' adds a ncurses dep which is not needed by
  -libs

* at first glance, 'aspell.info' contains end user information only
  and should stay in main and does not need to be moved into -lib

=======

Other issues:

* remove the explicit 'Requires: aspell-en'; I am pretty sure that my
  mom will never have use for the english dictionary but uses the
  german one only

* are all the 'Conflicts:' really required for a recent system?

* the
  | Provides: pspell < 0.13
  | Obsoletes: pspell < 0.13

  does not seem to make sense; either write 'Provides: pspell = 0.13',
  or remove it completely. Ditto for -devel

* the

  | Requires(pre): /sbin/install-info

  is wrong and should be 'Requires(post)'

* /sbin/ldconfig MUST be required

* afair, there was a rule, that Summary: must not begin with 'A'.

* the 'exit 0' in the scriptlets does not make sense
Comment 16 Enrico Scholz 2007-02-13 19:28:30 EST
the 

| PATH=/usr/lib/aspell-0.60:/usr/lib64/aspell-0.60:$PATH

line in /usr/bin/run-with-aspell looks suspicious on my i386 system. Last line

| exec $@

should be 

| exec "$@"

too.
Comment 17 Ivana Varekova 2009-11-27 06:16:13 EST
Hello, thanks for the comments, they are incorporated in aspell-0.60.6-8.fc13
in the last version I move aspell-import to documentation part (in the previous version aspell-import have not executable bit and nobody wants the rights back so I think the better solution is ot put it to the documentation).


(In reply to comment #15)
> I would even prefer a yet more radical splitting:
> 
> * a -lib package with content of /usr/lib/aspell-0.60, the libs and
>   the .mo files
> 
> * move the precat, preunzip, prezip, prezip-bin and word-list-compress
>   binaries either into -devel (afais, they are only used to create
>   dictionaries), or into a new -utils subpackage
> 
> * move 'aspell-import' into -utils
> 
> * keep aspell, ispell, run-with-aspell, spell and related man pages in
>   main package. 'aspell' adds a ncurses dep which is not needed by
>   -libs
> 
> * at first glance, 'aspell.info' contains end user information only
>   and should stay in main and does not need to be moved into -lib
For me the current system seems to be good enough (aspell-import is in documentation section - but nobody wants it)

 
> Other issues:
> 
> * remove the explicit 'Requires: aspell-en'; I am pretty sure that my
>   mom will never have use for the english dictionary but uses the
>   german one only

fixed

> * are all the 'Conflicts:' really required for a recent system?
thanks, removed
 
> * the
>   | Provides: pspell < 0.13
>   | Obsoletes: pspell < 0.13
thanks, removed

>   does not seem to make sense; either write 'Provides: pspell = 0.13',
>   or remove it completely. Ditto for -devel
> 
> * the
> 
>   | Requires(pre): /sbin/install-info
> 
>   is wrong and should be 'Requires(post)'
thanks, fixed
 
> * /sbin/ldconfig MUST be required
the dependency is automatically added - so no need to be there

> * afair, there was a rule, that Summary: must not begin with 'A'.
thanks, fixed

> * the 'exit 0' in the scriptlets does not make sense
thanks, fixed


Add #16 
| exec "$@"
is bogus - try e.g. exec "echo hello".

Is the package ok now?
Comment 18 Roman Rakus 2009-11-30 10:46:40 EST
#  MUST: rpmlint must be run on every package. The output should be posted in the review.[1]
rpmlint ~/Download/aspell-* aspell.spec 
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/nroff-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/sgml-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/context-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/email-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/tex-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/texinfo-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/aspell ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libpspell.so.15.1.4 ['/usr/lib64']
aspell.x86_64: W: spurious-executable-perm /usr/share/doc/aspell-0.60.6/aspell-import
aspell.x86_64: W: doc-file-dependency /usr/share/doc/aspell-0.60.6/aspell-import /usr/bin/perl
aspell-devel.x86_64: W: self-obsoletion pspell-devel < 0.13 obsoletes pspell-devel < 0.13
4 packages and 1 specfiles checked; 8 errors, 3 warnings.


# MUST: The package must be named according to the Package Naming Guidelines .
ok

# MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .
ok

# MUST: The package must meet the Packaging Guidelines .
# MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
ok

# MUST: The License field in the package spec file must match the actual license. [3]
BAD

# MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4]
ok

# MUST: The spec file must be written in American English. [5]
ok

# MUST: The spec file for the package MUST be legible. [6]
ok

# 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. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
ok

# MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]
ok

# MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8]
ok

# MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
ok

# MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]
BAD

# MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]
ok

# MUST: Packages must NOT bundle copies of system libraries.[11]
ok

# MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [12]
ok

# MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13]
ok

# MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [14]
ok

# MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [15]
ok
-------
# MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [16]
# MUST: Each package must consistently use macros. [17]
# MUST: The package must contain code, or permissable content. [18]
# MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [19]
# MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [19]
# MUST: Header files must be in a -devel package. [20]
# MUST: Static libraries must be in a -static package. [21]
# MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [22]
# MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [20]
# MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [23]
# MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[21]
# MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [24]
# MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [25]
# MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [26]
# MUST: All filenames in rpm packages must be valid UTF-8. [27]
----
Got to go. I will finish it later...
Comment 19 Roman Rakus 2009-12-01 03:59:48 EST
# MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT). [16]
ok

# MUST: Each package must consistently use macros. [17]
ok

# MUST: The package must contain code, or permissable content. [18]
ok

# MUST: Large documentation files must go in a -doc subpackage. (The definition
of large is left up to the packager's best judgement, but is not restricted to
size. Large can refer to either size or quantity). [19]
ok

# MUST: If a package includes something as %doc, it must not affect the runtime
of the application. To summarize: If it is in %doc, the program must run
properly if it is not present. [19]
ok

# MUST: Header files must be in a -devel package. [20]
ok

# MUST: Static libraries must be in a -static package. [21]
ok

# MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability). [22]
ok

# MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1),
then library files that end in .so (without suffix) must go in a -devel
package. [20]
ok

# MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release} [23]
ok

# MUST: Packages must NOT contain any .la libtool archives, these must be
removed in the spec if they are built.[21]
ok

# MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section. If you feel that your packaged GUI application does not need
a .desktop file, you must put a comment in the spec file with your explanation.
[24]
ok

# MUST: Packages must not own files or directories already owned by other
packages. The rule of thumb here is that the first package to be installed
should own the files or directories that other packages may rely upon. This
means, for example, that no package in Fedora should ever share ownership with
any of the files or directories owned by the filesystem or man package. If you
feel that you have a good reason to own a file or directory that another
package owns, then please present that at package review time. [25]
ok

# MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}
(or $RPM_BUILD_ROOT). [26]
ok

# MUST: All filenames in rpm packages must be valid UTF-8. [27]
ok
----
Summary:
Fix rpmlint errors.
Check licences:
- Many times LGPL (v2.1) (with incorrect FSF address)
- aspell-0.60.6/modules/speller/default/affix.cpp: BSD (2 clause) 
- aspell-0.60.6/misc/po-filter.c: GPL (v2 or later) (with incorrect FSF address) 
- aspell-0.60.6/ltmain.sh: GPL (v2 or later) 
- aspell-0.60.6/myspell/munch.c: BSD (2 clause) 
Remove using %{_datadir}/locale/*.
Comment 20 Roman Rakus 2009-12-01 04:07:02 EST
Also check comment #17. Changes are not made in rawhide.
Comment 21 Ivana Varekova 2009-12-01 09:20:54 EST
Fixed.
Comment 22 Roman Rakus 2009-12-03 06:49:43 EST
Still problems with rpath:
rpmlint aspell.spec aspell-0_60_6-9_fc13/*.rpm
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/nroff-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/sgml-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/context-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/email-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/tex-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/aspell-0.60/texinfo-filter.so ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/aspell ['/usr/lib64']
aspell.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libpspell.so.15.1.4 ['/usr/lib64']
4 packages and 1 specfiles checked; 8 errors, 0 warnings.
Comment 23 Ivana Varekova 2009-12-04 11:13:24 EST
Fixed in aspell-0.60.6-10.fc12, aspell-0.60.6-10.fc13.
Comment 24 Roman Rakus 2009-12-08 09:40:38 EST
Now it's ok.
Comment 25 Ivana Varekova 2009-12-14 09:21:55 EST
Review granted, so I'm closing this bug.

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