Bug 505259 - (python-utmp) Review Request: python-utmp - Python modules for utmp records
Review Request: python-utmp - Python modules for utmp records
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
http://kassiopeia.juls.savba.sk/~gara...
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-11 04:53 EDT by Juha Tuomala
Modified: 2010-04-21 08:15 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-04-21 08:15:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Juha Tuomala 2009-06-11 04:53:02 EDT
Spec URL: http://tuju.fi/fedora/11/python-utmp.spec
SRPM URL: http://tuju.fi/fedora/11/python-utmp-0.7-1.fc10.src.rpm
Description:
python-utmp consists of three modules, providing access to utmp records.
It is quite difficult to access utmp record portably, because every UNIX
has different structure of utmp files. Currently, python-utmp works on
platforms which provide getutent, getutid, getutline, pututline,
setutent, endutent and utmpname functions (such as GNU systems
(Linux and hurd) and System V unices) and on BSD systems using
simple utmp structure.
Comment 1 Juha Tuomala 2009-06-11 04:55:57 EDT
$ rpmlint python-utmp.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

f10 http://koji.fedoraproject.org/koji/taskinfo?taskID=1404809
f11 http://koji.fedoraproject.org/koji/taskinfo?taskID=1404815
f12 http://koji.fedoraproject.org/koji/taskinfo?taskID=1404820
Comment 2 Juha Tuomala 2009-06-11 04:57:05 EDT
http://packages.qa.debian.org/p/python-utmp.html

Only fishy part in this package is the license, which also have given one error in debian side. It appears to be free enough, but doesn't follow any well known named license.
Comment 3 Parag AN(पराग) 2009-06-11 05:11:56 EDT
so how is this different from http://www.clapper.org/software/python/pyutmp/ ?

If pyutmp seems latest development module, can you replace python-utmp with pytmp here?
Comment 4 Juha Tuomala 2009-06-11 05:36:11 EDT
Good question, we have been using this one at work.
http://kassiopeia.juls.savba.sk/~garabik/software/python-utmp/README.txt
vs
http://www.clapper.org/software/python/pyutmp/epydoc/

For me it looks that python-utmp has richer API, including setting utmp records (pututline etc).
Comment 5 Jason Tibbitts 2009-06-27 03:14:25 EDT
You should rpm rpmlint on the source rpm and final build rpms, not against the spec file.  If you did that, you'd see:

python-utmp.x86_64: W: summary-ended-with-dot Python modules for umtp records.
python-utmp.x86_64: W: invalid-license Unlimited right to use, redistribute
python-utmp.x86_64: W: invalid-license modify.
python-utmp.x86_64: W: dangling-relative-symlink /usr/share/doc/python-utmp-0.7/COPYING debian/copyright
python-utmp-debuginfo.x86_64: W: invalid-license Unlimited right to use, redistribute
python-utmp-debuginfo.x86_64: W: invalid-license modify.

So:
Trim the dot from Summary:.

You cannot just invent a license tag (especially since the field has a syntax which you are violating); you must use an approved one from https://fedoraproject.org/wiki/Licensing and if one doesn't fit then you must contact the legal folks.  I'll let them know.

You need to actually install the text of the COPYING file, not just a symlink that goes nowhere.


Legal folks, here's the license text:

License loosely modelled after Just van Rossum's Python-Powered LOGO
license:

Radovan Garabik, hereafter referred to as THE AUTHOR, herewith grants
you (THE USER) the right to use python-utmp (THE SOFTWARE).

Blah blah blah blah. Blah NO WARRANTIES blah blah EXPLICIT blah blah
blah blah OR IMPLIED blah blah blah. Blah.

Blah blah Unlimited Redistribution and Modification of THE SOFTWARE by
THE USER is Allowed by THE AUTHOR blah blah blah for Any Purpose blah
blah blah blah blah blah blah.

By Using THE SOFTWARE you Agree with this LICENSE blah blah blah blah.
Blah. Blah. Blah blah.

