Bug 225984 - Merge Review: lftp
Merge Review: lftp
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ruben Kerkhof
Fedora Package Reviews List
:
Depends On: 203574
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:18 EST by Nobody's working on this, feel free to take it
Modified: 2008-03-16 13:11 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-24 16:35:50 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ruben: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:18:18 EST
Fedora Merge Review: lftp

http://cvs.fedora.redhat.com/viewcvs/devel/lftp/
Initial Owner: mbarabas@redhat.com
Comment 1 Jose Pedro Oliveira 2007-02-03 11:47:54 EST
Pending issues from ticket
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=211483

 * Requirements and build requirement:  comment #7
 * Rpath problems: comments #9 and # 10

Comment 2 Ruben Kerkhof 2007-02-03 12:03:44 EST
* RPM name is OK
* Source lftp-3.5.1.tar.bz2 is the same as upstream
* Builds fine in mock
* File list looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)
* BuildRequires: gettext is missing (required to build the translations)
* The %makeinstall macro should not be used
  (wiki: PackagingGuidelines#MakeInstall)
* rpmlint is not silent, see below

Minor:
* Duplicate BuildRequires: autoconf (by automake), automake (by libtool)
* Please honor $RPM_OPT_FLAGS

rpmlint of lftp-3.5.1-2.fc6.i386.rpm:E: lftp binary-or-shlib-defines-rpath /usr/lib/liblftp-tasks.so.
0.0.0 ['/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.1/proto-ftp.so ['/usr/lib/lftp/3.5.1', '/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.1/proto-fish.so ['/usr/lib/lftp/3.5.1', '/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.1/proto-http.so ['/usr/lib/lftp/3.5.1', '/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.1/liblftp-network.so ['/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.1/proto-sftp.so ['/usr/lib/lftp/3.5.1', '/usr/lib']
W: lftp conffile-without-noreplace-flag /etc/lftp.conf
W: lftp devel-file-in-non-devel-package /usr/lib/liblftp-jobs.so
W: lftp devel-file-in-non-devel-package /usr/lib/liblftp-tasks.so
E: lftp library-without-ldconfig-postin /usr/lib/liblftp-jobs.so.0.0.0
E: lftp library-without-ldconfig-postun /usr/lib/liblftp-jobs.so.0.0.0
E: lftp library-without-ldconfig-postin /usr/lib/liblftp-tasks.so.0.0.0
E: lftp library-without-ldconfig-postun /usr/lib/liblftp-tasks.so.0.0.0

[ruben@odin lftp]$ rpmlint lftp-3.5.1-2.fc6.src.rpm 
W: lftp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 1)
W: lftp patch-not-applied Patch2: lftp-3.4.1-dont_core.patch
W: lftp patch-not-applied Patch181694: lftp-3.4.2-fix-redirect-coredump.patch
W: lftp patch-not-applied Patch173276: lftp-3.3.5-bz173276.patch

Comment 3 Jose Pedro Oliveira 2007-02-03 12:14:24 EST
Ruben,

Why did you review lftp 3.5.1 from FC-6? Should you have reviewed lftp 3.5.9
available in rawhide (the patches have already been removed) ?

jpo
Comment 4 Ruben Kerkhof 2007-02-04 05:11:13 EST
Ah, you're right, my mistake.
That leaves us with:

* RPM name is OK
* Source lftp-3.5.9.tar.bz2 is the same as upstream
* This is the latest version
* Builds fine in mock
* File list looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)
* BuildRequires: gettext is missing (required to build the translations)
* The %makeinstall macro should not be used
  (wiki: PackagingGuidelines#MakeInstall)
* Preserve timestamps when installing files
  (wiki: PackagingGuidelines#Timestamps

Minor:
* Duplicate BuildRequires: autoconf (by automake), automake (by libtool)

Rpmlint output:

Source RPM:
W: lftp mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 1)

rpmlint of lftp:
W: lftp incoherent-version-in-changelog 3.5.9 3.5.9-1.fc6
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.9/proto-ftp.so ['/usr/lib/lftp/3.5.9', '/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.9/proto-fish.so ['/usr/lib/lftp/3.5.9', '/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.9/proto-http.so ['/usr/lib/lftp/3.5.9', '/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.9/liblftp-network.so ['/usr/lib']
E: lftp binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.9/proto-sftp.so ['/usr/lib/lftp/3.5.9', '/usr/lib']
W: lftp conffile-without-noreplace-flag /etc/lftp.conf
W: lftp devel-file-in-non-devel-package /usr/lib/liblftp-jobs.so
W: lftp devel-file-in-non-devel-package /usr/lib/liblftp-tasks.so
E: lftp library-without-ldconfig-postin /usr/lib/liblftp-jobs.so.0.0.0
E: lftp library-without-ldconfig-postun /usr/lib/liblftp-jobs.so.0.0.0
E: lftp library-without-ldconfig-postin /usr/lib/liblftp-tasks.so.0.0.0
E: lftp library-without-ldconfig-postun /usr/lib/liblftp-tasks.so.0.0.0

Comment 5 Michael Schwendt 2007-02-04 06:32:26 EST
Instead of "Requires: perl-String-CRC32" prefer:

Requires: perl(String::CRC32)
Comment 6 Jose Pedro Oliveira 2007-02-04 09:32:41 EST
(In reply to comment #5)
> Instead of "Requires: perl-String-CRC32" prefer:
> 
> Requires: perl(String::CRC32)

A even better solution would be to drop this explicit requirement as rpmbuild
has no problem in detecting and adding it to the requirements list.

jpo
Comment 7 Ruben Kerkhof 2007-03-23 04:55:09 EDT
mbarabas, can you please create a new spec based on the comments above?
Comment 8 Jose Pedro Oliveira 2007-03-30 16:06:40 EDT
Upstream has released a new lftp version (3.5.10).

* Short changelog (http://lftp.yar.ru/news.html)

  Version 3.5.10 - 2007-03-26

    * fixed core dump when doing ls on file: connection.
    * fixed core dump when doing pget to write-protected directory. 
Comment 9 Terje Røsten 2007-04-05 11:37:00 EDT
From
https://www.redhat.com/archives/fedora-cvs-commits/2007-April/msg00189.html :

+%post
+/sbin/chkconfig --add lftp
+/sbin/ldconfig
+
+%postun
+if [ "$1" = 0 ]; then
+/sbin/chkconfig --del lftp
+/sbin/ldconfig
+fi
+exit 0
+

Why the call the to /sbin/chkconfig ?
There are no servers in the lftp packagew?
Cut-paste error?


Comment 10 Jose Pedro Oliveira 2007-04-05 17:03:07 EDT
Error message produced by the previous script during a yum update transaction
(rawhide@2007-04-05):
 
...
Finished Transaction Test
Transaction Test Succeeded
Running Transaction
... 
  Updating  : lftp                         ####################### [ 3/10] 
error reading information on service lftp: No such file or directory
  Updating  : yum-updatesd                 ####################### [ 4/10]
...
Comment 11 Terje Røsten 2007-04-05 18:31:14 EDT
> Running Transaction
> ... 
>   Updating  : lftp                         ####################### [ 3/10] 
> error reading information on service lftp: No such file or directory
>   Updating  : yum-updatesd                 ####################### [ 4/10]

Yeah , coming from the calls to chkconfig in scripts.

BTW: i guess automake and autoconf should be removed as buildreq?

More strange things: (rpath issue?):

# ldd /usr/lib/lftp/3.5.10/proto-file.so |grep ncur
        libncurses.so.5 => /lib/libncurses.so.5 (0x00fad000)

# /usr/lib/lftp/3.5.10/proto-http.so
/usr/lib/lftp/3.5.10/proto-http.so: error while loading shared libraries:
/usr/lib/libncurses.so.5: file too short

All lftp libs should be linked against libncurses in /lib, however because of rpath
libncurses in /usr/lib is used by some libs:

proto-sftp.so, proto-ftp.so, proto-fish.so  liblftp-network.so 
proto-http.so

Please read:
 http://fedoraproject.org/wiki/Packaging/Guidelines#head-a1dfb5f46bf4098841e31a75d833e6e1b3e72544
Comment 12 Jeremy Katz 2007-04-09 17:39:22 EDT
Also, there really shouldn't be a -devel subpackage for lftp.  It does not
provide an API for external things to use.
Comment 13 Karsten Hopp 2007-04-12 06:05:56 EDT
I'd like to repeat comment #9:
lftp provides no libraries in the standard paths and doesn't add a new path
in /etc/ld.so.conf.d. Therefore ldconfig in post/postun isn't required

lftp isn't a daemon, running chkconfig in post/postun isn't required.
Comment 14 Ruben Kerkhof 2007-06-17 03:00:50 EDT
Ping?
Comment 15 Maros Barabas 2007-10-04 08:35:26 EDT
Version: 3.5.14-2

/sbin/ldconfig -p | grep lftp
        liblftp-tasks.so.0 (libc6,x86-64) => /usr/lib64/liblftp-tasks.so.0
        liblftp-jobs.so.0 (libc6,x86-64) => /usr/lib64/liblftp-jobs.so.0

There are libraries in the standard paths: /usr/lib64/liblftp-*.so.0.0.0 and
therefore rpmlint yells, when there are no ldconfig postin(un) calls.

chkconfig is removed.
Comment 16 Ruben Kerkhof 2007-11-26 15:58:29 EST
Hi Maros,

Are you planning on rebasing to 3.6.1?

There are still a few things left to do:

Source RPM:
lftp.src: W: mixed-use-of-spaces-and-tabs (spaces: line 181, tab: line 1)
lftp.src: W: invalid-license GPL
I think this should be GPLv2+

Binary RPM:
[ruben@odin reports]$ rpmlint lftp/lftp-3.5.14-2.fc9.i386.rpm 
lftp.i386: W: file-not-utf8 /usr/share/doc/lftp-3.5.14/NEWS
lftp.i386: W: invalid-license GPL
lftp.i386: E: binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.14/proto-ftp.so ['/usr/lib/lftp/3.5.14', '/usr/lib']
lftp.i386: E: binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.14/proto-fish.so ['/usr/lib/lftp/3.5.14', '/usr/lib']
lftp.i386: E: binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.14/proto-http.so ['/usr/lib/lftp/3.5.14', '/usr/lib']
lftp.i386: E: binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.14/liblftp-network.so ['/usr/lib']
lftp.i386: E: binary-or-shlib-defines-rpath /usr/lib/lftp/3.5.14/proto-sftp.so ['/usr/lib/lftp/3.5.14', '/usr/lib']


Comment 17 Maros Barabas 2007-11-27 04:05:26 EST
Hi Ruben, 
     I was planning, but I'm no longer the maintainer of lftp. I'm sure Martin
will take care of it. Thanks.
Comment 18 Martin Nagy 2008-01-23 04:52:03 EST
Hi, I rebased to 3.6.1 and have committed changes to CVS, rpmlint now finds only
this:
lftp.i686: W: conffile-without-noreplace-flag /etc/ld.so.conf.d/lftp-i386.conf
This is okay, as it is a configuration file, but not to be changed by the user
and it can be changed in the future.
Please review, thanks!
Comment 19 Terje Røsten 2008-01-23 13:33:55 EST
Would be more easy if you could upload a srpm, well, well.

spec seems fine, however it's crashing, try this:

$ gdb
(gdb) exec-file /usr/bin/lftp
(gdb) run http://ftp.uninett.no/
(gdb) ls


I get:

cd ok, cwd=/                                           
lftp ftp.uninett.no:/> ls
`ls' at 0 [Waiting for response...]
Program received signal SIGSEGV, Segmentation fault.
parse_html (
    buf=0x96f1987 "<META content=\"\" name=\"redaksjon.vev\">\n<META
content=\"\" name=\"dato.verifisert\">\n<META content=\"\"
name=\"forfatter.navn\">\n<META content=\"\" name=\"forfatter.epost\">\n<META
content=\"\" name=\"forfatter.vev\">\n"..., buf_len=1645, eof=false,
list=@0x96cd1b8, set=0x0, all_links=0x96cd1d0, prefix=0x96cd078,
base_href=0x96cd1fc, 
    lsopt=0x96cd200, color=1) at HttpDir.cc:732
732           while(*scan && is_ascii_digit(*scan))
Current language:  auto; currently c++
(gdb) bt
#0  parse_html (
    buf=0x96f1987 "<META content=\"\" name=\"redaksjon.vev\">\n<META
content=\"\" name=\"dato.verifisert\">\n<META content=\"\"
name=\"forfatter.navn\">\n<META content=\"\" name=\"forfatter.epost\">\n<META
content=\"\" name=\"forfatter.vev\">\n"..., buf_len=1645, eof=false,
list=@0x96cd1b8, set=0x0, all_links=0x96cd1d0, prefix=0x96cd078,
base_href=0x96cd1fc, 
    lsopt=0x96cd200, color=1) at HttpDir.cc:732
#1  0x004fb00a in HttpDirList::Do (this=0x96cd188) at HttpDir.cc:1239
#2  0x0018b7a9 in SMTask::Schedule () at SMTask.cc:226
#3  0x001415af in Job::WaitDone (this=0x96a9ea0) at Job.cc:522
#4  0x0804c37e in ?? ()
#5  0x096a9ea0 in ?? ()
#6  0x096b91f0 in ?? ()
#7  0x00000000 in ?? ()

Comment 20 Martin Nagy 2008-01-24 09:09:02 EST
This was a bug in 3.6.1, this srpm contains fix, please test, thanks.
http://people.redhat.com/mnagy/lftp-3.6.1-1.fc9.src.rpm
Comment 21 Terje Røsten 2008-01-24 15:16:44 EST
Yes, works fine now, thanks!

rpmlint gives some warnings about "unused-direct-shlib-dependency", I leave
those to the reviewer (Ruben).
Comment 22 Martin Nagy 2008-01-24 15:34:56 EST
That's strange, I only see the one warning about conffile I mentioned earlier.
Comment 23 Terje Røsten 2008-01-24 15:52:49 EST
I guess you must install the package and then run 

$ rpmlint lftp 

to see these warnings.

Comment 24 Ruben Kerkhof 2008-01-24 16:35:50 EST
I've just rebuild the latest version in mock, and only see one warning:

[ruben@odin ~]$ rpmlint reports/lftp/lftp-3.6.1-1.fc9.i386.rpm 
lftp.i386: W: conffile-without-noreplace-flag /etc/ld.so.conf.d/lftp-i386.conf

But that's ok, and this package is approved.

Thanks Martin and Terje for your time.
Comment 25 Patrice Dumas 2008-01-24 17:08:40 EST
Are you sure that files in %{_libdir}/lftp/%{version}/ are
not dlopened and that /etc/ld.so.conf.d/%{name}-%{_arch}.conf
is useful?

I think that the DEBUG stuff should be removed.

The 
LDFLAGS="-L`pwd`/src/.libs $LDFLAGS";
certainly needs an explanation. I tried without and it built fine.

Also why use the system libtool? Is it to avoid the rpath? If it
doesn't work is it useful to keep it? And also is it really
useful to use system libtool for the install?

I also suggest doing
iconv -f ISO88591 -t UTF8 NEWS -o NEWS.tmp
touch -r NEWS NEWS.tmp
mv -f NEWS.tmp NEWS
to keep the NEWS file timestamp.

The postun is wrong, it should be run each time.

Comment 26 Martin Nagy 2008-01-25 08:34:36 EST
(In reply to comment #25)
I have made yet another srpm:
http://people.redhat.com/mnagy/lftp-3.6.1-1.fc9.src.rpm

> I think that the DEBUG stuff should be removed.
I see no reason for it to be there, so done.

> The 
> LDFLAGS="-L`pwd`/src/.libs $LDFLAGS";
> certainly needs an explanation. I tried without and it built fine.
This is a bit odd, it's been there for a longer time than it is in CVS, so no
idea who put it there. Anyway, I removed it and it built fine for me too.

> Also why use the system libtool? Is it to avoid the rpath? If it
> doesn't work is it useful to keep it? And also is it really
> useful to use system libtool for the install?
Hmm, good idea, I'll ask the previous maintainer if he remembers (this was done
6 years ago) why are we using system libtool. I fixed this in srpm.
 
> I also suggest doing
> iconv -f ISO88591 -t UTF8 NEWS -o NEWS.tmp
> touch -r NEWS NEWS.tmp
> mv -f NEWS.tmp NEWS
> to keep the NEWS file timestamp.
My fault, sorry about that, fixed.

> The postun is wrong, it should be run each time.
Fixed as well.

I also changed the patch which is better now.

Patrice: Thanks for spotting these.
Ruben: I still haven't commited the changes into cvs, could you please review if
these changes are good?
Comment 27 Patrice Dumas 2008-02-23 18:05:50 EST
The latest version seems to be 3.6.3, although there was no
announcement.
Comment 28 Martin Nagy 2008-02-25 04:55:42 EST
Hm, yes, a bit weird that it was not announced. I rebased anyway, thanks.
Comment 29 Terje Røsten 2008-02-27 14:33:54 EST
lftp is in good shape now, however I would use the %{_prefix} macro
here:

%configure --with-modules --disable-static --with-openssl=/usr --with-debug
--disable-rpath

like this:

%configure --with-modules --disable-static --with-openssl=%{_prefix}
--with-debug --disable-rpath
Comment 30 Martin Nagy 2008-02-28 11:10:23 EST
Will do, thanks.
Comment 31 Patrice Dumas 2008-03-16 13:11:58 EDT
All my concerns are fixed, now, thanks.

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