Bug 378791 - Review Request: netdump-server - netdump crash recovery capture server
Review Request: netdump-server - netdump crash recovery capture server
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks: 248013
  Show dependency treegraph
 
Reported: 2007-11-12 16:23 EST by Neil Horman
Modified: 2012-01-13 07:22 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-01-13 07:22:20 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tcallawa: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Neil Horman 2007-11-12 16:23:16 EST
Spec URL: http://people.redhat.com/nhorman/rpms/netdump-server.spec
SRPM URL: http://people.redhat.com/nhorman/rpms/netdump-server-0.7.16-15.fc8.src.rpm
Description: netdump-server is teh capture server for netdump core recovery clients.  It is teh primary memory image capture tool used in a crash post-mortem analysis for pre-RHEL-5 /FC-6 based systems


I'd like to get this reviewed please, for inclusion in Extras and EPEL.
Comment 1 Jason Tibbitts 2007-11-17 22:23:14 EST
Some comments:

This fails to build for me due to a missing dependency on popt-devel.  Adding
that gets it building.

Your BuildRoot: is incorrect; see http://fedoraproject.org/wiki/Packaging/Guidelines

If you call useradd in %pre, you need to have Requires(pre): shadow-utils.  You
can't just Require: it.  Probably best to follow the established guidelines for
adding users/groups: http://fedoraproject.org/wiki/Packaging/UsersAndGroups

License tag need to specify the version of the GPL which applies; see
http://fedoraproject.org/wiki/Licensing

Is there really no upstream source for this package?

Let's go over the rpmlint output:

  netdump-server.x86_64: W: spurious-executable-perm 
   /usr/share/doc/netdump-server-0.7.16/example_scripts/netdump-reboot
  netdump-server.x86_64: W: spurious-executable-perm 
   /usr/share/doc/netdump-server-0.7.16/example_scripts/netdump-nospace
  netdump-server.x86_64: W: spurious-executable-perm 
   /usr/share/doc/netdump-server-0.7.16/example_scripts/netdump-crash
  netdump-server.x86_64: W: spurious-executable-perm 
   /usr/share/doc/netdump-server-0.7.16/example_scripts/netdump-start
Generally documentation shouldn't be executable.

  netdump-server.x86_64: E: zero-length /var/crash/.ssh/authorized_keys2
Since comments are valid in that file, it might be nice to at least include one
indicating what's supposed to go there.  

  netdump-server.x86_64: E: non-readable /var/crash/.ssh/authorized_keys2 0600
This is OK; the file isn't supposed to be public.

  netdump-server.x86_64: W: hidden-file-or-dir /var/crash/.ssh
This is OK; it's not as if you have a choice of what to name the .ssh directory.

  netdump-server.x86_64: W: incoherent-version-in-changelog -0.7.16-15 
   0.7.16-15.fc9
A space between the dash and the version should quiet this.  See the Changelogs
section of http://fedoraproject.org/wiki/Packaging/Guidelines

  netdump-server.x86_64: W: invalid-license GPL
Need to specify GPL version.

  netdump-server.x86_64: W: no-url-tag
Depends on whether there's really an upstream for this package.

The rest are all non-standard-{uid,gid} complaints, which are OK in this case.
Comment 2 Neil Horman 2007-11-20 13:40:16 EST
Spec URL: http://people.redhat.com/nhorman/rpms/netdump-server.spec
SRPM URL: http://people.redhat.com/nhorman/rpms/netdump-server-0.7.16-16.fc8.src.rpm

new package available

"Your BuildRoot: is incorrect"
fixed

"call useradd in %pre..."
fixed

"License tag need to specify the version of the GPL"
fixed

"Is there really no upstream source for this package?"
Nope.  Netdump never made it upstream, nor did the server component

"spurious-executable-perm "
files removed.  They arent needed/helpful

"zero-length /var/crash/.ssh/authorized_keys2"
commdent added to file
"incoherent-version-in-changelog"
should be fixed


Comment 3 Jason Tibbitts 2007-11-21 16:38:13 EST
> Nope.  Netdump never made it upstream, nor did the server component

Then please see the "We are Upstream" section of
http://fedoraproject.org/wiki/Packaging/SourceURL

