[Bug 175047] Review Request: NetworkManager-openvpn

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: NetworkManager-openvpn


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=175047





------- Additional Comments From j.w.r.degoede at hhs.nl  2006-07-26 12:09 EST -------
Okay, here is a full review for this one, I'll do the other 2 you mentioned as
time permits.

MUST:
=====
O rpmlint output is:
E: NetworkManager-openvpn explicit-lib-dependency libglade2
E: NetworkManager-openvpn explicit-lib-dependency libgnomeui
W: NetworkManager-openvpn no-url-tag
W: NetworkManager-openvpn devel-file-in-non-devel-package
/usr/lib64/libnm-openvpn-properties.so
W: NetworkManager-openvpn non-conffile-in-etc
/etc/dbus-1/system.d/nm-openvpn-service.conf
E: NetworkManager-openvpn zero-length
/usr/share/doc/NetworkManager-openvpn-0.3.2/NEWS
W: NetworkManager-openvpn non-conffile-in-etc
/etc/NetworkManager/VPN/nm-openvpn-service.name
W: NetworkManager-openvpn-debuginfo no-url-tag
W: NetworkManager-openvpn no-url-tag
W: NetworkManager-openvpn prereq-use %{_bindir}/update-desktop-database
W: NetworkManager-openvpn prereq-use %{_bindir}/gtk-update-icon-cache
See Must FIX list.
* Package and spec file named appropriately
* Packaged according to packaging guidelines
O License (GPL) ok, but license file not included
* spec file is legible and in Am. English.
O Source matches upstream ?? (couldn't verify, but you are upstream right?)
* Compiles and builds on devel-x86_64
* BR: ok (see below)
* No locales
* No shared libraries
* Not relocatable
* Package owns / or requires all dirs (with some strangeness see Must fix below)
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files.
* no gui -> no .desktop file required


MUST fix:
=========
* The followjng rpmlint output:
E: NetworkManager-openvpn explicit-lib-dependency libglade2
E: NetworkManager-openvpn explicit-lib-dependency libgnomeui
Why explicitly require these, explicit requires are ok if you want to force a
certain version, but since these requires are versionless, the autodep rpm does
on dso's should suffice or?

W: NetworkManager-openvpn devel-file-in-non-devel-package
/usr/lib64/libnm-openvpn-properties.so
Is this needed for dl-opening by NetWorkManager, if not, is this needed at all?

W: NetworkManager-openvpn non-conffile-in-etc
/etc/dbus-1/system.d/nm-openvpn-service.conf
You must mark this %config (just put %config in front of the line) normally we
always use %config(noreplace) in FE, bubt I don't know if thats appropiate here.

E: NetworkManager-openvpn zero-length 
/usr/share/doc/NetworkManager-openvpn-0.3.2/NEWS
Please don't package this empty file

W: NetworkManager-openvpn non-conffile-in-etc 
/etc/NetworkManager/VPN/nm-openvpn-service.name
You must mark this %config (just put %config in front of the line) normally we
always use %config(noreplace) in FE, bubt I don't know if thats appropiate here.

W: NetworkManager-openvpn prereq-use %{_bindir}/update-desktop-database
W: NetworkManager-openvpn prereq-use %{_bindir}/gtk-update-icon-cache
Do not use PreReq, it has issues. Instead use Requires(post): xxx and
Requires(postun): xxx

* Add a Requires(post) (and postun): /sbin/ldconfig
* Why no %{?_smp_mflags} after make? either add a comment why tis admitted or
  add it.
* %install must start with: "rm -fr %{buildroot}"
* %makeinstall has issues (see f-e-l archives) instead use
  "make install DESTDIR=%{buildroot}"


Should fix:
===========
* Do I understand correctly that you are upstream for this package? If so
  I'll assume that the included tarbal matches upstream. Still it would be nice
  to provide the tarbal for download from your site and use a full URL as
  Source0. (rpmlint: W: NetworkManager-openvpn no-url-tag)
* You should include a COPYING in the tarbal and in %doc
* "rm -f %{buildroot}%{_libdir}/lib*.a" why not just add --disable-static to
  %configure that reduces build time quite a bit. while you're add it also add
  --disable-dependency-tracking


Questions:
==========
* Why is this there? "## %find_lang %{name}" (similar for %files)



-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.