Bug 484535 - Review Request: kde-plasma-networkmanagement - Plasmoid to control Network Manager
Summary: Review Request: kde-plasma-networkmanagement - Plasmoid to control Network Ma...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Milos Jakubicek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-02-08 02:53 UTC by Ben Boeckel
Modified: 2009-03-03 17:01 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-03-03 17:01:21 UTC
xjakub: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ben Boeckel 2009-02-08 02:53:57 UTC
Spec URL: http://benboeckel.net/packaging/kde-plasma-networkmanagement/kde-plasma-networkmanagement.spec
SRPM URL: http://benboeckel.net/packaging/kde-plasma-networkmanagement/kde-plasma-networkmanagement-0.1-0.3.20090207svn.fc10.src.rpm
Description: Plasmoid for KDE4 to control Network Manger.

It still has some issues, but we (the KDE SIG) are looking to get it into Rawhide so it can get testing before the F11 beta and so we can evaluate it as a possible default for F11 if it matures in time, otherwise, we plan to at least offer it as an option. We also plan to push it out to F10 and possibly F9 once it is reliable enough for a non-default option.

Comment 1 Armin 2009-02-08 21:21:56 UTC
there is an rpmlint issue with the src rpm:

kde-plasma-networkmanagement.src:25: W: unversioned-explicit-obsoletes kde-plasma-networkmanager
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

it's better if you specify the highest version of kde-plasma-networkmanager it obsoletes.

Comment 2 Armin 2009-02-08 21:23:08 UTC
there is an rpmlint issue with the src rpm:

kde-plasma-networkmanagement.src:25: W: unversioned-explicit-obsoletes kde-plasma-networkmanager
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

it's better if you specify the highest version of kde-plasma-networkmanager it obsoletes.

Comment 3 Armin 2009-02-08 21:32:24 UTC
kde-plasma-networkmanagement.x86_64: W: no-documentation
kde-plasma-networkmanagement.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/NetworkManager-kde4.conf
kde-plasma-networkmanagement.x86_64: W: obsolete-not-provided kde-plasma-networkmanager
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

There is no %doc (the TODO and DESIGN files could be added as %doc)

Comment 4 Armin 2009-02-08 21:38:27 UTC
also, for debug-info, it's better if you chop the %description in two lines, rpmlint complains about this:

kde-plasma-networkmanagement-debuginfo.x86_64: E: description-line-too-long This package provides debug information for package kde-plasma-networkmanagement.
kde-plasma-networkmanagement-debuginfo.x86_64: W: filename-too-long-for-joliet kde-plasma-networkmanagement-debuginfo-0.1-0.3.20090207svn.fc10.x86_64.rpm

and the filename is too long for all of them, but I don't think that would be a big issue.

Comment 5 Armin 2009-02-08 21:42:41 UTC
otherwise, it builds fine on x86_64 =)

Comment 6 Kevin Kofler 2009-02-08 21:49:25 UTC
> kde-plasma-networkmanagement.src:25: W: unversioned-explicit-obsoletes
kde-plasma-networkmanager

That one ought to be fixed.

> kde-plasma-networkmanagement.x86_64: W: no-documentation

Mostly harmless. Is there no COPYING? That's worth a complaint upstream. TODO and DESIGN may or may not be worth shipping.

> kde-plasma-networkmanagement.x86_64: W: non-conffile-in-etc
/etc/dbus-1/system.d/NetworkManager-kde4.conf

That's the usual D-Bus "config files" which aren't really "config files". It could be marked %config, but I think it probably shouldn't, that file isn't really intended to be customized. The path is in /etc because that's where D-Bus expects those files to be.

> kde-plasma-networkmanagement.x86_64: W: obsolete-not-provided
kde-plasma-networkmanager

Harmless, but I think adding the Provides can't hurt and it will make "yum install kde-plasma-networkmanager" work.

> kde-plasma-networkmanagement-debuginfo.x86_64: E: description-line-too-long
This package provides debug information for package
kde-plasma-networkmanagement.

The line exceeds 80 characters, it should be broken into 2 lines.

