Fedora Merge Review: aspell http://cvs.fedora.redhat.com/viewcvs/devel/aspell/
BLOCKER: * dependency bloat by adding huge perl requirement due to a small utility (aspell-import) which is not needed for core functionality
Which huge perl requirement? > rpm -q --requires aspell | grep perl /usr/bin/perl >
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
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
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
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
(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..
(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.
Initial Owner: varekova
(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.
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.
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
aspell-0.60.5-3.fc7 is in development branch now - please Jef could you look at it.
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
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
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.
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?
# 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...
# 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/*.
Also check comment #17. Changes are not made in rawhide.
Fixed.
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.
Fixed in aspell-0.60.6-10.fc12, aspell-0.60.6-10.fc13.
Now it's ok.
Review granted, so I'm closing this bug.