Bug 472658

Summary: Review Request: shmpps - Shared Memory driver for PPS signals
Product: [Fedora] Fedora Reporter: Chris Adams <linux>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: davids, fedora-package-review, mlichvar, notting, tcallawa
Target Milestone: ---Flags: j: fedora-review+
j: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 1.03-1.el5 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-08-25 20:54:36 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Chris Adams 2008-11-23 01:15:48 UTC
Spec URL: http://cmadams.fedorapeople.org/shmpps/shmpps.spec
SRPM URL: http://cmadams.fedorapeople.org/shmpps/shmpps-1.03-1.fc9.src.rpm
Description: 
SHM driver to allow PPS time sources to work without a PPS kernel.  The
PPS pin must be connected to the declared signal pin on a serial or
parallel port.

Comment 1 Jason Tibbitts 2009-07-10 17:33:29 UTC
URLs are 404 now.  Sorry for nobody having responded over the past eight months; I'm trying to put some effort into these old review tickets.  Unfortunately most folks don't really want to review something they don't understand, and I have to say that I don't understand the package summary.  I suspect that's why this ticket hasn't been looked at yet.

In any case, I'll try to take a look at this if I can get working links to the spec and source package.

Comment 2 Chris Adams 2009-07-10 18:35:39 UTC
Hmm, not sure why my files were deleted from fedorapeople.org. :-(  Anyway, it is back now.

SHM driver == IPC shared memory driver with ntpd (Network Time Protocol daemon)
PPS == pulse-per-second high-resolution time signal

Thanks for looking at this.  It is a pretty bare-bones package; the only documentation is in the source code, although I documented all the options in the sysconfig file.

Comment 3 Jason Tibbitts 2009-07-14 02:07:05 UTC
It would be nice to get those explanations into the package %description; enlightening me helps (well, theoretically), but enlightening everyone helps more.

Builds fine; rpmlint says:
  shmpps.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 27)
Not a big deal; fix it if you like.

  shmpps.x86_64: W: no-documentation
Not a problem.

  shmpps.x86_64: W: incoherent-subsys /etc/rc.d/init.d/shmpps $prog
rpmlint doesn't understand many common idioms in initscripts; you should ignore this.

I don't see any value in using "%{__install}" instead of just "install", but if you really want to use macros, you need to use %{__rm} and indeed %{__install_p}.

I'm afraid I don't understand the licensing here.  Which of the three files in the binary rpm are public domain?  Two of the files are supplied by you, and one is a mix of PD and one of the four different licenses the NTP code is under, so I can't see how the resulting binary could possibly be PD.  

It looks to me like the time_shm and the attach_shm code come from refclock_shm.c, which according to the NTP spec file is under the MIT license, so I think the resulting binary is MIT licensed and nothing in the final package is PD.  I've blocked FE-Legal for a double-check, however.

* source files match upstream.  sha256sum:          
   0c48634e3a5068fb69def61daae44656a5a4cd36f537e0c12b893806df6c5598  shmpps.tar
* package meets naming and versioning guidelines.
X specfile does not use macros consistently.
* summary is OK.                                                              
X description could use some elaboration.
* dist tag is present.
* build root is OK.
X license field does not seem to match the actual license.
* license is open source-compatible.
* license text not included upstream.
* latest version is being packaged.
* BuildRequires are proper (none).
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
   config(shmpps) = 1.03-1.fc12
   shmpps = 1.03-1.fc12
   shmpps(x86-64) = 1.03-1.fc12
  =
   /bin/sh
   /sbin/chkconfig
   /sbin/service
   config(shmpps) = 1.03-1.fc12
   ntp

* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* scriptlets are OK (service registration).
* code, not content.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

The package review process needs reviewers!  If you haven't done any package
reviews recently, please consider doing one.

Comment 4 Chris Adams 2009-07-16 01:39:41 UTC
I'll admit I wasn't entirely clear on the licensing.  The C source file says:

 * By: David J. Schwartz
 * <davids>
 * Version 1.03
 *
 * Put into the public domain. Please keep the acknowledgement above
 * intact. Portions taken from the NTP source (the time_shm structure
 * and attach_shm code) are covered by the NTP copyright/license