> kde-plasma-networkmanagement-debuginfo.x86_64: W: filename-too-long-for-joliet
kde-plasma-networkmanagement-debuginfo-0.1-0.3.20090207svn.fc10.x86_64.rpm

Ignore that one. The name is as it has to be according to the guidelines, you can't shorten it.

Comment 7 Armin 2009-02-08 21:56:16 UTC
> Mostly harmless. Is there no COPYING? That's worth a complaint upstream. TODO
> and DESIGN may or may not be worth shipping.

no, there isn't.  But I think TODO and DESIGN should be part of %doc, as they are upstream.  Someone might need it.

Comment 8 Kevin Kofler 2009-02-08 23:06:43 UTC
Actually the -debuginfo description is autogenerated, so that description-line-too-long warning can also be ignored.

Comment 9 Milos Jakubicek 2009-02-09 01:04:20 UTC
- regarding the rpmlint output, as discussed in previous comments -- please fix:
kde-plasma-networkmanagement.src:25: W: unversioned-explicit-obsoletes
kde-plasma-networkmanagement.x86_64: W: obsolete-not-provided
kde-plasma-networkmanagement.x86_64: W: no-documentation
(include the TODO and DESIGN file)

- use macros in %build and %files:
%{cmake_kde4} .. -DDBUS_SYSTEM_POLICY_DIR=/etc/dbus-1/system.d
/etc/dbus-1/system.d/NetworkManager-kde4.conf

/etc => %{_sysconfdir}

- remove unnecessary BR (main package) and R (for -devel subpackage):
  BR/R: NetworkManager-devel which is required by NetworkManager-glib-devel
  BR/R: kdelibs4-devel which is required by kdebase-workspace-devel

  Requires(post): /sbin/ldconfig xdg-utils
  Requires(postun): /sbin/ldconfig xdg-utils
  (Those are taken by rpm automagically)

- please provide a more appropriate description than %{summary} for the main package.

- please provide a less general URL than just kde.org, e.g.
http://websvn.kde.org/trunk/playground/base/plasma/applets/networkmanager/

- there is no licensing info besides the qt dual license and .desktop files; nor in an attached LICENSE file, nor in the source files -- that's bad. Please ask upstream to include a license file and to mention the license in the sources.
You should also include a LICENSE.GPL and GPL_EXCEPTION.txt file as the qt license states.

- please fix the gcc flags, something like this before the make call should be enough:

find . -name "flags.make" -execdir sed -i -e 's/-fno-exceptions -fno-check-new -fno-common//' -e 's/-fno-threadsafe-statics -fvisibility=hidden -fvisibility-inlines-hidden//' -e 's/-ansi//' {} \;

Comment 10 Milos Jakubicek 2009-02-09 01:41:52 UTC
ad *-debuginfo.*: E: description-line-too-long: filed as 484616

Comment 11 Rex Dieter 2009-02-11 03:09:04 UTC
imo, leave the gcc flags alone, this is how pretty much all of kde is built in fedora already.  If you have specific concerns, then we could consider those and consult upstream, but that's outside the scope of this review.

Comment 12 Ben Boeckel 2009-02-21 03:02:08 UTC
Fixed SPEC: http://benboeckel.net/packaging/kde-plasma-networkmanagement/kde-plasma-networkmanagement.spec
Fixed SRPM: http://benboeckel.net/packaging/kde-plasma-networkmanagement/kde-plasma-networkmanagement-0.1-0.4.20090217svn.fc10.src.rpm

rpmlint says that the Provides: should be versioned. Not sure if I should, and if so, what version it should be. %{version}-%{release} I'd guess, if any.

