Bug 192606 - Review Request: yafc: yet another ftp client
Review Request: yafc: yet another ftp client
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-05-21 17:11 EDT by Chris Petersen
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-13 02:21:47 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑cvs+


Attachments (Terms of Use)
Patch for compiling with krb5 (441 bytes, patch)
2006-07-09 07:21 EDT, Gérard Milmeister
no flags Details | Diff

  None (edit)
Description Chris Petersen 2006-05-21 17:11:50 EDT
Spec URL: http://rpm.forevermore.net/yafc/yafc.spec
SRPM URL: http://rpm.forevermore.net/yafc/yafc-1.1.1-1.src.rpm
Description:

yafc is Yet Another FTP Client.  It is an interactive interface to the FTP
and SFTP protocols.

FEATURES
  - cached directory listings
  - uses readline (tab completion, emacs/vi editing keys, history file, etc.)
  - extensive tab completion
  - multiple connections open
  - aliases
  - colored ls (i.e., ls --color, uses $LS_COLORS like GNU ls)
  - autologin and bookmarks
  - recursive get/put/rm/ls
  - nohup mode get and put
  - tagging (queueing) of files for later transferring
  - automagically enters nohup-mode when SIGHUP received (in get and put)
  - redirection to local command or file ('>', '>>' and '|')
  - proxy support
  - more...
Comment 1 Gérard Milmeister 2006-06-05 09:10:08 EDT
* Probably better summary: "Yet Another FTP/SFTP Client"
* Better to take the description from http://sourceforge.net/projects/yafc:
  "Yafc is an OpenSource console mode FTP client. It has support for Kerberos 4/5  
   authentication and sftp (ssh2). Other features include tab completion,
   directory cache, powerful aliases, recursive file commands and bookmarks with
   autologin."
* fails to build in mock:
  RPM build errors:
    File not found: /var/tmp/yafc-1.1.1-1-root-mockbuild/usr/share/info/dir

  The dir file has not been created. Instead of "%exclude", simply
  use "rm -f" in the %install section, this works even if dir has not
  been created.
* Normally "install-info --delete" goes into %postun instead of %preun
* You don't need "Req: readline", rpmbuild takes care of this
* Simply removing the static keyworkd in gssapi.c makes
  it compile with kerberos enabled
* I don't think, that it is a good idea to enable color always. This
  does only work in ANSI terminals, it is a mess in dumb terminal, for
  example with emacs. In fact the doc states that, just as the real ls,
  you have to add the --color option to ls to enable color.

  
Comment 2 Chris Petersen 2006-07-03 16:56:48 EDT
I've fixed most of the concerns (haven't posted yet since I want to address the
other concerns, first).

I don't know what you mean about the fix for kerberos.  I think you meant to
type "static keywords" but don't know which ones you're talking about.  I don't
know much about compiling things, so I'm having a hard time figuring out what
you mean.  I'm also asking upstream, in hope that they can just fix the problem
so I don't need to maintain a patch.
Comment 3 Gérard Milmeister 2006-07-09 07:21:10 EDT
Created attachment 132127 [details]
Patch for compiling with krb5

Here is the patch for compiling with krb5.
You need of course add krb5-devel to the BR.
Comment 4 Chris Petersen 2006-07-23 18:24:29 EDT
spec:  http://rpm.forevermore.net/yafc/yafc.spec
srpm:  http://rpm.forevermore.net/yafc/yafc-1.1.1-3.src.rpm

ok, patch, etc applied.  Builds in mock, too.
Comment 5 Gérard Milmeister 2006-07-25 15:13:05 EDT
rpmlint result:
E: yafc zero-length /usr/share/doc/yafc-1.1.1/doc/yafc.info

