Bug 526041 - Review Request: luci - Web-based cluster management application
Summary: Review Request: luci - Web-based cluster management application
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
(Show other bugs)
Version: rawhide
Hardware: All Linux
high
high
Target Milestone: ---
Assignee: Alan Pevec
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On: 468230
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-28 13:35 UTC by Ryan McCabe
Modified: 2012-12-18 17:56 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-12-18 17:56:34 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
apevec: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ryan McCabe 2009-09-28 13:35:08 UTC
Spec URL: http://people.redhat.com/rmccabe/luci/luci.spec
SRPM URL: http://people.redhat.com/rmccabe/luci/luci-0.20.0-1.fc11.src.rpm
Description: Luci is a web-based cluster administration application based on TurboGears 2.

Comment 1 Alan Pevec 2009-09-28 15:42:51 UTC
luci.src: E: no-description-tag

Please add some text to %description and repost spec/SRPM

Comment 2 Ryan McCabe 2009-09-28 15:55:05 UTC
Updated spec and srpm at the same URLs as above.

Comment 3 Alan Pevec 2009-09-28 16:27:14 UTC
Here is the review:

 +:ok, =:needs attention, -:needs fixing

MUST Items:
[+] MUST: rpmlint must be run on every package.

rpmlint luci-0.20.0-1.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
rpmlint luci-0.20.0-1.fc11.noarch.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license and meet
the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual
license.
[n/a] MUST: 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 must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL.

a67aa193e2b15a9106190f84aeb99b92  luci-0.20.0.tar.bz2

[+] MUST: The package must successfully compile and build into binary rpms on
at least one supported architecture.

[n/a] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch.
[+] MUST: All build dependencies must be listed in BuildRequires
[n/a] MUST: The spec file MUST handle locales properly. This is done by using
the %find_lang macro.
[n/a] MUST: Every binary RPM package which stores shared library files (not
just symlinks) in any of the dynamic linker's default paths, must call ldconfig
in %post and %postun.
[n/a] MUST: If the package is designed to be relocatable, the packager must
state this fact in the request for review
[+] MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every %files section must include a
%defattr(...) line.
[+] MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros, as described in the macros
section of Packaging Guidelines.
[+] MUST: The package must contain code, or permissible content. This is
described in detail in the code vs. content section of Packaging Guidelines.
[+] MUST: Large documentation files should go in a doc subpackage.
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[n/a] MUST: Header files must be in a -devel package.
[n/a] MUST: Static libraries must be in a -static package.
[n/a] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability).
[n/a] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), then library files that end in .so (without suffix) must go in
a -devel package.
[n/a] MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release}
[n/a] MUST: Packages must NOT contain any .la libtool archives, these should be
removed in the spec.
[n/a] MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

APPROVED

Comment 5 Ryan McCabe 2009-09-28 17:11:23 UTC
New Package CVS Request
=======================
Package Name: luci
Short Description: Web-based cluster administration application
Owners: rmccabe cfeist jpokorny
Branches: F-11
InitialCC: rmccabe

Comment 6 Rudolf Kastl 2009-09-29 09:39:24 UTC
wow... that was a quick review... but after quickly looking at the package i can see that the included readme references files that are non existant in the package. wouldnt it make sense to either exclude the readme or package the required files to test the turbogears application (they are existant in the tarball)?

Comment 7 Robert Scheck 2009-09-29 10:00:34 UTC
The %description is *very* short as well.

Comment 8 Christoph Wickert 2009-09-29 10:23:41 UTC
A few more comments:
- %description should end with a dot
- use %global instead of %define
https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
- license is completely unclear: Is this GPLv2 or GPLv2+? How about a COPYING file or license headers in the files?
- any reason not to use %defattr(-,root,root,-) as we usually do? Not that it really matters...
- timestamps don't match: Source0 in the srpm is 
28. Sep 15:27, while the one from the url is 15:33. Timestamps should match, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

/me knows he is pedantic, but we must not ship this until at least the license question is answered.

