[Bug 239471] Review Request: httptunnel - Tunnels a data stream in HTTP requests

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: httptunnel - Tunnels a data stream in HTTP requests


https://bugzilla.redhat.com/show_bug.cgi?id=239471


s-t-rhbugzilla at wwwdotorg.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |s-t-rhbugzilla at wwwdotorg.org




------- Additional Comments From s-t-rhbugzilla at wwwdotorg.org  2007-11-18 23:15 EST -------
This is not an official review; I don't think I can do that yet. Also, this is
actually my first review, so I'm using it as a learning experience too. Here are
my comments:

The spec file at the link above doesn't match the one in the SRPM:
[swarren at esk extracted]$ diff ../*spec *spec
11a12
> BuildRequires:  dos2unix
54d54
< - Make sure all files are encoded with UTF-8

BuildRoot doesn't match the most preferred value, although it's OK. Most
preferred is:
%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
See
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473

I see usage of both $RPM_BUILD_ROOT (using $ variable syntax) and %{name}. The
packaging guidelines indicated a requirement for consistency of macros. I'm not
sure if this implies all $ or all % type variable references, or just that
macros should always be used, or ...?

The following files should probably be added to %doc: DISCLAIMER HACKING NEWS TODO

License: (More of an issue for upstream) Logically, it's pretty clear this is
GPLv2. However, the source files simply say "see COPYING for license". Isn't GPL
code supposed to have a specific format for the comments in the source files
that reference COPYING. And, this specific format should state GPLv2 v.s. GPLv2+
too. I'm talking about this kind of thing:

==========
This program is free software; you can redistribute it and/or modify it
under the terms of the GNU General Public License as published by the
Free Software Foundation; either version 2, or (at your option) any
later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
USA.  */
============================================================

In %files, the following:

%defattr(-,root,root,-)

relies on "make install" to set the correct permissions. I've seen reviews that
requested something explicit, like this:

%files
%defattr(0644,root,root,0755)
%doc COPYING
%doc README.txt
%attr(0755, root, root) /sbin/fxload
%{_mandir}/*/*

Is the file port/getopt1.c (and other related files) a duplicate of code in the
standard getopt library? Can the application simply used standard getopt?

In the files port/getopt1.c and port/getopt.h, it seems like the copyright
header has been mucked with (I assume by upstream) since the fourth line of the
file doesn't seem to make sense, or tie into the rest of the comment:

============================================================
/* Declarations for getopt.
   Copyright (C) 1989,90,91,92,93,94,96,97 Free Software Foundation, Inc.

   the C library, however.  The master source lives in /gd/gnu/lib.

NOTE: The canonical source of this file is maintained with the GNU C Library.
Bugs can be reported to bug-glibc at prep.ai.mit.edu.
============================================================

File Makefile has (c) statement but no license. I don't know if that's OK.

Should the RFCs in the doc directory be packaged as %doc? The Debian package
admittedly doesn't do this.

Do you need to add another BuildRequires to ensure that iconv is available
during the build?

I am not totally sure if the path references in %install are OK. Is it
guaranteed where the source files will be extracted for you to modify?

... $RPM_BUILD_DIR/%{name}-%{version}/AUTHORS ...

I think that's it!


-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.