Does including a structure and one function mean the whole thing falls under the NTP license (I looked at the NTP source and that file is MIT licensed)?  I wasn't sure; that's why I left it as claimed above (but I'm not a lawyer and I suspect the above author is not either).

I'll get the other things cleaned up pending a licensing review.  How does this look for a description:

===
Shared memory (SHM) driver to allow high-resolution pulse-per-second
(PPS) time sources to be used with a Network Time Protocol (NTP) server
without a PPS-enabled kernel.  The PPS pin of the time source must be 
connected to the declared signal pin on a serial or parallel port.

See /etc/sysconfig/shmpps for configuration.
===

And yes, I need to get more involved in the reviews (haven't done one yet).

Comment 5 Jason Tibbitts 2009-07-29 19:01:52 UTC
Not sure if this is waiting on me; I'd say the above description is fine but I think the legal folks still need to make the determination about the license.  spot will probably get to it when he has a free minute.

Comment 6 Tom "spot" Callaway 2009-08-06 14:24:32 UTC
Hmm.... that really isn't the proper way to put something into the public domain.

Even though he isn't stating copyright, he isn't explicitly disclaiming it either, and in the US, you automatically gain copyright on a work that you create.

Can you ask David Schwartz to make the following statement:

2009.08.06: I hereby place all of my code in the shmpps package (in particular, shmpps.tar, with MD5 checksum [[INSERT_CHECKSUM]]) into the public domain. I disclaim all copyright on my code. Portions taken from the NTP source (the time_shm structure and attach_shm code) are still covered by the NTP copyright/license.

That would make it much clearer. He can do it via email, although, it would be better if he did so on the project webpage.

Alternately, if he doesn't want to do so, he could simply use the MIT license, as that is extremely permissive and already compatible with the NTP code.

As to the labeling of the License tag, you could either list it as you have done (which is correct), or simply use the MIT license (because those are the only terms you have to comply with).

Comment 7 David J. Schwartz 2009-08-09 00:31:12 UTC
I am the author of the PPS-SHM interface code. Portions are taken from the NTP code and are still covered by the NTP copyright/license. Everything not taken from the NTP code was authored by me. I placed that code into the public domain and disclaimed copyright via public statements at the time. (I believe they went to comp.protocols.ntp, but I can't recall for certain.)

I disclaim copyright on the PPS-SHM code.

Comment 8 Chris Adams 2009-08-09 01:28:17 UTC
I have updated the spec with the updated description, and I believe the licensing bits are cleared up now.

Spec URL: http://cmadams.fedorapeople.org/shmpps/shmpps.spec
SRPM URL: http://cmadams.fedorapeople.org/shmpps/shmpps-1.03-1.fc11.src.rpm
Description: 
Shared memory (SHM) driver to allow high-resolution pulse-per-second
(PPS) time sources to be used with a Network Time Protocol (NTP) server
without a PPS-enabled kernel.  The PPS pin of the time source must be
connected to the declared signal pin on a serial or parallel port.

See /etc/sysconfig/shmpps for configuration.

Comment 9 Tom "spot" Callaway 2009-08-09 04:38:42 UTC
Thanks David. Lifting FE-Legal.

Comment 10 Jason Tibbitts 2009-08-19 18:33:15 UTC
Looks good to me; still builds fine, and the issues I found are fixed.

APPROVED

Comment 11 Chris Adams 2009-08-20 14:07:45 UTC
New Package CVS Request
=======================
Package Name: shmpps
Short Description: Shared Memory driver for PPS time signals
Owners: cmadams
Branches: F-10 F-11 EL-5
InitialCC:

Comment 12 Jason Tibbitts 2009-08-20 17:39:40 UTC
CVS done.

Comment 13 Fedora Update System 2009-08-20 18:22:41 UTC
shmpps-1.03-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/shmpps-1.03-1.fc10

Comment 14 Fedora Update System 2009-08-20 18:22:47 UTC
shmpps-1.03-1.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/shmpps-1.03-1.el5

Comment 15 Fedora Update System 2009-08-20 18:22:53 UTC
shmpps-1.03-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/shmpps-1.03-1.fc11

Comment 16 Chris Adams 2009-08-20 18:56:03 UTC
Packages submitted to updates.  Thanks everybody!

Comment 17 Fedora Update System 2009-08-22 00:59:02 UTC
shmpps-1.03-1.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update shmpps'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-8841

Comment 18 Fedora Update System 2009-08-22 01:06:26 UTC
shmpps-1.03-1.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update shmpps'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-8881

Comment 19 Tom "spot" Callaway 2009-08-22 15:23:23 UTC
Just as an FYI, it generally doesn't make much sense to push new packages into testing, as it is very unlikely that anyone will test a new package. You should be safe to simply push them to stable.

Comment 20 Fedora Update System 2009-08-25 04:29:37 UTC
shmpps-1.03-1.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update shmpps'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2009-0329

Comment 21 Chris Adams 2009-08-25 20:54:36 UTC
(In reply to comment #19)
> Just as an FYI, it generally doesn't make much sense to push new packages into
> testing, as it is very unlikely that anyone will test a new package. You should
> be safe to simply push them to stable.  

Oops, I meant to change that.  I thought it was considered okay to push a new package (especially something small like this) directly to stable for Fedora, and I went digging in the wiki to see if there was a policy for EPEL (didn't really find anything definite for either Fedora or EPEL), and then forgot to change it.

Comment 22 Fedora Update System 2009-08-27 02:11:19 UTC
shmpps-1.03-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2009-08-27 02:14:35 UTC
shmpps-1.03-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2009-09-10 22:55:51 UTC
shmpps-1.03-1.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.