Bug 248431 - (perl-Net-FTPServer) Review Request: perl-Net-FTPServer - Secure, extensible and configurable Perl FTP server
Review Request: perl-Net-FTPServer - Secure, extensible and configurable Perl...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
: Reopened
Depends On: 248425 248427
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-16 15:41 EDT by Steven Pritchard
Modified: 2008-10-11 10:41 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-11 10:41:05 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rjones: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Steven Pritchard 2007-07-16 15:41:28 EDT
Spec URL: http://ftp.kspei.com/pub/steve/rpms/perl-Net-FTPServer/perl-Net-FTPServer.spec
SRPM URL: http://ftp.kspei.com/pub/steve/rpms/perl-Net-FTPServer-1.122-1.src.rpm
Description:
Net::FTPServer is a secure, extensible and configurable FTP server
written in Perl.
Comment 1 Parag AN(पराग) 2007-07-20 01:27:15 EDT
Got many errors in make test
Can you check SRPM in mock build?

some of few lines I got in mock build are
t/330perl....................ok
t/350filters.................which: no uudecode in
(/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin)
skipped
        all skipped: no reason given
t/350generatorlist...........ok
t/350generatorzip............error: Tried to add member with zero or undef value
for time
 at /usr/lib/perl5/vendor_perl/5.8.8/Archive/Zip/Member.pm line 487

Comment 2 Steven Pritchard 2007-07-20 12:48:59 EDT
I was debating adding BR /usr/bin/uudecode, but that would make more sense if I
added a runtime dependency also, and I'm not sure I really want to encourage
turning on that functionality in a FTP server.

The zip generation is optional functionality.  I'm not sure how important the
test error is.  The build completed fine for me even with that error.

I can look into it a little deeper now if you want though.  (I planned on
looking at that later anyway...)
Comment 3 Chris Weyl 2007-08-09 21:56:53 EDT
Excellent.  I'm going to need this package on a couple boxes of mine soon :)