Unfortunately the -16 release still fails to build for me.  Here's a scratch build:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=253474
Scratch builds are really easy to make:
  koji build --scratch dist-f9 netdump-server-0.7.16-16.fc8.src.rpm

If adding a build dependency on popt-devel isn't the way you want to solve this,
then what should be done?
Comment 4 Neil Horman 2007-11-26 08:45:14 EST
thanks, I built locally  in amock bukildroot that already had popt-devel.  sorry
I missed the dep.  new pkg available with needed fixes:
Spec URL: http://people.redhat.com/nhorman/rpms/netdump-server.spec
SRPM URL: http://people.redhat.com/nhorman/rpms/netdump-server-0.7.16-17.fc8.src.rpm
Comment 5 Jason Tibbitts 2007-12-02 20:14:59 EST
OK, this one builds for me, but some issues remain.  This has turned out to be a
bit more complicated than I thought it would be.

rpmlint still has the same complaint:
  netdump-server.x86_64: W: incoherent-version-in-changelog -0.7.16-17 
   0.7.16-17.fc9
This is due to the lack of a space between the dash and the version.  I know
it's nitpicking, but
http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs is rather specific
and we do have code which parses changelog entries.

Looking more closely at your scriptlets, I see that you're restarting the
service in %post while the recommended scriptlets at
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#services do it in
%postun.  I'll admit that I don't fully understand why, and I'm no fan of
blindly following arcane knowledge, but there must be some reason for the more
complicated version.  I'm trying to figure that out now.

The Summary: is a little confusing; it says "Client setup..." but the package is
netdump-server.

I don't see anything in the package which indicates that indicates the GPL
version.  Your spec says GPLv2 but I don't see where that comes from.

Looking at the build process, I see that the compiler isn't being called with
the proper set of flags.  -g is there so at least the debuginfo looks OK, but
the rest of the flags aren't.  A simple override of CFLAGS on the make line
doesn't work, but overriding DEBUG_FLAGS does:
  make DEBUG_FLAGS="%{optflags}" %{?_smp_mflags}

I note that /var/crash is owned in rawhide by kexec-tools.  Normally this
wouldn't be a significant problem, but kexec-tools has the directory owned by
root while this package has it owned by netdump:netdump.  So at minimum this
package needs to conflict with kexec-tools (and the reverse), but perhaps it
would be prudent to consider having this package use something other than
/var/crash.


* No upstream to compare source files against.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
? summary is somewhat confusing.
* description is OK.
* dist tag is present.
* build root is OK.
? license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper.
X compiler flags are not correct.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly
* debuginfo package looks complete.
X rpmlint still has a valid complaint.
* final provides and requires are sane:
   config(netdump-server) = 0.7.16-17.fc9
   netdump-server = 0.7.16-17.fc9
     =
   /bin/sh
   /sbin/ifconfig
   /usr/bin/ssh
   /usr/bin/ssh-keygen
   config(netdump-server) = 0.7.16-17.fc9
   fileutils
   gawk
   libglib-1.2.so.0()(64bit)
   libpopt.so.0()(64bit)
   libpopt.so.0(LIBPOPT_0)(64bit)
   shadow-utils
   textutils

* %check is not present; no test suite upstream.  I have no means to test this.
* no shared libraries are added to the regular linker search paths.
? /var/crash ownership causes some issues.
*  no duplicates in %files.
* file permissions are appropriate.
? service restart scriptlet is not the recommended one.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.
Comment 6 Neil Horman 2007-12-03 13:11:49 EST
ok, new version available:
Spec URL: http://people.redhat.com/nhorman/rpms/netdump-server.spec
SRPM URL: http://people.redhat.com/nhorman/rpms/netdump-server-0.7.16-18.fc8.src.rpm

>netdump-server.x86_64: W: incoherent-version-in-changelog -0.7.16-17
Fixed


>Looking more closely at your scriptlets, I see that you're restarting the
service in %post while the recommended scriptlets at...

I think the %post condrestart is vestigual.  I've added an identical %postun
scriptles to be compliant.  I'll remove the %post script at a later date, when I
can confirm that its no longer needed.

>The Summary: is a little confusing
Fixed

>I don't see anything in the package which indicates that indicates the GPL
version
See COPYING in the docs.  Its included in the srpm

>Looking at the build process, I see that the compiler isn't being called with
the proper set of flags
Fixed