The package does indeed contain an empty compressed file yafc.info.gz.
Comment 6 Ralf Corsepius 2006-07-26 01:46:20 EDT
(In reply to comment #5)
> rpmlint result:
> E: yafc zero-length /usr/share/doc/yafc-1.1.1/doc/yafc.info
> 
> The package does indeed contain an empty compressed file yafc.info.gz.

The cause is building triggers regeneration of the *.info while "makeinfo" is
missing.

/builddir/build/BUILD/yafc-1.1.1/support/missing: line 46: makeinfo: command not
found
WARNING: `makeinfo' is missing on your system.  You should only need it if
         you modified a `.texi' or `.texinfo' file, or any other file
         indirectly affecting the aspect of the manual.  The spurious
         call might also be the consequence of using a buggy `make' (AIX,
         DU, IRIX).  You might want to install the `Texinfo' package or
         the `GNU make' package.  Grab either from any GNU archive site.

=> BR: /usr/sbin/makeinfo
could be applied to work around this issue.


But .. the actual cause is deeper: The tarball is not packaged properly.
It contains a raw preconfigured CVS snapshot with all temporary files and broken
timestamps inside.

Due to this I strongly recommend to add a 
make distclean
to %prep to assure the subsequent %configure doesn't "reconfigure" the source
tree, but to configure it anew.

Further issues:

* You put *.texi's in %doc. These *.texi's are the sources of yafc.info. It
doesn't make much sense to put them into %docdir

* Related to the previous issue, the "prepare for %doc" block doesn't seem
useful to me. The package ships and installs mans and infos, its Makefiles
handle them correctly. There is no for any special treatment.

Comment 7 Chris Petersen 2006-09-07 16:22:42 EDT
I think I addressed the requested issues.  rpmlint is quiet now, too.

spec:  http://rpm.forevermore.net/yafc/yafc.spec
srpm:  http://rpm.forevermore.net/yafc/yafc-1.1.1-4.src.rpm
Comment 8 Kevin Fenzi 2006-09-12 17:57:19 EDT
Gérard or Ralf: Are either of you going to do a formal review on this package?

If not, I would be happy to step in and do so...
Comment 9 Gérard Milmeister 2006-09-12 19:28:25 EDT
(In reply to comment #8)
> Gérard or Ralf: Are either of you going to do a formal review on this package?
> 
> If not, I would be happy to step in and do so...
Yes, go on.
Comment 10 Kevin Fenzi 2006-09-12 22:11:53 EDT
OK - Package name
OK - Spec file matches base package name.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
832d074183a36ee15b47553ed5962fce  yafc-1.1.1.tar.bz2
832d074183a36ee15b47553ed5962fce  yafc-1.1.1.tar.bz2.1
OK - Package compiles and builds on at least one arch.
OK - BuildRequires correct
OK - Package owns all the directories it creates.
OK - Package has no duplicate files in %files.
See below - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Spec has consistant macro usage.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package doesn't own any directories other packages own.
OK - No rpmlint output.

SHOULD Items:

OK - Should include License or ask upstream to include it.
OK - Should build in mock.

Issues:

1. You might change your default attributes from:
%defattr(-, root, root)
to
%defattr(-, root, root,-)

Aside from that I see no issues.
Feel free to change issue #1 above when you check the package in.

Pending that, this package is APPROVED.
Comment 11 Chris Petersen 2006-09-13 02:16:19 EDT
Which license?  The GPL is there already (document is called COPYING, which it
is in many GPL programs).  Package builds fine in mock in my system, too.

Anyway, issue #1 is fixed, and I will add the package to extras soon.
Comment 12 Kevin Fenzi 2006-09-13 11:24:25 EDT
In reply to comment #11: 

Yeah, thats my checklist of items when looking at packages... 
the OK there means that they License was included and is ok. :) 
Comment 13 Chris Petersen 2007-06-22 14:26:00 EDT
Might as well get all of my good packages into EPEL:

Package Change Request
======================
Package Name: yafc
New Branches: EL-5
Comment 14 Kevin Fenzi 2007-06-22 15:50:42 EDT
cvs done.

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