(In reply to comment #2)
> I was debating adding BR /usr/bin/uudecode, but that would make more sense if I
> added a runtime dependency also, and I'm not sure I really want to encourage
> turning on that functionality in a FTP server.

Adding a BR will allow the test suite to fully exercise functional bits of the
module and would seem to be in order...
 
> The zip generation is optional functionality.  I'm not sure how important the
> test error is.  The build completed fine for me even with that error.
> 
> I can look into it a little deeper now if you want though.  (I planned on
> looking at that later anyway...)

Filing a bug upstream and noting it in the spec would probably be a good way to
address this sort of error in the test itself.  (Just from the message here, it
sounds like t/350generatorzip.t is probably mis-calling Archive::Zip.)
Comment 4 Parag AN(पराग) 2007-09-27 11:18:42 EDT
Any progress here?
Comment 5 Parag AN(पराग) 2007-10-26 07:39:18 EDT
afaik make test should run all tests successfully otherwise specify a good
reason to avoid it from calling in check section.
Comment 6 Parag AN(पराग) 2008-01-15 10:05:48 EST
I prefer to move away from this review.
Comment 7 Ruben Kerkhof 2008-01-19 17:50:23 EST
Steve, can you update the package according to the comments?
I'll do a full review then.
Comment 8 Richard W.M. Jones 2008-02-28 14:38:43 EST
BTW, just a note to say that I am upstream maintainer for this
package.

The tests aim to test all possible functionality of the package,
so I'd suggest that you either add the necessary BuildRequires
for them to run, or else patch out the tests which you don't
want to run.
Comment 9 Jason Tibbitts 2008-03-01 14:17:10 EST
It's been many months since there's been any response from the submitter and
it's been set to needinfo for over four months, so I'm going to close this. 
Richard, if you happen to want to submit this package, I'm sure it will see a
quick review.
Comment 10 Steven Pritchard 2008-03-01 14:43:29 EST
Sorry, it's not that I lost interest in this package, it's just that I've had
*much* higher priority stuff to work on for a while now.

Ultimately, I need this as a dependency for Bricolage, but that's still not
ready for submission...

The package I submitted is nearly done.  If someone wants to take over as
submitter/primary maintainer, you certainly have my blessing.  I'll help with it
as much as I can.
Comment 11 Richard W.M. Jones 2008-03-01 16:47:53 EST
Reopening ...

Steve if you can't do anything on this then let me know and I'll
take a look at the package.
Comment 12 Jason Tibbitts 2008-04-04 14:59:17 EDT
So what's going on here?  Richard, if you want to submit a perl-Net-FTPServer
package then please go ahead.  I doubt it will take long to get it reviewed.
Comment 13 Richard W.M. Jones 2008-04-07 04:59:12 EDT
In a few days ... I'm going to be very busy this week.
Comment 14 Steven Pritchard 2008-05-07 17:38:22 EDT
Here's another try:

http://ftp.kspei.com/pub/steve/rpms/perl-Net-FTPServer-1.122-2.src.rpm

* Wed May 07 2008 Steven Pritchard <steve@kspei.com> 1.122-2
- Update License tag.
- BR uudecode, compress.
- Fix a problem with using constants from Archive::Zip
  (http://rt.cpan.org/Ticket/Display.html?id=35698).
- Remove both _bindir and _sbindir to be safe.

The t/350generatorzip warnings are still there, but I'm working on that (and I
reported them in that RT ticket mentioned above).
Comment 15 Richard W.M. Jones 2008-05-08 09:42:05 EDT
The package looks sensible at a quick glance.  The only thing that
rpmlint notices is that the README file isn't UTF-8.  You should
run it through iconv.

perl-Net-FTPServer.noarch: W: file-not-utf8
/usr/share/doc/perl-Net-FTPServer-1.122/README

What was the thinking with not distributing the /usr/sbin files?
Comment 16 Steven Pritchard 2008-05-08 11:15:00 EDT
(In reply to comment #15)
> The package looks sensible at a quick glance.  The only thing that
> rpmlint notices is that the README file isn't UTF-8.  You should
> run it through iconv.
> 
> perl-Net-FTPServer.noarch: W: file-not-utf8
> /usr/share/doc/perl-Net-FTPServer-1.122/README

Oops, right, I completely forgot to run rpmlint after the latest round of
changes.  I'll fix that.

> What was the thinking with not distributing the /usr/sbin files?

Do you think there's really all that much demand for the bundled stand-alone
ftpd(s)?  I'm assuming that most people will want the module to add that
functionality to some larger project (like, in my case, Bricolage).  They are in
%doc if I'm wrong...
Comment 17 Steven Pritchard 2008-05-09 11:44:13 EDT
(In reply to comment #16)
> (In reply to comment #15)
> > The package looks sensible at a quick glance.  The only thing that
> > rpmlint notices is that the README file isn't UTF-8.  You should
> > run it through iconv.
> > 
> > perl-Net-FTPServer.noarch: W: file-not-utf8
> > /usr/share/doc/perl-Net-FTPServer-1.122/README
> 
> Oops, right, I completely forgot to run rpmlint after the latest round of
> changes.  I'll fix that.

Fixed in -3.

http://ftp.kspei.com/pub/steve/rpms/perl-Net-FTPServer-1.122-3.src.rpm

> > What was the thinking with not distributing the /usr/sbin files?
> 
> Do you think there's really all that much demand for the bundled stand-alone
> ftpd(s)?  I'm assuming that most people will want the module to add that
> functionality to some larger project (like, in my case, Bricolage).  They are in
> %doc if I'm wrong...

That said, maybe we should break the bin/* items into their own "perl-ftpd"
subpackage or something.  That might be the best answer...
Comment 18 Jason Tibbitts 2008-05-10 18:47:07 EDT
I'm not taking this review so as to not steal it from Richard if he wants it, but here's a completed review checklist and some comments.

Seems to me the license should be GPLv2+.  The copyright notices are there in the code and documentation and have the usual "either version 2 of the License, or (at your option) any later version" language.

I don't have a problem with not including the actual server executables, or with splitting them out, but if the package doesn't actually install a server, shouldn't the Summary: and %description be adjusted?

Some weird stuff appears when running the tests:

error: Tried to add member with zero or undef value for time
 at /usr/lib/perl5/vendor_perl/5.10.0/Archive/Zip/Member.pm line 487
        Archive::Zip::Member::_unixToDosTime(0) called at /usr/lib/perl5/vendor_perl/5.10.0/Archive/Zip/Member.pm line 180
        Archive::Zip::Member::setLastModFileDateTimeFromUnix('Net::FTPServer::ZipMember=HASH(0x145a8d0)', 0) called at /builddir/build/BUI
LD/Net-FTPServer-1.122/blib/lib/Net/FTPServer.pm line 8121
        Net::FTPServer::ZipMember::_newFromFileHandle('Net::FTPServer::ZipMember', 'Net::FTPServer::InMem::FileHandle=HASH(0x14590e0)') ca
lled at /builddir/build/BUILD/Net-FTPServer-1.122/blib/lib/Net/FTPServer.pm line 4219
        Net::FTPServer::__ANON__() called at /builddir/build/BUILD/Net-FTPServer-1.122/blib/lib/Net/FTPServer.pm line 4341
        Net::FTPServer::visit('Net::FTPServer::InMem::Server=HASH(0xb0f418)', 'Net::FTPServer::InMem::DirHandle=HASH(0x1450ef8)', 'HASH(0x
1450e80)') called at /builddir/build/BUILD/Net-FTPServer-1.122/blib/lib/Net/FTPServer.pm line 4334
        Net::FTPServer::visit('Net::FTPServer::InMem::Server=HASH(0xb0f418)', 'Net::FTPServer::InMem::DirHandle=HASH(0x13ffae8)', 'HASH(0x
1450e80)') called at /builddir/build/BUILD/Net-FTPServer-1.122/blib/lib/Net/FTPServer.pm line 4243
        Net::FTPServer::_archive_generator_zip('Net::FTPServer::InMem::Server=HASH(0xb0f418)', 'Net::FTPServer::InMem::DirHandle=HASH(0x13
ffae8)') called at /builddir/build/BUILD/Net-FTPServer-1.122/blib/lib/Net/FTPServer.pm line 5362
        Net::FTPServer::_RETR_command('Net::FTPServer::InMem::Server=HASH(0xb0f418)', 'RETR', 'dir.zip') called at /builddir/build/BUILD/N
et-FTPServer-1.122/blib/lib/Net/FTPServer.pm line 3002
        Net::FTPServer::run('Net::FTPServer::InMem::Server', 'ARRAY(0x6239f8)') called at t/350generatorzip.t line 41

This error repeats several times.  Nevertheless, the tests pass.

* source files match upstream:
   d7a5257c982edaa83b2cd5c1cbb3190a27b8f9996b382cbaa69d2c5d6171d75c  
   Net-FTPServer-1.122.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is OK.
X license field does not match the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint is silent.
* final provides and requires are sane:
   perl(Net::FTPServer) = 1.122
   perl(Net::FTPServer::DBeg1::DirHandle)
   perl(Net::FTPServer::DBeg1::FileHandle)
   perl(Net::FTPServer::DBeg1::IOBlob)
   perl(Net::FTPServer::DBeg1::Server)
   perl(Net::FTPServer::DirHandle)
   perl(Net::FTPServer::FileHandle)
   perl(Net::FTPServer::Full::DirHandle)
   perl(Net::FTPServer::Full::FileHandle)
   perl(Net::FTPServer::Full::Server)
   perl(Net::FTPServer::Handle)
   perl(Net::FTPServer::InMem::DirHandle)
   perl(Net::FTPServer::InMem::FileHandle)
   perl(Net::FTPServer::InMem::Server)
   perl(Net::FTPServer::Proxy::DirHandle)
   perl(Net::FTPServer::Proxy::FileHandle)
   perl(Net::FTPServer::Proxy::Server)
   perl(Net::FTPServer::RO::DirHandle)
   perl(Net::FTPServer::RO::FileHandle)
   perl(Net::FTPServer::RO::Server)
   perl(Net::FTPServer::ZipMember)
   perl-Net-FTPServer = 1.122-3.fc9
  =
   perl(:MODULE_COMPAT_5.10.0)
   perl(Authen::PAM)
   perl(BSD::Resource)
   perl(Carp)
   perl(Carp::Heavy)
   perl(Config)
   perl(DBI)
   perl(Errno)
   perl(Fcntl)
   perl(File::Sync)
   perl(File::Temp)
   perl(FileHandle)
   perl(Getopt::Long)
   perl(IO::Dir)
   perl(IO::File)
   perl(IO::Scalar)
   perl(IO::Scalar) >= 1.126
   perl(IO::Seekable)
   perl(IO::Select)
   perl(IO::Socket)
   perl(IPC::Open2)
   perl(Net::FTP)
   perl(Net::FTPServer)
   perl(Net::FTPServer::DBeg1::DirHandle)
   perl(Net::FTPServer::DBeg1::FileHandle)
   perl(Net::FTPServer::DBeg1::IOBlob)
   perl(Net::FTPServer::DirHandle)
   perl(Net::FTPServer::FileHandle)
   perl(Net::FTPServer::Full::DirHandle)
   perl(Net::FTPServer::Full::FileHandle)
   perl(Net::FTPServer::Handle)
   perl(Net::FTPServer::InMem::DirHandle)
   perl(Net::FTPServer::InMem::FileHandle)
   perl(Net::FTPServer::Proxy::DirHandle)
   perl(Net::FTPServer::Proxy::FileHandle)
   perl(Net::FTPServer::RO::DirHandle)
   perl(Net::FTPServer::RO::FileHandle)
   perl(POSIX)
   perl(Socket)
   perl(Sys::Hostname)
   perl(strict)
   perl(vars)

* %check is present and all tests pass:
   All tests successful.
   Files=43, Tests=573, 20 wallclock secs ( 8.01 cusr +  1.28 csys =  9.29 CPU)
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
Comment 19 Steven Pritchard 2008-05-10 21:09:14 EDT
(In reply to comment #18)
> Seems to me the license should be GPLv2+.  The copyright notices are there in
the code and documentation and have the usual "either version 2 of the License,
or (at your option) any later version" language.

You're right.  My script that bumps the release converts "GPL" to "GPL+", and I
didn't think to look any closer than that.  That's fixed in -4.

> I don't have a problem with not including the actual server executables, or
with splitting them out, but if the package doesn't actually install a server,
shouldn't the Summary: and %description be adjusted?

Well, it *does* provide the ftp server functionality...

What to put in %description gets really confusing splitting out the server
scripts.  :-)

Anyway, let me know what you think of -4, if you have time:

http://ftp.kspei.com/pub/steve/rpms/perl-Net-FTPServer-1.122-4.src.rpm

> Some weird stuff appears when running the tests:
> 
> error: Tried to add member with zero or undef value for time
>  at /usr/lib/perl5/vendor_perl/5.10.0/Archive/Zip/Member.pm line 487
>         Archive::Zip::Member::_unixToDosTime(0) called at
/usr/lib/perl5/vendor_perl/5.10.0/Archive/Zip/Member.pm line 180
[...]

Reported in the RT ticket mentioned in comment #14.  I haven't had time to dig
any deeper than that yet.
Comment 20 Jason Tibbitts 2008-05-10 22:57:15 EDT
I'd probably add a note to each of the descriptions about the exact contents, like:
  This package contains the Perl modules; install the perl-ftpd package for the 
  server executables.
and
  This package contains server executables.
Also, once you have server executables, it may be prudent to consider how
they'll be started by the system.  That probably means initscripts or something
in /etc/xinetd.d depending on how the server is supposed to be run, unless you
really don't think anyone should be running this as their actual FTP server.  I
personally am wary of FTP servers, especially when they claim to be secure and
haven't been updated in three years.
Comment 21 Richard W.M. Jones 2008-05-12 05:28:18 EDT
I agree with Jason in comment 20.

The license is GPLv2+ (that was a condition of getting the code released
in the first place), and the license field is now correct in the -4 version.

I don't get the warning message that Jason reports in comment 18.  Maybe
something to do with the version of Perl?  Anyhow they seem harmless
enough.

Based on the checklist in comment 18, I will approve this package.
Comment 22 Steven Pritchard 2008-05-17 13:43:19 EDT
New Package CVS Request
=======================
Package Name: perl-Net-FTPServer
Short Description: Secure, extensible and configurable Perl FTP server
Owners: steve
Branches: F-8 F-9 EL-4 EL-5
InitialCC: perl-sig
Cvsextras Commits: yes
Comment 23 Steven Pritchard 2008-05-17 13:47:13 EDT
(In reply to comment #20)
> I'd probably add a note to each of the descriptions about the exact contents,
like:
>   This package contains the Perl modules; install the perl-ftpd package for the 
>   server executables.
> and
>   This package contains server executables.

I'll do that in -5 (after I've checked -4 into CVS).

> Also, once you have server executables, it may be prudent to consider how
> they'll be started by the system.  That probably means initscripts or something
> in /etc/xinetd.d depending on how the server is supposed to be run, unless you
> really don't think anyone should be running this as their actual FTP server.

I'll figure out what the package needs and add that to -5.

> I personally am wary of FTP servers, especially when they claim to be secure and
> haven't been updated in three years.

Then again, it's not like FTP has changed much in the last decade (or two)...  :-)
Comment 24 Steven Pritchard 2008-05-17 14:11:54 EDT
(In reply to comment #23)
> (In reply to comment #20)
> > Also, once you have server executables, it may be prudent to consider how
> > they'll be started by the system.  That probably means initscripts or something
> > in /etc/xinetd.d depending on how the server is supposed to be run, unless you
> > really don't think anyone should be running this as their actual FTP server.
> 
> I'll figure out what the package needs and add that to -5.

Actually, that will be in -6.  I seem to have messed up enough in -4 that I was
hoping someone could check out -5 for me while I work on that...

http://ftp.kspei.com/pub/steve/rpms/perl-Net-FTPServer-1.122-5.src.rpm
Comment 25 Kevin Fenzi 2008-05-17 16:24:38 EDT
cvs done.
Comment 26 Lubomir Rintel 2008-07-23 09:00:18 EDT
Please close once this is imported and build
Comment 27 Peter Robinson 2008-10-11 10:41:05 EDT
Closing - in rawhide

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