Bug 465943 - Review Request: NetworkManager-openconnect - NetworkManager VPN integration for openconnect
Summary: Review Request: NetworkManager-openconnect - NetworkManager VPN integration f...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Peter Robinson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-10-07 10:14 UTC by David Woodhouse
Modified: 2009-01-08 08:50 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-01-08 08:50:36 UTC
Type: ---
Embargoed:
pbrobinson: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description David Woodhouse 2008-10-07 10:14:51 UTC
Spec URL: http://david.woodhou.se/NetworkManager-openconnect.spec
SRPM URL: http://david.woodhou.se/NetworkManager-openconnect-0.94-1.fc9.src.rpm
Description: This package contains software for integrating the openconnect VPN software with NetworkManager and the GNOME desktop

Comment 1 David Woodhouse 2008-10-07 15:29:32 UTC
koji test build at http://koji.fedoraproject.org/koji/taskinfo?taskID=866770

rpmlint says this, which it also says of the other NetworkManager-foo packages:
NetworkManager-openconnect.ppc: W: non-conffile-in-etc /etc/dbus-1/system.d/nm-openconnect-service.conf
NetworkManager-openconnect.ppc: W: non-conffile-in-etc /etc/NetworkManager/VPN/nm-openconnect-service.name
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

Comment 2 Dan Williams 2008-10-07 20:54:25 UTC
rpmlint is wrong; that's where dbus stores service config files.

Note that these should _NOT_ be marked %config either, they are not meant to be user-editable.

Comment 3 David Woodhouse 2008-10-31 12:46:10 UTC
Spec URL: http://david.woodhou.se/NetworkManager-openconnect.spec
SRPM URL: http://david.woodhou.se/NetworkManager-openconnect-0.96-1.fc9.src.rpm

Updated to run the VPN client itself as a non-root user.

Comment 4 Peter Robinson 2008-12-16 21:48:20 UTC
A couple of things. Where is the location for the source tarballs? I can't see them on either the openconnect site or NetworkManager site. Also alot of the Requires shouldn't be needed as RPM will automatically work it out from the library versions it that its built against. Explicit requires are only required for things like dbus.

Comment 5 Peter Robinson 2008-12-16 21:51:13 UTC
Also has build issues in koji
http://koji.fedoraproject.org/koji/taskinfo?taskID=1002223

Comment 6 David Woodhouse 2008-12-16 21:55:03 UTC
Yeah, stuff changed in rawhide; I have to build a slightly newer version there.

Tarballs will be coming from the same place as the rest of NetworkManager in the future. The code has now been imported into GNOME SVN.

Most of the specfile was copied directly from NetworkManager-vpnc. As was the code, for that matter.

dcbw, what version number should we use for the first build?

Comment 8 David Woodhouse 2008-12-16 22:31:40 UTC
Tarballs uploaded now.

At dcbw's request I'll change the version number to 0.7.0-1.

Comment 9 David Woodhouse 2008-12-16 22:53:13 UTC
OK, rebuilt as 0.7.0-1, and a bunch of superfluous Requires: dropped.

Spec URL: http://david.woodhou.se/NetworkManager-openconnect.spec
SRPM URL: http://david.woodhou.se/NetworkManager-openconnect-0.7.0-1.svn3.fc10.src.rpm

Comment 11 Peter Robinson 2008-12-17 00:56:55 UTC
A few fixes required. Also as a side note it doesn't look like libgnome is used in the source files but its included in the configure checks but I'm not sure.

+ rpmlint output

$ rpmlint NetworkManager-openconnect.spec 
NetworkManager-openconnect.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 70)
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
- license matches the actual package license

There's no COPYING file included in the source. Some of the source files include the license but some don't include any at all.

+ %doc includes license file
+ spec file written in American English
+ spec file is legible
- upstream sources match sources in the srpm
  399dc23f2df67d994770dfdfdaec0ecb  NetworkManager-openconnect-0.7.0.svn3.tar.gz
  
  Package needs to adhere to source requirements, if its using VCS snapshots it needs to be specified as per the Packaging docs.
  https://fedoraproject.org/wiki/Packaging/SourceURL

