Fedora Merge Review: expect http://cvs.fedora.redhat.com/viewcvs/devel/expect/ Initial Owner: mitr
* RPM name is OK * Source expect-5.43.0.tar.gz is the same as upstream * This is the latest version * Builds fine in mock * rpmlint of expectk looks OK * rpmlint of expect-devel looks OK * File list of expectk looks OK * File list of expect-devel looks OK * File list of expect looks OK Needs work: * Use of buildroot is not consistant (wiki: PackagingGuidelines#UsingBuildRootOptFlags) * BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) (wiki: PackagingGuidelines#BuildRoot) * Spec file: some paths are not replaced with RPM macros (wiki: QAChecklist item 7) * The package should contain the text of the license (wiki: Packaging/ReviewGuidelines) Minor: * Duplicate BuildRequires: tcl-devel (by tk-devel), libX11-devel (by tk-devel), autoconf (by automake) Notes: * Please use {?dist} in the Release tag * Can you use make DESTDIR instead of make INSTALLROOT? * Replace /usr/share/man with %{_docdir} everywhere * Are you willing to consider building with --disable-static. You're not packaging static libraries, and this saves some build time. * If one of the packages is a gui application, a .desktop file should be installed (wiki: PackagingGuidelines#desktop) rpmlint of expect-5.43.0-6.i386.rpm:E: expect invalid-soname /usr/lib/libexpect5.43.so libexpect5.43.so E: expect script-without-shebang /usr/lib/expect5.43/pkgIndex.tcl E: expect wrong-script-interpreter /usr/lib/expect5.43/cat-buffers "expect" E: expect non-executable-script /usr/lib/expect5.43/cat-buffers 0644
Thanks! * Use of buildroot is not consistant Changed to use "$RPM_BUILD_ROOT" everywhere. * BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) Fixed. * Spec file: some paths are not replaced with RPM macros /usr/share/man replaced by %{_mandir} * The package should contain the text of the license It does, in /usr/share/expect-*/README: | I hereby place this software in the public domain. NIST and I would | appreciate credit if this program or parts of it are used. * Duplicate BuildRequires: tcl-devel (by tk-devel), libX11-devel (by tk-devel), autoconf (by automake) Because expect explicitly refers to tcl-devel and autoconf it IMHO should BuildRequire it explicitly; this is unrelated to the question whether e.g. automake depends on autoconf. BuildRequires: libX11-devel removed, expect doesn't directly refer to libX11. * Please use {?dist} in the Release tag I'd rather not; as long as each Fedora release uses a different NEVR, the dist tag is IMHO just useless clutter. * Can you use make DESTDIR instead of make INSTALLROOT? No, Makefile.in doesn't support this aspect of GNU coding standards and the variable is called INSTALL_ROOT. * Replace /usr/share/man with %{_docdir} everywhere ... with %{_mandir}, done. * Are you willing to consider building with --disable-static. You're not packaging static libraries, and this saves some build time. expect doesn't support --disable-static. * If one of the packages is a gui application, a .desktop file should be installed expectk is a programming language interpreter, I don't think it can be considered a GUI application (try running it). E: expect script-without-shebang /usr/lib/expect5.43/pkgIndex.tcl * Fixed by making the file unexecutable E: expect wrong-script-interpreter /usr/lib/expect5.43/cat-buffers "expect" E: expect non-executable-script /usr/lib/expect5.43/cat-buffers 0644 * Both Fixed by removing the file altogether E: expect invalid-soname /usr/lib/libexpect5.43.so libexpect5.43.so I'll try to clean this up tomorrow.
Hi Miloslav My comments inline: >> * The package should contain the text of the license > It does, in /usr/share/expect-*/README: I missed that one, thanks for pointing it out. >> * Duplicate BuildRequires: tcl-devel (by tk-devel), libX11-devel >> (by tk-devel), autoconf (by automake) > Because expect explicitly refers to tcl-devel and autoconf it IMHO should > BuildRequire it explicitly; this is unrelated to the question whether e.g. > automake depends on autoconf. Agreed. > BuildRequires: libX11-devel removed, expect doesn't directly refer to libX11. Thanks. >> * Please use {?dist} in the Release tag > I'd rather not; as long as each Fedora release uses a different NEVR, the > dist tag is IMHO just useless clutter. There are a number of pros and cons for the disttag. One pro is that it makes it easier to do mass rebuilds (for example for FC7-test1). You're not required to change it, of course, but please reconsider it. Some more pros (and cons) at http://fedoraproject.org/wiki/PackagingDrafts/DisttagsForRawHide >> * Can you use make DESTDIR instead of make INSTALLROOT? > No, Makefile.in doesn't support this aspect of GNU coding standards and > the variable is called INSTALL_ROOT. Ok. Can you let me know when you've updated the spec in cvs? I'll have another look then. Thanks, Ruben
I have just built expect-5.43.0-7 with the abovementioned changes (and some other cleanups). This leaves: E: expect invalid-soname /usr/lib/libexpect5.43.so libexpect5.43.so I have no idea where did the rpmlint's rules for soname come from. The upstream libexpect man page explicitly recommends linking with -lexpect5.43, so changing the soname would break this. I can change the soname, but it's a deviation from upstream and an extra patch to maintain, so I'd prefer to see some benefit to the change. Ville, any idea why rpmlint restricts soname values?
I can't say for sure, but perhaps this helps: $ rpmlint -I invalid-soname invalid-soname : The soname of the library is neither of the form lib<libname>.so.<major> or lib<libname>-<major>.so. The regexp used for the check is: ^lib.+(\.so\.[\.0-9]+|-[\.0-9]+\.so)$ Someone more familiar with sonames should to comment on whether there's something wrong with libexpect5.43.so. My guess would be no, don't change it, it's just unusual - cases like that are more often found in form like libexpect5-43.so or libexpect-5.43.so. Perhaps ask upstream what they think and if they'd like to change towards a more usual looking sonames for future releases?
(In reply to comment #5) > I can't say for sure, but perhaps this helps: > > $ rpmlint -I invalid-soname > invalid-soname : > The soname of the library is neither of the form lib<libname>.so.<major> or > lib<libname>-<major>.so. > > The regexp used for the check is: > > ^lib.+(\.so\.[\.0-9]+|-[\.0-9]+\.so)$ > > Someone more familiar with sonames should to comment on whether there's > something wrong with libexpect5.43.so. My guess would be no, don't change it, > it's just unusual - cases like that are more often found in form like > libexpect5-43.so or libexpect-5.43.so. Perhaps ask upstream what they think and > if they'd like to change towards a more usual looking sonames for future releases? The libfoo<major>.<minor>.so format is common for Tcl extensions (see Tcl and Tk), but doesn't seem to be used much elsewhere. As mentioned in comment #4, packages that wish to link against libexpect often use the -lexpect5.43 in order to guarantee a specific version. This seems to be historical cruft that never got replaced with a better alternative, and now 'libfoo<major>.<minor>.so is common enough that I expect other things might break if it's changed now.
expect-5.43.0-8 in devel contains abovementioned changes.
seems no-one is assigned here now, I will continue with the review: OK source files match upstream: e6aaab98967f6410099b40f2b3ddebb4 expect-5.43.0.tar.bz2 OK source contains full URL OK package meets naming and versioning guidelines. OK specfile is properly named, is cleanly written and uses macros consistently. OK dist tag is present. OK build root is correct. OK license field matches the actual license (public domain). OK license is open source-compatible. License text included in package. OK latest version is being packaged. OK BuildRequires are proper. OK compiler flags are appropriate. OK %clean is present. OK package builds in mock. BAD debuginfo package looks complete. - rpmlint warns expect-debuginfo.i386: W: spurious-executable-perm /usr/src/debug/expect-5.43/exp_main_tk.c (maybe that is nothing to worry about, let's see your comments) BAD rpmlint is silent. W: patch-not-applied Patch7: expect-5.43.0-tcl8.5.6.patch (this minor warning can be easily corrected) OK final provides and requires look sane. OK %check is present and all tests pass. BAD shared libraries should be added to the regular linker search paths. every binary RPM package which contains shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. -there's /usr/lib/libexpect5.43.so and no call to /sbin/ldconfig OK owns the directories it creates. OK doesn't own any directories it shouldn't. OK no duplicates in %files. OK file permissions are appropriate. OK no scriptlets present. OK code, not content. OK documentation is small, so no -docs subpackage is necessary. OK %docs are not necessary for the proper functioning of the package. OK headers in -devel. OK no pkgconfig files. OK no libtool .la droppings. OK not a GUI app. - expectk can interact or create gui apps, but isn't one itself, so that's ok the most important issue is imho the /sbin/ldconfig call when we add shared library to /usr/lib
cc-ing maintainer
ping?
(In reply to comment #8) [snip] > > BAD debuginfo package looks complete. - rpmlint warns > expect-debuginfo.i386: W: spurious-executable-perm > /usr/src/debug/expect-5.43/exp_main_tk.c > (maybe that is nothing to worry about, let's see your comments) Honestly, don't know why it's executable. Permissions are preserved from upstream. > > BAD rpmlint is silent. > W: patch-not-applied Patch7: expect-5.43.0-tcl8.5.6.patch > (this minor warning can be easily corrected) Fixed. > > OK final provides and requires look sane. > OK %check is present and all tests pass. > > BAD shared libraries should be added to the regular linker search paths. > every binary RPM package which contains shared library files (not just > symlinks) in any of the dynamic linker's default paths, must call ldconfig in > %post and %postun. > -there's /usr/lib/libexpect5.43.so and no call to /sbin/ldconfig > I think that this library is used by Tcl only, which knows the path to the library from /usr/lib64/tcl8.5/expect5.43/pkgIndex.tcl file. [snip] FYI, there's new version in devel branch...
seems OK now, thanks, review+