Comment 9 Alan Pevec 2009-09-29 14:04:30 UTC
(In reply to comment #6)
> wow... that was a quick review... but after quickly looking at the package i
> can see that the included readme references files that are non existant in the
> package. wouldnt it make sense to either exclude the readme or package the
> required files to test the turbogears application (they are existant in the
> tarball)?  

I guess we'll need some guideline how to properly RPMize TurboGears applications.
But strictly speaking this is not a blocking issue for the package review i.e. not MUST.

Comment 10 Alan Pevec 2009-09-29 14:09:44 UTC
(In reply to comment #7)
> The %description is *very* short as well.  

Still better than empty :)

Comment 11 Ryan McCabe 2009-09-29 14:28:40 UTC
The license is GPL version 2. I'll create a new package with a COPYING file so there's no confusion. I'll fix the README, too.

Comment 12 Alan Pevec 2009-09-29 14:45:50 UTC
(In reply to comment #8)
> - %description should end with a dot

Yes, that's usual, but I don't see it in guidelines: https://fedoraproject.org/wiki/Packaging/Guidelines#Summary_and_description
so I couldn't complain, it's not MUST

> - use %global instead of %define
> https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

it's a draft suggestion, not MUST

> - license is completely unclear: Is this GPLv2 or GPLv2+? How about a COPYING
> file or license headers in the files?

not all, but files under luci/lib/ do have clear license headers:
# This program is free software; you can redistribute
# it and/or modify it under the terms of version 2 of the
# GNU General Public License as published by the
# Free Software Foundation.

Also luci/templates/footer.html is clear:
Distributed under the <a href="http://creativecommons.org/licenses/GPL/2.0/">GNU GPL v2 license</a>.

So it's exactly GPLv2 as stated in spec, there isn't "or higher" clause to allow GPLv2+
I have already suggested off-line to the packager=upstream maintainer to include LICENSE file into tarball to make it completely clear.

> - any reason not to use %defattr(-,root,root,-) as we usually do? Not that it
> really matters...

yeah, setup.py creates correct permissions, but not blocking issue

> - timestamps don't match: Source0 in the srpm is 
> 28. Sep 15:27, while the one from the url is 15:33. Timestamps should match,
> see
> https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

md5sum is important which is fine, guideline says "consider"
 
> /me knows he is pedantic, but we must not ship this until at least the license
> question is answered.  

I don't see why do you think that GPLv2 license is not clear?

Comment 13 Christoph Wickert 2009-09-29 16:25:07 UTC
(In reply to comment #12)
> (In reply to comment #8)
> > - %description should end with a dot
> 
> Yes, that's usual, but I don't see it in guidelines:
> https://fedoraproject.org/wiki/Packaging/Guidelines#Summary_and_description
> so I couldn't complain, it's not MUST

That's why I used the word *should*.

> > - use %global instead of %define
> > https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
> 
> it's a draft suggestion, not MUST

I didn't say it's a MUST ether.

> > - license is completely unclear: Is this GPLv2 or GPLv2+? How about a COPYING
> > file or license headers in the files?
> 
> not all, but files under luci/lib/ do have clear license headers:
> # This program is free software; you can redistribute
> # it and/or modify it under the terms of version 2 of the
> # GNU General Public License as published by the
> # Free Software Foundation.

OK, I didn't see or grep those. My fault.

> Also luci/templates/footer.html is clear:
> Distributed under the <a
> href="http://creativecommons.org/licenses/GPL/2.0/">GNU GPL v2 license</a>.

Sorry, but this does not mean a lot, because you still cannot distinguish between GPLv2 and GPLv2+ with it. There is no "or any later version" that could be linked.

> So it's exactly GPLv2 as stated in spec, there isn't "or higher" clause to
> allow GPLv2+

Agreed.

> I have already suggested off-line to the packager=upstream maintainer to
> include LICENSE file into tarball to make it completely clear.

As it is a SHOULD thing in the review guidelines it should be mentioned here. off-line communication makes a review non-transparent.

> > - timestamps don't match: Source0 in the srpm is 
> > 28. Sep 15:27, while the one from the url is 15:33. Timestamps should match,
> > see
> > https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
> 
> md5sum is important which is fine, guideline says "consider"

Again I said *should*, especially as he have a very uncommon situation here (Source in srpm newer than from URL)

> I don't see why do you think that GPLv2 license is not clear?  

Because I didn't see those headers. Again, my bad.

Comment 14 Kevin Fenzi 2009-09-29 19:52:20 UTC
Looks like jpokorny isn't in the packager group yet: 

luci: Unable to save all information for luci: jpokorny must be in one of these groups: ('cvsadmin', 'packager', 'provenpackager') to hold the approveacls acl

Can you add a updated cvs request and reset the flag when ready?

Comment 15 Ryan McCabe 2009-09-30 14:47:19 UTC
Actually, it's fine to leave him off for now. I'll get his account status sorted out, then update his project status.

Comment 16 Rudolf Kastl 2009-09-30 15:04:22 UTC
(In reply to comment #9)
> (In reply to comment #6)
> > wow... that was a quick review... but after quickly looking at the package i
> > can see that the included readme references files that are non existant in the
> > package. wouldnt it make sense to either exclude the readme or package the
> > required files to test the turbogears application (they are existant in the
> > tarball)?  
> 
> I guess we'll need some guideline how to properly RPMize TurboGears
> applications.
> But strictly speaking this is not a blocking issue for the package review i.e.
> not MUST.  

not a must but i still prefer to use common sense and check for "relevant" issues when looking at a package (not trying to be a pain but trying to help increase the quality of the packages that enter the distro). while i know this is not a software review but a package review it still makes sense to get things sorted. if i see issues i am going to report them, even if they are not on the checklist and not rpmlint output. while still under review we are saving time and bandwidth to get things sorted.

Comment 17 Kevin Fenzi 2009-09-30 23:43:52 UTC
cvs done.

Comment 18 Orcan Ogetbil 2010-07-27 01:42:00 UTC
Hello, why is there no cvs record of this package on F-13 and devel?

Comment 19 Orcan Ogetbil 2010-07-27 01:46:14 UTC
I asked this, because if this package is not retired, we will need to rebuild it for python-2.7. Please answer asap. Thanks.

Comment 20 Dave Malcolm 2010-07-31 11:50:49 UTC
(In reply to comment #19)
> I asked this, because if this package is not retired, we will need to rebuild
> it for python-2.7. Please answer asap. Thanks.    
I've opened bug 620014 for this.


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