If you do not Agree with the LICENSE you should Better NOT Use
THE SOFTWARE.
Comment 7 Tom "spot" Callaway 2009-07-06 10:49:36 EDT
That's the license? Seriously? I should block this package on the grounds that the license author is not acting even remotely responsibly.

However, if one does sed 's|blah||g' over that license, it looks like a Copyright only license, so just use:

# Copyright holder places no restrictions on use, modification, or redistribution
License: Copyright only

Lifting FE-Legal.
Comment 8 Juha Tuomala 2009-07-07 06:14:41 EDT
$ rpmlint -v /home/tuju/PKGS/SRPMS/python-utmp-0.7-3.fc10.src.rpm
python-utmp.src: I: checking
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

spec: http://tuju.fi/fedora/11/python-utmp.spec
src:  http://tuju.fi/fedora/11/python-utmp-0.7-3.fc10.src.rpm
f10:  http://koji.fedoraproject.org/koji/taskinfo?taskID=1458499
f11:  http://koji.fedoraproject.org/koji/taskinfo?taskID=1458504
Comment 9 Jason Tibbitts 2009-07-08 20:56:32 EDT
Indeed, builds fine for me and rpmlint is silent.

You need to be consistent in your use of $RPM_BUILD_ROOT versus %{buildroot}.  Either style is fine, but you must pick one.

You seen to use a %pyver macro that is not defined anywhere.  It doesn't seem to make much difference, though; maybe 'make install' doesn't actually care if you pass PYTHONVER at all.

%global is recommended over %define these days, although I don't think it matters for the situation where you're using it.

The C code isn't compiled with the correct set of compiler flags.  Fortunately -g is passed so the debuginfo package comes out correct, but you should pass %optflags (or $RPM_OPT_FLAGS if you prefer that form).  Unfortunately it looks like this package thinks it knows best and overrides CFLAGS, so you may have to patch Makefile.common.

Your manual python dependency is unnecessary; rpm will generate one.

* source files match upstream.  sha256sum:
   b411e012244a3433b250a0cd63ce50aaf74ab4ddda8c5ef37c3598ee05ff5f2c
   python-utmp_0.7.tar.gz
* package meets naming and versioning guidelines.
X specfile does not use macros consistently.
* summary is OK.                                                              
* description is OK.                                                          
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible (according to FE-Legal review).
* license text included in package.
* latest version is being packaged.
* 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.
* rpmlint is silent.
? final provides and requires (unnecessary manual python dependency):
   utmpaccessmodule.so()(64bit)
   python-utmp = 0.7-3.fc12
   python-utmp(x86-64) = 0.7-3.fc12
  =
?  python
   python(abi) = 2.6

* %check is not present; no test suite upstream.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* code, not content.
* documentation is small, so no -doc 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.

