Bug 248431 (perl-Net-FTPServer)
Summary: | Review Request: perl-Net-FTPServer - Secure, extensible and configurable Perl FTP server | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Steven Pritchard <steve> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | cweyl, fedora-package-review, j, notting, pbrobinson, rjones, ruben |
Target Milestone: | --- | Keywords: | Reopened |
Target Release: | --- | Flags: | rjones:
fedora-review+
kevin: fedora-cvs+ |
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2008-10-11 14:41:05 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 248425, 248427 | ||
Bug Blocks: |
Description
Steven Pritchard
2007-07-16 19:41:28 UTC
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 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...) 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.) Any progress here? afaik make test should run all tests successfully otherwise specify a good reason to avoid it from calling in check section. I prefer to move away from this review. Steve, can you update the package according to the comments? I'll do a full review then. 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. 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. 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. Reopening ... Steve if you can't do anything on this then let me know and I'll take a look at the package. 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. In a few days ... I'm going to be very busy this week. 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> 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). 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? (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... (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... 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. (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. 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. 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. 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 (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)... :-) (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 cvs done. Please close once this is imported and build Closing - in rawhide |