+ package successfully builds on at least one architecture
  built fine in koji
- BuildRequires list all build dependencies

Not sure why libpng is required. It builds fine without it and doesn't seem to check for it in the configure script.

+ %find_lang instead of %{_datadir}/locale/*
  Documented that there's no translations yet but its handled
+ binary RPM with shared library files must call ldconfig in %post and %postun
+ does not use Prefix: /usr
- package owns all directories it creates

/usr/share/gnome-vpn-properties/openconnect/ is not owned by the package 
/usr/share/gnome-vpn-properties/ isn't either and should probably be owned by NetworkManager itself as more than one NM vpn package installs things there.

n/a no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
n/a header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a libfoo.so must go in -devel
n/a devel must require the fully versioned base
+ packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
- %install must start with rm -rf $RPM_BUILD_ROOT etc.
  needs to remove old buildroots
+ filenames must be valid UTF-8

Optional:

- if there is no license file, packager should query upstream
  mentioned above
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock
+ the package should build into binary RPMs on all supported architectures
+ review should test the package functions as described (basic testing but don't have a Cisco Anyconnect VPN currently available)
+ scriptlets should be sane
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin

Comment 12 David Woodhouse 2008-12-17 12:49:19 UTC
(In reply to comment #11)
> A few fixes required. Also as a side note it doesn't look like libgnome is used
> in the source files but its included in the configure checks but I'm not sure.

I've added a patch to remove it from configure.in. Well spotted.

> + rpmlint output
> 
> $ rpmlint NetworkManager-openconnect.spec 
> NetworkManager-openconnect.spec: W: mixed-use-of-spaces-and-tabs (spaces: line
> 1, tab: line 70)

Fixed.

> - license matches the actual package license
> 
> There's no COPYING file included in the source. Some of the source files
> include the license but some don't include any at all.

Added COPYING file in a patch also. Will push upstream.

> - upstream sources match sources in the srpm
>   399dc23f2df67d994770dfdfdaec0ecb 
> NetworkManager-openconnect-0.7.0.svn3.tar.gz
> 
>   Package needs to adhere to source requirements, if its using VCS snapshots it
> needs to be specified as per the Packaging docs.
>   https://fedoraproject.org/wiki/Packaging/SourceURL

Added instructions.

> - BuildRequires list all build dependencies
> 
> Not sure why libpng is required. It builds fine without it and doesn't seem to
> check for it in the configure script.

Removed.

> - package owns all directories it creates
> 
> /usr/share/gnome-vpn-properties/openconnect/ is not owned by the package 

Fixed.

> /usr/share/gnome-vpn-properties/ isn't either and should probably be owned by
> NetworkManager itself as more than one NM vpn package installs things there.

Yeah, that's a NetworkManager bug.

> - %install must start with rm -rf $RPM_BUILD_ROOT etc.
>   needs to remove old buildroots

Fixed.

Spec URL: http://david.woodhou.se/NetworkManager-openconnect.spec
SRPM URL:
http://david.woodhou.se/NetworkManager-openconnect-0.7.0-2.svn3.fc10.src.rpm

http://koji.fedoraproject.org/koji/taskinfo?taskID=1003330

Comment 13 Peter Robinson 2008-12-18 01:08:53 UTC
Confirmed issues are fixed. Thanks. APPROVED!

Comment 14 David Woodhouse 2008-12-18 10:42:08 UTC
New Package CVS Request
=======================
Package Name: NetworkManager-openconnect
Short Description: NetworkManager VPN integration for openconnect
Owners: dwmw2 dcbw
Branches: F-9 F-10 devel

Comment 15 Kevin Fenzi 2008-12-21 04:21:08 UTC
cvs done.

Comment 16 Peter Robinson 2009-01-03 10:48:02 UTC
I think this is in rawhide, OK to close?

Comment 17 Peter Robinson 2009-01-08 08:50:36 UTC
Its built in koji for rawhide/F-10/F-9 so closing.


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