Comment 13 Milos Jakubicek 2009-02-22 02:35:25 UTC
(In reply to comment #12)

> rpmlint says that the Provides: should be versioned. Not sure if I should, and
> if so, what version it should be. %{version}-%{release} I'd guess, if any.

- Yes, it should and those Provides/Obsoletes statements must be placed also by the subpackages, see this draft for details:
https://fedoraproject.org/wiki/Archive:PackagingDrafts/ProvidesObsoletes

- remove duplicate Requires: openvpn/vpnc, they're again pulled by the NetworkManager-openvpn/vpnc packages.

- by providing a more appropriate description, I really didn't mean to expand the macro;) You should briefly describe the functions of the plasmoid, please try to do so for a few lines.

- regardins licensing it's a big mess:

GPLv2+:
- libs/dbus/*

LGPLv2:
- in libs/ui:
accesspoint.cpp,h
apitemdelegate.cpp,h
apitemmodel.cpp,h
apitemview.cpp,h
ifaceitemmodel.cpp,h
scanwidget.cpp,h
- in libs:
marshalarguments.h
types.h
- in libs/storage/settings:
wephash.cpp,h
- in settings:
ip4config.cpp,h
- in settings/service:
busconnection.cpp,h
networksettings.cpp,h

LGPLv2+:
- in tests:
testconfigxml.cpp,h
testnewstorage.cpp,h

GPLv2 or GPLv3 ("Qt Nokia"):
tests/qdbusfornm.cpp

MISSING LICENSE:
libs/ui/vpnuiplugin.cpp

all others under libs/ are LGPLv2+
all others are GPLv2+

So...this is a bit problem, please:

- you can (but not must of course) try to persuade upstream to unify the licenses to GPLv2+ and LGPLv2+ if it is possible. Currently it violates the GPL licenses according to:
https://fedoraproject.org/wiki/Licensing#GPL_Compatibility_Matrix
(You cannot copy LGPLv2 code within GPLv2+ project without converting to GPLv2)
So you should at least try that.

- you really should persuade them to:
-- add the license info to libs/ui/vpnuiplugin.cpp if it not autogenerated
-- include the license files for GPLv2 and LGPLv2, LICENSE.GPL and GPL_EXCEPTIONS.TXT. Suggest naming the GPLv2 license file as LICENSE.GPL to avoid duplicates.
- please put the information I've gathered above into a LICENSING.INFO file and package it as a %doc

As it is now, the License tag would be:
(GPLv2 or GPLv3) and GPLv2+ and LGPLv2+ and LGPLv2

Comment 14 Kevin Kofler 2009-02-22 02:55:54 UTC
As discussed on IRC, using the LGPLv2 code there is not a violation.

Comment 15 Ben Boeckel 2009-02-24 01:17:45 UTC
Fixed SPEC:
http://benboeckel.net/packaging/kde-plasma-networkmanagement/kde-plasma-networkmanagement.spec
Fixed SRPM:
http://benboeckel.net/packaging/kde-plasma-networkmanagement/kde-plasma-networkmanagement-0.1-0.5.20090217svn.fc10.src.rpm

Fixed the licensing line, removed dupe R a, fixed P/O for subpackages and got a better summary/description.

Comment 16 Milos Jakubicek 2009-02-24 20:18:47 UTC
>rpmlint -i ../RPMS/x86_64/kde-plasma-networkmanagement-*0.5*
[...]
kde-plasma-networkmanagement-devel.x86_64: W: tag-in-description Obsoletes:
Something that looks like a tag was found in the package's description. This
may indicate a problem where the tag was not actually parsed as a tag but just
textual description content, thus being a no-op.  Verify if this is the case,
and move the tag to a place in the specfile where %description won't fool the
specfile parser, and rebuild the package.
[...]

The P/O for subpackages are wrong placed, they must be before the %description  otherwise they're taken as part of the %description as described in the rpmlint warning.

Comment 18 Milos Jakubicek 2009-02-27 15:10:06 UTC
OK, there are some minor outstanding issues regarding licensing, but none of them are blocking and can be done later on, otherwise all the problems have been addressed, this package is APPROVED.

Comment 19 Ben Boeckel 2009-02-27 16:12:59 UTC
New Package CVS Request
=======================
Package Name: kde-plasma-networkmanagement
Short Description: NetworkManager KDE 4 integration
Owners: mathstuf rdieter
Branches: F-9 F-10
InitialCC:

Comment 20 Kevin Fenzi 2009-03-01 00:19:35 UTC
cvs done.

Comment 21 Ben Boeckel 2009-03-03 17:01:21 UTC
Built for rawhide, closing.


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