* 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+