Bug 239471 - Review Request: httptunnel - Tunnels a data stream in HTTP requests
Review Request: httptunnel - Tunnels a data stream in HTTP requests
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Package Reviews List
:
: 251040 416391 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-08 13:14 EDT by Sindre Pedersen Bjørdal
Modified: 2008-01-08 17:04 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-08 17:04:00 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Sindre Pedersen Bjørdal 2007-05-08 13:14:16 EDT
Spec URL: http://folk.ntnu.no/sindrb/packages/green_nyc/httptunnel.spec
SRPM URL: http://folk.ntnu.no/sindrb/packages/green_nyc/httptunnel-3.3-1.fc7.src.rpm

Description:
HTTPTunnel creates a bidirectional virtual data stream tunnelled in
HTTP requests. The requests can be sent via a HTTP proxy if so desired.
Comment 1 Jochen Schmitt 2007-05-08 13:49:47 EDT
Good:
+ Naming seems ok.
+ Tar ball matches with upstream.
* License seems ok.
+ Local build works fine.
+ Rpmlint is quite for the source rpm.
+ Pacakge contains verbatin text of the license.
+ Mock build works fine.
+ Local start of the application works.
+ Local install und uninstall works fine.

Bad:
- Rpmlint complaints binary package
W: httptunnel incoherent-version-in-changelog 2.0.8-1 3.3-1

Perhaps anone can check, if the compiled package works properly.



Comment 2 Sindre Pedersen Bjørdal 2007-06-06 12:19:27 EDT
Updated: 

- Fix incoherent version in changelog

Spec URL: http://folk.ntnu.no/sindrb/packages/green_nyc/httptunnel.spec
SRPM URL: http://folk.ntnu.no/sindrb/packages/green_nyc/httptunnel-3.3-2.fc7.src.rpm

This works properly for me, but as I can't really review my own package someone
else SHOULD take a look. 
Comment 3 Till Maas 2007-08-29 05:57:40 EDT
GPL is not a valid license tag anymore:

http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#head-f21ae23bf2f278444e2c385463cfa74a502396b8
Comment 4 Sindre Pedersen Bjørdal 2007-11-11 18:02:26 EST
Updated, finally:

- Update License tag to GPLv2
- Make sure all files are encoded with UTF-8

Spec URL: http://sindrepb.fedorapeople.org/packages/httptunnel.spec
SRPM URL: http://sindrepb.fedorapeople.org/packages/httptunnel-3.3-3.fc8.src.rpm
Comment 5 Stephen Warren 2007-11-18 23:15:10 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@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@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!
Comment 6 Stephen Warren 2007-11-18 23:46:30 EST
One more thing. I notice that the latest stable release is 3.0.5, and version
3.3 (which is being packaged) is a development release. Is it appropriate to
package the development release?
Comment 7 Søren Sandmann Pedersen 2007-12-05 16:57:05 EST
*** Bug 251040 has been marked as a duplicate of this bug. ***
Comment 8 Jason Tibbitts 2007-12-10 15:33:08 EST
*** Bug 416391 has been marked as a duplicate of this bug. ***
Comment 9 Kevin Kofler 2007-12-10 20:11:53 EST
> BuildRoot doesn't match the most preferred value, although it's OK.

Most packages use that BuildRoot and it's sane, so I don't see why it should be 
changed. :-)

> 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.

That's upstream's call to make, the problem with this format is that it's vague 
as to what versions of the GPL are allowed, but putting in License: GPLv2 is 
safe.

> In %files, the following:
> %defattr(-,root,root,-)
> relies on "make install" to set the correct permissions.

And "make install" should set the correct permissions if it's working 
correctly. :-) Actually, almost all packages use this format, setting 
permissions explicitly is normally only used if the upstream "make install" is 
broken.

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

All the rest being GPLed, I'd assume this is just an oversight and the Makefile 
is covered by the COPYING being shipped too. It isn't copied from somewhere 
else or anything.

> One more thing. I notice that the latest stable release is 3.0.5, and version
> 3.3 (which is being packaged) is a development release. Is it appropriate to
> package the development release?

The "development release" isn't really under development. :-)

To the submitter: can you please look at Stephen Warren's other comments and 
either fix the issues or say why they aren't problems? It looks to me like the 
review is stalled on that.
Comment 10 Sindre Pedersen Bjørdal 2007-12-11 05:56:25 EST
Finally dealing with this, sorry for taking so long: 

BuildRoot doesn't match the most preferred value:

This is a non-issue, I'll keep on using the BuildRoot created  by rpmdev-newspec

Added files to %doc DISCLAIMER HACKING NEWS TODO

As for the getopt issue. I just dont know how to deal with this, or if it's an
issue at all. Upstream is dead so I can't ask there. I've looked to other
distributions, and neither debian nor gentoo seems to deal with the issue. Any
advice here is most welcome.

Re-built package using the debian tarball as upstream tarball. The debian
tarball does not include non-free files:

httptunnel-3.3/doc/rfc1945.txt 
httptunnel-3.3/doc/rfc2068.txt 
httptunnel-3.3/doc/rfc2045.txt 

See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=393374

iconv is provided by glibc-common, which is part of the base packages one can
always assume to be present. Replaced iconv lines with more elegant function and
moved it to %prep

Package doesn't require dos2unix anymore.

I think that's it. 

Updated package:
http://sindrepb.fedorapeople.org/packages/httptunnel.spec
http://sindrepb.fedorapeople.org/packages/httptunnel-3.3-4.fc8.src.rpm
Comment 11 manuel wolfshant 2007-12-11 07:37:40 EST
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: x86_64
 [x] Rpmlint output:
$ rpmlint httptunnel-3.3-4.fc9.src.rpm
httptunnel.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 24)
 empty output for the binary package
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:GPLv2
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     SHA1SUM of package: d538fdf35a0dfd5f3164fa79a1a156c912d6da62 
/tmp/httptunnel_3.3+dfsg.orig.tar.gz
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.
=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on:devel/x86_64
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:i386 and x86_64
 [x] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [-] File based requires are sane.



================
*** APPROVED ***
================
Before commit, please try to do the cosmetic change identified by rpmlint
                                                                            
Comment 12 manuel wolfshant 2008-01-07 16:01:27 EST
ping ?
Comment 13 Sindre Pedersen Bjørdal 2008-01-07 19:09:38 EST
New Package CVS Request
=======================
Package Name: httptunnel
Short Description: Tunnels a data stream in HTTP requests
Owners: foolish@guezz.net
Branches: F-7, F-8
InitialCC: 
Cvsextras Commits: yes
Comment 14 Kevin Fenzi 2008-01-07 22:02:40 EST
Please use fedora account system names instead of email addresses. 

cvs done.

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