The package review process needs reviewers!  If you haven't done any package
reviews recently, please consider doing one.
Comment 10 Juha Tuomala 2009-07-21 11:46:35 EDT
(In reply to comment #9)
> You need to be consistent in your use of $RPM_BUILD_ROOT versus %{buildroot}. 

fixed.

> You seen to use a %pyver macro that is not defined anywhere.  It doesn't seem
> to make much difference, though; maybe 'make install' doesn't 
> actually care if you pass PYTHONVER at all.

fixed.

> %global is recommended over %define these days, although I don't think it
> matters for the situation where you're using it.

fixed. vim doesn't seem to recognize it, someone should file it somewhere.

> The C code isn't compiled with the correct set of compiler flags.  
> Fortunately -g is passed so the debuginfo package comes out correct
>, but you should pass %optflags (or $RPM_OPT_FLAGS if you prefer that form). 
> Unfortunately it looks like this package thinks it knows best and 
> overrides CFLAGS, so you may have to patch Makefile.common.

You mean that line:
  CFLAGS = -g -Wall -I$(PYTHONINCLUDE) $(DEFINES)

should include itself with
  CFLAGS := $(CFLAGS) -g -Wall -I$(PYTHONINCLUDE) $(DEFINES)
 
> Your manual python dependency is unnecessary; rpm will generate one.

fixed.


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

I look into it.

$ rpmlint --verbose /home/tuju/PKGS/SRPMS/python-utmp-0.7-4.fc10.src.rpm
python-utmp.src: I: checking
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1489773
src: http://tuju.fi/fedora/11/python-utmp-0.7-4.fc10.src.rpm
spec: http://tuju.fi/fedora/11/python-utmp.spec
Comment 11 Jason Tibbitts 2009-07-22 20:43:55 EDT
Everything looks good with the exception of the compiler flags.  You need to pass the proper flags to the compiler at %build time, so that the two calls to gcc have the proper flags.  It doesn't go any good to pass those flags to the "make install" call, because that just copies various files into place.
Comment 12 Juha Tuomala 2009-07-28 06:20:22 EDT
I made it work by having that part in spec:

%build
make -f Makefile.glibc \
    DEFINES=" \
        -D_HAVE_UT_SESSION -D_HAVE_UT_ADDR_V6 -D_HAVE_UT_EXIT \
        -D_HAVE_UT_HOST -D_HAVE_UT_ID -D_HAVE_UT_TV -D_HAVE_UT_USER \
        -D_HAVE_UTMPNAME -D_HAVE_SETUTENT -D_HAVE_GETUTENT -D_HAVE_ENDUTENT \
        -D_HAVE_GETUTID -D_HAVE_GETUTLINE -D_HAVE_PUTUTLINE \
        %{optflags}" \
    PYTHONVER=%{python_version}

Is that okay approach? I communicate with author so that he could modify the makefiles allowing simpler options without patching them.

f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1549469
f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1549509
src: http://tuju.fi/fedora/11/python-utmp-0.7-5.fc10.src.rpm
spec: http://tuju.fi/fedora/11/python-utmp.spec
Comment 13 Jason Tibbitts 2009-07-29 13:54:23 EDT
That is kind of a hack, but it does seem to work and the only other solution I see is to patch the makefile which isn't demonstrably cleaner.

So I'd say that things are OK now, but you should keep an eye on your build logs to make sure that this doesn't break in the future.

APPROVED
Comment 14 Juha Tuomala 2009-07-29 14:22:20 EDT
(In reply to comment #13)
> see is to patch the makefile which isn't demonstrably cleaner.

Agreed.

> So I'd say that things are OK now, but you should keep an eye on your build
> logs to make sure that this doesn't break in the future.

Yep, that's why i try to get upstream to enhance it.

> APPROVED  

Thank you for review.
Comment 15 Juha Tuomala 2009-07-29 14:24:26 EDT
New Package CVS Request
=======================
Package Name: pyhton-utmp
Short Description: Python modules for umtp records
Owners: tuju
Branches: F-10 F-11 EL-4 EL-5
InitialCC:
Comment 16 Jason Tibbitts 2009-07-29 14:43:01 EDT
CVS done.
Comment 17 Juha Tuomala 2009-08-02 06:47:56 EDT
% fedora-cvs python-utmp
Checking out python-utmp from fedora CVS as tuju:
cvs server: cannot find module `python-utmp' - ignored
cvs [checkout aborted]: cannot expand modules
Comment 18 Jason Tibbitts 2009-08-02 08:50:24 EDT
Ugh, I think if you look at the CVS request you made you'll see the problem: you asked for "phython-utmp" and I didn't catch it.  Please double-check your CVS requests.

Your module should be there now; I'll get the bad one cleaned up.
Comment 19 Juha Tuomala 2009-08-02 09:08:01 EDT
(In reply to comment #18)
> Ugh, I think if you look at the CVS request you made you'll see the problem:
> you asked for "phython-utmp" and I didn't catch it.  Please double-check your
> CVS requests.

No, I actually asked "pyhton-utmp", not "phython-utmp" nor "python-utmp". Yeah, that was the problem anyway. Works now great.
Comment 20 Jason Tibbitts 2009-11-05 17:48:02 EST
Is there any reason for this ticket to remain open?

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