Bug 225735

Summary: Merge Review: ethtool
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: manuel wolfshant <wolfy>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: jgarzik, redhat-bugzilla, ruben, sven, tcallawa
Target Milestone: ---Flags: wolfy: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-01-17 13:41:52 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Bug Depends On:    
Bug Blocks: 426387    

Description Nobody's working on this, feel free to take it 2007-01-31 18:34:02 UTC
Fedora Merge Review: ethtool

Initial Owner: jgarzik@redhat.com

Comment 1 Ruben Kerkhof 2007-02-03 18:32:37 UTC
* RPM name is OK
* Source ethtool-5.tar.gz is the same as upstream
* This is the latest version
* Builds fine in mock
* File list looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* Spec file: some paths are not replaced with RPM macros
  (wiki: QAChecklist item 7)
* The %makeinstall macro should not be used
  (wiki: PackagingGuidelines#MakeInstall)

Rpmlint is not clean:

[ruben@odin ethtool]$ rpmlint -i ethtool-5-1.fc6.i386.rpm 
W: ethtool symlink-should-be-relative /usr/sbin/ethtool /sbin/ethtool
Absolute symlinks are problematic eg. when working with chroot environments.

Comment 2 Robert Scheck 2008-01-02 19:23:42 UTC
I [cw]ould maintain ethtool for Fedora - but I think, I can't for RHEL ;-) If 
somebody from Red Hat comes up for RHEL, please let me know, whether he or she
wants or not wants to maintain ethtool in Fedora as well...

Comment 3 Robert Scheck 2009-01-13 21:43:24 UTC
Jeff, ping? Can you please follow the review and perform the suggested changes?

Comment 5 Robert Scheck 2009-01-15 21:25:02 UTC
I signed up for co-maintainer as Jeff seems to be AWOL. Looks like he's 
maybe a better upstream maintainer rather a downstream one - sorry Jeff.
When looking around in Red Hat Bugzilla for open bug reports, I also had
a look to upstream version control system in order to gather maybe some
patches solving mentioned issues or even referenced fixes being not in
Fedora: git clone git://git.kernel.org/pub/scm/network/ethtool/ethtool.git

Unluckily, I've now to add a FE_LEGAL blocker as Jeff was somehow so nice
to remove *any* hint how ethtool is now licensed after version 6. And the
only license hint in this clone seems to be just the man page - which is
referring in a comment, that the man page itself (and just the man page)
is licensed as "GNU Public License." - could be a mislabeled GPL. So: Tom, 
your turn first before continuing with something else in this review.

Comment 6 Jeff Garzik 2009-01-15 21:33:03 UTC
It's not missing licensing at all.

The bog-standard COPYING file is present in every release tarball, including the one in each Fedora and RHEL source rpm.

And autogen.sh installs COPYING locally via automake if you are a developer building the git repo.

It is 100% standard GNU autotools setup including license.

Comment 7 Sven Lankes 2009-01-15 22:08:41 UTC
So depending on the Version of automake I'm using to build (the package from vcs) I get a different license (GPLv2+ vs. GPLV3+)?

Can I change the license at random by building it using evilmake, my very own automake-fork?

Comment 8 Robert Scheck 2009-01-15 23:09:40 UTC
Following from Freenode #fedora-devel (time is UTC+01:00):

[23:19:32] < spot> rsc: jgarzik is going to add a LICENSE file to git
                   that states the licensing

Comment 9 Robert Scheck 2009-01-17 11:27:18 UTC
Tom has clarified, that the legal points are solved for Fedora, removing
the blocker.

--- snipp ---
Date: Fri, 16 Jan 2009 09:53:41 -0500
From: Tom spot Callaway
To: Robert Scheck
Cc: Sven Lankes, Jeff Garzik
Subject: Re: COPYING for ethtool

On Fri, 2009-01-16 at 15:50 +0100, Robert Scheck wrote:
> On Fri, 16 Jan 2009, Tom spot Callaway wrote:
> > As Jeff has pointed out, he's not inserting COPYING, the autotool stack
> > is. This is one main reason why we cannot go off the version it uses.
> > GPLv3 COPYING gets stuck in all sorts of things that are not GPLv3 as a
> > result.
> That's wrong. It's caused, because he's putting no COPYING there in GIT
> already and thus the --add-missing at autotools are putting that into. As
> far as I know, an existing COPYING gets not overwritten except with use
> of maybe --force (which can be overwritten by IIRC --foreign).

Well, feel free to try to convince him. We don't need this additional
step for Fedora licensing.

--- snapp ---

To continue the review as co-maintainer, I've made multiple changes to the
package, I'm suggesting the following for formal review. Update to git seems
to be necessary for me as it solves some bug reports and feature requests. In
order to make me and Sven happy, I've solved the COPYING thing downstream.

Spec URL: http://labs.linuxnetz.de/bugzilla/ethtool.spec
SRPM URL: http://labs.linuxnetz.de/bugzilla/ethtool-6-2.20090115git.src.rpm

Sven, will you go for the formal review?

Comment 10 manuel wolfshant 2009-01-17 11:55:09 UTC
Package Review

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

 [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: devel/x86_64
 [x] Rpmlint output:
source RPM: empty
binary RPM:empty
 [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 match the upstream source, as provided in the spec URL.
     SHA1SUM of package:
e2cc807b553da0f7df337638aeb4319afd6f48db  ethtool-6.tar.gz
Note: this is a snapshot version
 [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.
 [x] 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 $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.
 [x] Final provides and requires are sane.

 [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: koji
 [x] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.
 [x] %check is present and the test passes.

=== Issues ===
1. Timestamps are not preserved
2. autoconf is already brought in by automake so it's not really needed to BR it

*** APPROVED ***

Comment 11 manuel wolfshant 2009-01-17 11:56:05 UTC
Comment #10 was meant for the src.rpm from #9...

Comment 12 Robert Scheck 2009-01-17 13:41:52 UTC
Manuel, thanks for doing the review. As we've spoken in IRC, preserving
timestamps is not supported in GIT. Seems that old-style CVS and SVN have
a useful feature more at that point rather the cool GIT... ;-)

I've commited my updates into CVS, tagged and built packages from it.

Comment 13 Fedora Update System 2009-01-17 13:43:10 UTC
ethtool-6-2.20090115git.fc9 has been submitted as an update for Fedora 9.

Comment 14 Fedora Update System 2009-01-17 13:43:12 UTC
ethtool-6-2.20090115git.fc10 has been submitted as an update for Fedora 10.