Bug 443807 (nm-pptp-review) - Review Request: NetworkManager-pptp - PPTP support for NetworkManager
Summary: Review Request: NetworkManager-pptp - PPTP support for NetworkManager
Keywords:
Status: CLOSED NEXTRELEASE
Alias: nm-pptp-review
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dan Horák
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-04-23 14:00 UTC by Lubomir Kundrak
Modified: 2008-05-16 06:03 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-04-27 15:25:59 UTC
Type: ---
Embargoed:
dan: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Lubomir Kundrak 2008-04-23 14:00:44 UTC
SRPM:
http://people.redhat.com/lkundrak/mock-results/NetworkManager-pptp-0.6.4-1.el5.i386/NetworkManager-pptp-0.6.4-1.el5.src.rpm
SPEC: http://people.redhat.com/lkundrak/SPECS/NetworkManager-pptp.spec
mock:
http://people.redhat.com/lkundrak/mock-results/NetworkManager-pptp-0.6.4-1.el5.i386/

Description: PPTP support for NetworkManager

This package contains software for integrating PPTP VPN support with
the NetworkManager and the GNOME desktop.

Comment 1 Lubomir Kundrak 2008-04-23 14:03:25 UTC
This works only with NetworkManager 0.6.4, that is in RHEL-5, and thus is only
intended for Fedora EPEL.

Comment 2 Lubomir Kundrak 2008-04-23 14:12:22 UTC
Just two things I noticed now; Calling ldconfig in scriptlets most likely
doesn't make sense, since we probably don't install libraries just, plugins and
I did not comment the patches. I'll address those next time I roll packages.

Comment 3 Dan Horák 2008-04-24 10:07:16 UTC
OK	source files match upstream:
	    archives have no differences when using the decribed steps
OK	package meets naming and versioning guidelines.
BAD	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	build root is correct.
OK	license field matches the actual license.
OK	license is open source-compatible (GPLv2+). License text included in package.
OK	latest version is being packaged.
BAD	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	%clean is present.
OK	package builds in mock (CentOS-5/x86_64).
OK	debuginfo package looks complete.
BAD	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	no shared libraries are added to the regular linker search paths.
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK	file permissions are appropriate.
BAD	scriptlets present.
OK	code, not content.
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	no headers.
OK	no pkgconfig files.
OK	no libtool .la droppings.
OK	is a GUI app, desktop file is installed correctly


comments:
- mixed usage of $RPM_BUILD_ROOT and %{buildroot} in %install section
- you should remove BR: autoconf automake
- files in %{_sysconfdir} should be marked as %config
- NetworkManager-pptp.x86_64: W: devel-file-in-non-devel-package
/usr/lib64/libnm-pptp-properties.so
    is libnm-pptp-properties a plugin or real library? plugins should have
-avoid-version in their LDFLAGS
- remove ldconfig calls from the scriptlets (depends on ^)
- remove R(post,postun): /sbin/ldconfig - added automagically when ldconfig is
used in scriptlets

Comment 4 Lubomir Kundrak 2008-04-24 21:07:19 UTC
(In reply to comment #3)

Thanks for the review.

> comments:
> - mixed usage of $RPM_BUILD_ROOT and %{buildroot} in %install section

Fixed.

> - you should remove BR: autoconf automake

Fixed.

> - files in %{_sysconfdir} should be marked as %config

These are not really configuration files. At least not user-adjustable.

> - NetworkManager-pptp.x86_64: W: devel-file-in-non-devel-package
> /usr/lib64/libnm-pptp-properties.so
>     is libnm-pptp-properties a plugin or real library? plugins should have
> -avoid-version in their LDFLAGS

Right. Patched that out.

> - remove ldconfig calls from the scriptlets (depends on ^)

Done.

> - remove R(post,postun): /sbin/ldconfig - added automagically when ldconfig is
> used in scriptlets

Done.

http://people.redhat.com/lkundrak/SPECS/NetworkManager-pptp.spec
http://people.redhat.com/lkundrak/SRPMS/NetworkManager-pptp-0.6.4-2.fc8.src.rpm

Comment 5 Dan Horák 2008-04-25 07:51:01 UTC
All issues resolved or explained, so package is APPROVED.

I have created a bug with a patch in b.g.o to solve the "-avoid-version" problem
in upstream (#529836) for all the vpn plugins.

Comment 6 Lubomir Kundrak 2008-04-25 08:17:19 UTC
New Package CVS Request
=======================
Package Name: NetworkManager-pptp
Short Description: PPTP support for NetworkManager
Owners: lkundrak
Branches: EL-5 (Is devel mandatory? This won't ever make it to Fedora, it is
named differently for newer NetworkManager versions)
Cvsextras Commits: yes

Comment 7 Kevin Fenzi 2008-04-25 15:14:37 UTC
yes, devel is required. You can just dead.package the devel branch explaining
that this is a EL-5 only package?

cvs done.

Comment 8 Lubomir Kundrak 2008-04-27 15:25:59 UTC
All done. Thanks Kevin, Thanks Dan.

Comment 9 Adam Sobotka 2008-05-16 06:03:11 UTC
Am I supposed to see package in main repo in F9?


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