>I note that /var/crash is owned in rawhide by kexec-tools
I've moved this package to write to /var/netdump/.  We need to allow this
package and kexec-tools to co-exist.
Comment 7 Jason Tibbitts 2007-12-03 14:19:37 EST
I'll look more closely after work, but I can note now that the COPYING file is
simply the usual GPL notice and does not indicate which version of the GPL the
sources are under.  See 
http://fedoraproject.org/wiki/Licensing and specifically the statement:

"
A GPL or LGPL licensed package that lacks any statement of what version that
it's licensed under in the source code/program output/accompanying docs is
technically licensed under *any* version of the GPL or LGPL, not just the
version in whatever COPYING file they include.
"

So what I'm not finding is any statement in the source or documentation
indicating just what version of the GPL this program is under.  Or at least I
hadn't, but now I see that netdump-server.8 includes comments (not visible in
the formatted output) with a proper GPL notice saying "version 2 or later".  So
if that's to believed, then the proper License: tag should contain "GPLv2+". 
But honestly that's not the best place for the only license notice to go.
Comment 8 Jason Tibbitts 2007-12-04 01:27:44 EST
OK, your move to /var/netdump has elicited the following from rpmlint:
  netdump-server.x86_64: W: non-standard-dir-in-var netdump
which I don't suppose is problematic.  Personally I'd think /var/lib/netdump
would be better, but if /var/crash is there and this stores the same data then I
don't see why it shouldn't be in the same place.

The %postun scriptlet looks proper but there's certainly no reason to restart
the service twice on upgrade.  Currently you're doing a condrestart of the
service in %post at install time ($1 == 1) and at upgrade time ($1 == 2), and
then at upgrade time in %postun.  Honestly, I can't see what the point of that
is; just restart it in %postun like any other service and be done with it.

At this point I think the package is fine save for two issues:

License: tag.  Please try to determine whether this is GPLv2 or GPLv2+ (or
something else) and then set the License: tag appropriately.  Judging from the
only statement anywhere in the package (in comments at the start of
netdump-server.8) I'd say GPLv2+ is proper.

Scriptlets.  Please just remove the condrestart call from %post.  I really can't
envision a reason for the way it is now.  Or, if you'd rather not, we can get a
third party to comment.
Comment 9 Neil Horman 2007-12-05 11:29:52 EST
Spec URL: http://people.redhat.com/nhorman/rpms/netdump-server.spec
SRPM URL: http://people.redhat.com/nhorman/rpms/netdump-server-0.7.16-19.fc8.src.rpm


I fixed up the %post script appropriately.

As for the license, the COPYING file which is included with both the SRPM and
the RPM clearly indicates that the distribution is under the terms of strict GPLv2
Comment 10 Tom "spot" Callaway 2007-12-11 14:34:42 EST
Neil,

The GPL is unique, in that it explicitly says:

"If the Program does not specify a version number of
this License, you may choose any version ever published by the Free Software
Foundation."

Thus, we cannot go off of the versioning in COPYING, we need to look at what the
code itself says. (This is why it is important to include license/copyright
information in code headers).

Most of the code in this package has no license or copyright attribution, but
one of the headers (netconsole.h) specifies GPLv2+, so go with GPLv2+ for the
license tag.

Comment 11 Neil Horman 2007-12-11 15:07:33 EST
No.

The one header that includes copyright information specifies that we can use a
later version than GPLv2 at my option.  I'm opting not to.  The COPYING file
indicates GPLv2, thats what I'm going with.
Comment 12 Tom "spot" Callaway 2007-12-11 15:31:05 EST
That is simply wrong. Blocking this package until the License tag is corrected.
Comment 18 Jason Tibbitts 2007-12-11 18:14:03 EST
So I'm told that in bug 248013, which I cannot access but which I see has now
been closed, Neil indicated that he would not make the change and that he would
rather abandon this package than make the change.  For my part, I simply cannot
approve the package with an incorrect License: tag.

So I guess this ticket should be closed.  If someone comes along that doesn't
object to the single '+' and wishes to take over this submission, I will be
happy to approve it with that change.
Comment 19 Neil Horman 2007-12-11 18:43:12 EST
you were told wrong.  

