Created attachment 416977 [details]
NetworkManager-openswan spec file

Created attachment 416978 [details]
NetworkManager-openswan srpm

Some non-packaging review points:

- NM-vpnc describes itself in the UI as for 'Cisco Compatible VPN'
- This describes itself in the UI as for 'IPSec based VPNs (Openswan)'

1) this should use 'VPNs' instead of 'VPN'
2) I don't know that '(Openswan)' is a useful detail to show the user

Also, it appears that this only supports Cisco-style xauth+ike VPNs. As I understand it, Openswan supports a variety of auth mechanisms (certificates, etc.) that this isn't exposing.

(In reply to comment #3)
> Some non-packaging review points:
> 
> - NM-vpnc describes itself in the UI as for 'Cisco Compatible VPN'
> - This describes itself in the UI as for 'IPSec based VPNs (Openswan)'
> 
> 1) this should use 'VPNs' instead of 'VPN'
NetworkManager-openswan has "VPNs". Not sure if i understood you correctly. 

> 2) I don't know that '(Openswan)' is a useful detail to show the user
> 
Showing Openswan is necessary, because it is only specific to Openswan. So users should be able to know that this plugin only works with Openswan.

> Also, it appears that this only supports Cisco-style xauth+ike VPNs. As I
> understand it, Openswan supports a variety of auth mechanisms (certificates,
> etc.) that this isn't exposing.    

You are right. I am exposing only XAUTH and PSK based VPNs right now mainly like "road-warrior connections". But that does not mean that it not only works with Cisco VPN servers. It can also work other vpn servers that supports XAUTH+PSK. By doing this would help a lot to get feedback from various users who mostly use it with Cisco VPN servers. As I have NetworkManager-openswan helper in place now, adding other features would mainly require GUI changes only, and some minor changes in the helper.

(In reply to comment #4)

> > - NM-vpnc describes itself in the UI as for 'Cisco Compatible VPN'
> > - This describes itself in the UI as for 'IPSec based VPNs (Openswan)'
> > 
> > 1) this should use 'VPNs' instead of 'VPN'
> NetworkManager-openswan has "VPNs". Not sure if i understood you correctly. 

It should be changed to 'VPN' to be consistent.

> > 2) I don't know that '(Openswan)' is a useful detail to show the user
> > 
> Showing Openswan is necessary, because it is only specific to Openswan. So
> users should be able to know that this plugin only works with Openswan.

No, the plugin will talk to any IPSEC gateway that implements the same xauth/PSK authentication. If you mean it only works with openswan as a backend for this NM vpn plugin, that should be expressed just with RPM dependencies, not with a UI string - the user shouldn't have to care that openswan is being used under the hood.

(In reply to comment #5)
> (In reply to comment #4)
> 
> > > - NM-vpnc describes itself in the UI as for 'Cisco Compatible VPN'
> > > - This describes itself in the UI as for 'IPSec based VPNs (Openswan)'
> > > 
> > > 1) this should use 'VPNs' instead of 'VPN'
> > NetworkManager-openswan has "VPNs". Not sure if i understood you correctly. 
> 
> It should be changed to 'VPN' to be consistent.

Ok. changed.

> 
> > > 2) I don't know that '(Openswan)' is a useful detail to show the user
> > > 
> > Showing Openswan is necessary, because it is only specific to Openswan. So
> > users should be able to know that this plugin only works with Openswan.
> 
> No, the plugin will talk to any IPSEC gateway that implements the same
> xauth/PSK authentication. If you mean it only works with openswan as a backend
> for this NM vpn plugin, that should be expressed just with RPM dependencies,
> not with a UI string - the user shouldn't have to care that openswan is being
> used under the hood.    

ok, changed.

Thanks.
Avesh

Created attachment 417020 [details]
NetworkManager-openswan srpm

Why not to upload SPEC and SRPM to some pulic space?

e.g. fedorapeople dropbox

See https://fedoraproject.org/wiki/Infrastructure/fedorapeople.org

It'll be better to change %define snapshot .git20100411 to %define snapshot .20100411git or .%{gitdate}git%{githash}.

The naming style for the NetworkManger seems a little different compared to other packages in fedora.

See http://fedoraproject.org/wiki/PackageNamingGuidelines#Pre-Release_packages

