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.
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.
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.
GPL is not a valid license tag anymore: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#head-f21ae23bf2f278444e2c385463cfa74a502396b8
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
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.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!
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?
*** Bug 251040 has been marked as a duplicate of this bug. ***
*** Bug 416391 has been marked as a duplicate of this bug. ***
> 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.
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
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
ping ?
New Package CVS Request ======================= Package Name: httptunnel Short Description: Tunnels a data stream in HTTP requests Owners: foolish Branches: F-7, F-8 InitialCC: Cvsextras Commits: yes
Please use fedora account system names instead of email addresses. cvs done.