I closed the bug you reference as wonfix, because I won't change the license
without more authority than that which tom represents.  I'll happily reopen it
in the event that Tom comes around on this issue, or a legal authority indicates
that its ok to change (which I've sent a private email off to request).  I
specifically left this bug open to handle that resolution.  Your assumption that
the License tag is currently incorrect is in question, and waiting for a legal
opinion to resolve.

Comment 20 Tom "spot" Callaway 2007-12-11 22:53:04 EST
You're welcome to wait for a lawyer to talk with you on this issue, but the
Fedora licensing policies are very clear on this issue. COPYING is trumped by
what the code says.

Alternately, you might consider asking Ingo Molnar what the license should be,
since he wrote that code.
Comment 21 Neil Horman 2007-12-12 08:32:12 EST
Mingo isn't the only one (although I'm getting hold of him as well).  Of the
three places you have noted the possibility of a GPLv2+ option:

2 are not code at all, but rather man pages, documentation no different from a
COPYING file.  These are not written by Ingo, but rather the project author,
Alex Larson, who is no longer here.  Of these two man pages, the netdump.8 man
page is no longer relevant to the package and should be removed (given that the
functionality it documents is no longer present)

1 header file , whos contents are borrowed from the kernel netdump.h header
file, which is covered strictly by GPLv2.

Clearly this issue is not as cut and dry as you would like to make it out to be. 
Comment 22 Tom "spot" Callaway 2007-12-12 09:21:29 EST
The header file in question is CLEARLY marked as GPLv2 or later.

The GPLv2 or later license is the ONLY license that appears in the source. The
text of the GPL says that if a version is specified, then that is the version of
the license to be used.

The version specified is GPLv2 or later. COPYING does NOT trump this.

I honestly still cannot believe we're having this conversation.
Comment 23 Neil Horman 2007-12-12 09:57:17 EST
"The version specified is GPLv2 or later. COPYING does NOT trump this."

So I've copied you on the email I sent to red hat legal regarding this, as well
as on the note to Ingo requesting clarification on his license (given that its
included in a project listed as GPLv2 only).  I'll wait to see what they say.

If you can point me to a documentation from some legal authority that clearly
states the COPYING file is irrelevant when a conflicting statement is contained
in a piece of code, I'll consider changing it.  You had mentioned that you had
discussed this at length with Mark W., I imagine thats written down somewhere.
Comment 24 Neil Horman 2007-12-12 11:29:12 EST
Spec URL: http://people.redhat.com/nhorman/rpms/netdump-server.spec
SRPM URL: http://people.redhat.com/nhorman/rpms/netdump-server-0.7.16-20.fc8.src.rpm

As per the legal response to this issue, I've posted an update to this rpm. 
Same as previous, with additions to all the files to make them unambiguously GPLv2.
Comment 25 Tom "spot" Callaway 2007-12-12 12:46:52 EST
License matches the code now. Setting this review to +.
Comment 26 Jason Tibbitts 2007-12-12 12:50:09 EST
I concurr.

APPROVED
Comment 27 Neil Horman 2007-12-13 07:43:21 EST
New Package CVS Request
=======================
Package Name: netdump-server
Short Description:  Server for network kernel message logging and crash dumps
Owners: nhorman@redhat.com
Branches: EL-5
Comment 28 Kevin Fenzi 2007-12-13 14:11:05 EST
cvs done.
Comment 29 Jason Tibbitts 2007-12-13 19:50:25 EST
I just happened to catch something odd which I didn't notice before: a bunch of
  cc: unrecognized option '-j8'
messages, arising from this:
  %build
  export CFLAGS="%{optflags} %{?_smp_mflags} `glib-config --cflags`"; make
%{?_smp_mflags} should be passed to make, not inserted into CFLAGS, like so:
  export CFLAGS="%{optflags} `glib-config --cflags`"; make %{?smp_mflags}
Comment 30 Neil Horman 2007-12-14 09:15:57 EST
yeah, I just saw that too, EL-5 also seems to have some issues with glib-config
too that should be fixed in a separate bug.  I'll take care of them shortly.
Comment 31 Jason Tibbitts 2008-01-08 19:24:16 EST
Can we close this now?
Comment 32 Neil Horman 2008-01-09 09:35:18 EST
yep, all done.  Thanks
Comment 33 Alexander Kurtakov 2012-01-13 07:22:20 EST
Closing.

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