You've attached tar.gz instead of src.rpm.
Please put the updated .src.rpm to the fedorapeople web server as suggested by Chen Lei above.

Note that you cannot comment out %define like you do:
#%define snapshot .git20100411
Macros are expanded in comments in .spec. You have to remove the %. Not that it matters much.

Also please add the COPYING file as %doc in %files.

Hello Chen, and Tomas,

Thanks for comments:

Here is the public link for the files:

http://people.redhat.com/avagarwa/files/NetworkManager-openswan/

(In reply to comment #9)
> It'll be better to change %define snapshot .git20100411 to %define snapshot
> .20100411git or .%{gitdate}git%{githash}.
> 
> The naming style for the NetworkManger seems a little different compared to
> other packages in fedora.
> 
> See http://fedoraproject.org/wiki/PackageNamingGuidelines#Pre-Release_packages


The naming style for NetworkManager-openswan, I have taken, is based on NetworkManager-vpnc and NetworkManager-openvpn , so that this new package is consistent with them.

(In reply to comment #10)
> You've attached tar.gz instead of src.rpm.
> Please put the updated .src.rpm to the fedorapeople web server as suggested by
> Chen Lei above.

Done: 

http://people.redhat.com/avagarwa/files/NetworkManager-openswan/

> 
> Note that you cannot comment out %define like you do:
> #%define snapshot .git20100411

> Macros are expanded in comments in .spec. You have to remove the %. Not that it
> matters much.

Instead commenting it, I have included it to make it consistent with other packages like NetworkManager-vpnc and NetworkManager-openvpn .
 

> 
> Also please add the COPYING file as %doc in %files.    
Done.

Some comment:
1.
Source:    %{name}-%{realversion}.tar.gz
Should add some comments about how to genrate the tarball
See http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

2.Epoch:     1

Remove it from spec, when possible packager should avoid of using epoch.


3.
BuildRoot: %{_tmppath}/%{name}-%{version}-root
Only needed for epel5

4.
%post
/sbin/ldconfig

%postun
/sbin/ldconfig

It's a plugin, I don't think ldconfig is necesscery

5.
%define nm_version          1:0.7.997-1
%define dbus_version        1.1
%define gtk2_version        2.10.0
%define shared_mime_version 0.16-3

Those lines are useless, all branches in fedora match those versions.


6.%{_datadir}/icons/hicolor/48x48/apps/gnome-mime-application-x-openswan-ipsec-vpn-settings.png

Is this file useful for openswan?


7.
%files -f %{name}.lang
%defattr(-, root, root)
->
%files -f %{name}.lang
%defattr(-, root, root,-)


8.
Requires: gtk2             >= %{gtk2_version}
Requires: dbus             >= %{dbus_version}
Requires: shared-mime-info >= %{shared_mime_version}
Requires: GConf2
Those dependicies may not need, please reconsider it, normally rpmbuild will pull in shlib depenpencies automatically.

(In reply to comment #14)
> Some comment:
> 1.
> Source:    %{name}-%{realversion}.tar.gz
> Should add some comments about how to genrate the tarball
> See http://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
> 
Done.

> 2.Epoch:     1
> 
> Remove it from spec, when possible packager should avoid of using epoch.
> 
Done.


> 
> 3.
> BuildRoot: %{_tmppath}/%{name}-%{version}-root
> Only needed for epel5
> 

Did not change as this package may be built for epel5 too. However, I have modified it to have as per fedora packaging guidelines:

%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

> 4.
> %post
> /sbin/ldconfig
> 
> %postun
> /sbin/ldconfig
> 
> It's a plugin, I don't think ldconfig is necesscery
> 
Done.
> 5.
> %define nm_version          1:0.7.997-1
> %define dbus_version        1.1
> %define gtk2_version        2.10.0
> %define shared_mime_version 0.16-3
> 
> Those lines are useless, all branches in fedora match those versions.
> 
> 
Done.

> 6.%{_datadir}/icons/hicolor/48x48/apps/gnome-mime-application-x-openswan-ipsec-vpn-settings.png
> 
> Is this file useful for openswan?

Done.
> 
> 
> 7.
> %files -f %{name}.lang
> %defattr(-, root, root)
> ->
> %files -f %{name}.lang
> %defattr(-, root, root,-)
>
Done. 
> 
> 8.
> Requires: gtk2             >= %{gtk2_version}
> Requires: dbus             >= %{dbus_version}
> Requires: shared-mime-info >= %{shared_mime_version}
> Requires: GConf2
> Those dependicies may not need, please reconsider it, normally rpmbuild will
> pull in shlib depenpencies automatically.    
Done.

In addition, added requires for openswan, and also made a few other changes. Current versions are avilable at

http://people.redhat.com/avagarwa/files/NetworkManager-openswan/

Thanks for your detailed comments.
Avesh

Chen Lei, will you do the formal review?

(In reply to comment #16)
> Chen Lei, will you do the formal review?    

I plan to do so :)

Is there anyone to help me to check whether the shared-mime-info dependency is needed for NM plugins? I don't understand  why all NM plugins require shared-mime-info. If all NM plugins requires mime info why not to add it to NM instead of plugins?


Some more suggestions:
It'll better to add --disable-static to %configure, thus rm -f %{buildroot}%{_libdir}/NetworkManager/lib*.a is not needed in spec.

From http://cvs.fedoraproject.org/viewvc/rpms/NetworkManager-pptp/devel/NetworkManager-pptp.spec?revision=1.22&view=co

I found that desktop file and icons are commended out in spec, %post and %postun seems unnecessary.

I can confirm the share-mime-info is useless now, all NM plugins in rawhide don't include desktop files now.

(In reply to comment #17)
> (In reply to comment #16)
> > Chen Lei, will you do the formal review?    
> 
> I plan to do so :)
> 
> Is there anyone to help me to check whether the shared-mime-info dependency is
> needed for NM plugins? I don't understand  why all NM plugins require
> shared-mime-info. If all NM plugins requires mime info why not to add it to NM
> instead of plugins?
> 
> 
> Some more suggestions:
> It'll better to add --disable-static to %configure, thus rm -f
> %{buildroot}%{_libdir}/NetworkManager/lib*.a is not needed in spec.
> 
> From
> http://cvs.fedoraproject.org/viewvc/rpms/NetworkManager-pptp/devel/NetworkManager-pptp.spec?revision=1.22&view=co
> 
> I found that desktop file and icons are commended out in spec, %post and
> %postun seems unnecessary.    

Thanks for your comments. They have been incorporated.

Rpm building fails on koji. I think you should remove the icon file, because you don't include a desktop file in rpm, also other NM plugins already remove icons and desktop files recently.

error: Installed (but unpackaged) file(s) found:
   /usr/share/icons/hicolor/48x48/apps/gnome-mime-application-x-openswan-ipsec-vpn-settings.png



formal review here:
+:ok, =:needs attention, -:needs fixing

MUST Items:
[-] MUST: rpmlint must be run on every package.
rpmlint NetworkManager-openswan.spec 
NetworkManager-openswan.spec:31: W: mixed-use-of-spaces-and-tabs (spaces: line 7, tab: line 31)

rpmlint NetworkManager-openswan-*rpm
NetworkManager-openswan.x86_64: I: enchant-dictionary-not-found en_US
NetworkManager-openswan.x86_64: W: incoherent-version-in-changelog 0.8.0-1 ['0.8.0-1.git20100411.fc14', '0.8.0-1.git20100411']
NetworkManager-openswan.x86_64: W: non-conffile-in-etc /etc/NetworkManager/VPN/nm-openswan-service.name
NetworkManager-openswan.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/nm-openswan-service.conf

[-] 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. [FIXME?: covers this list and more]
[+] 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.
[+] 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.
<<md5sum checksum>>065b8f38d89e7bc40bfe0d15a9e7f8ba
[+] MUST: The package must successfully compile and build into binary rpms on at least one supported architecture.
[=] MUST: All build dependencies must be listed in BuildRequires
[+] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro.
[+] 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.
[+] 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: If a package includes something as %doc, it must not affect the runtime of the application.
[+] MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec.
[+] 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: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[+] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[=] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.

Issues:

1. Most of the rpmlist warnings are harmless.
You should fix 
NetworkManager-openswan.spec:31: W: mixed-use-of-spaces-and-tabs (spaces: line 7, tab: line 31)

NetworkManager-openswan.x86_64: W: incoherent-version-in-changelog 0.8.0-1 ['0.8.0-1.git20100411.fc14', '0.8.0-1.git20100411']


2.According to naming guideline %define snapshot .git20100411 should be %define snapshot .20100411git, I found that the whole NM package stack are agaist naming guideline
See http://fedoraproject.org/wiki/PackageNamingGuidelines#Snapshot_packages
3.
BuildRequires: dbus-devel
BuildRequires: NetworkManager-devel

Those line can be commented out just for a reference, NetworkManager-glib-devel already requires them.

See http://koji.fedoraproject.org/koji/rpminfo?rpmID=1970296

(In reply to comment #20)
> Rpm building fails on koji. I think you should remove the icon file, because
> you don't include a desktop file in rpm, also other NM plugins already remove
> icons and desktop files recently.
> 
> error: Installed (but unpackaged) file(s) found:
>   
> /usr/share/icons/hicolor/48x48/apps/gnome-mime-application-x-openswan-ipsec-vpn-settings.png
> 

Fixed.

> 
> 
> formal review here:
> +:ok, =:needs attention, -:needs fixing
> 
> MUST Items:
> [-] MUST: rpmlint must be run on every package.
> rpmlint NetworkManager-openswan.spec 
> NetworkManager-openswan.spec:31: W: mixed-use-of-spaces-and-tabs (spaces: line
> 7, tab: line 31)
> 
> rpmlint NetworkManager-openswan-*rpm
> NetworkManager-openswan.x86_64: I: enchant-dictionary-not-found en_US
> NetworkManager-openswan.x86_64: W: incoherent-version-in-changelog 0.8.0-1
> ['0.8.0-1.git20100411.fc14', '0.8.0-1.git20100411']
> NetworkManager-openswan.x86_64: W: non-conffile-in-etc
> /etc/NetworkManager/VPN/nm-openswan-service.name
> NetworkManager-openswan.x86_64: W: non-conffile-in-etc
> /etc/dbus-1/system.d/nm-openswan-service.conf
> 

Fixed all rpmlint errors.

> [-] MUST: The package must be named according to the Package Naming Guidelines.

Fixed.

> Issues:
> 
> 1. Most of the rpmlist warnings are harmless.
> You should fix 
> NetworkManager-openswan.spec:31: W: mixed-use-of-spaces-and-tabs (spaces: line
> 7, tab: line 31)
> 
> NetworkManager-openswan.x86_64: W: incoherent-version-in-changelog 0.8.0-1
> ['0.8.0-1.git20100411.fc14', '0.8.0-1.git20100411']
> 

Fixed.

> 
> 2.According to naming guideline %define snapshot .git20100411 should be %define
> snapshot .20100411git, I found that the whole NM package stack are agaist
> naming guideline
> See http://fedoraproject.org/wiki/PackageNamingGuidelines#Snapshot_packages

Fixed.


> 3.
> BuildRequires: dbus-devel
> BuildRequires: NetworkManager-devel
> 

Fixed.

> Those line can be commented out just for a reference, NetworkManager-glib-devel
> already requires them.
> 
> See http://koji.fedoraproject.org/koji/rpminfo?rpmID=1970296    

The latest modified versions are available at 
http://people.redhat.com/~avagarwa/files/NetworkManager-openswan/

When I fixed the rpmlint error "non-conffile-in-etc", by putting %config section, rpmlint started showing the error "conffile-without-noreplace-flag", so I had to put %config(noreplace) .

(In reply to comment #22)
> When I fixed the rpmlint error "non-conffile-in-etc", by putting %config
> section, rpmlint started showing the error "conffile-without-noreplace-flag",
> so I had to put %config(noreplace) .    

From fedora guideline
"Use your common sense and do not follow rpmlint warnings blindly"

Many rpmlint warnings or errors are harmless or even wrong, e.g. "spelling error".


The package is approved, but you still need to remove %config(noreplace) from %file. I think those config files should be overrided every time when updating this package.

> The package is approved, but you still need to remove %config(noreplace) from
> %file. I think those config files should be overrided every time when updating
> this package.    

fixed, and package updated. Thanks for the review.

New Package CVS Request
=======================
Package Name: NetworkManager-openswan
Short Description: This package contains software for integrating the openswan VPN software with NetworkManager and the GNOME desktop
Owners: avesh
Branches: F-12 F-13
InitialCC: avesh

CVS done (by process-cvs-requests.py).