Bug 443807 - (nm-pptp-review) Review Request: NetworkManager-pptp - PPTP support for NetworkManager
Review Request: NetworkManager-pptp - PPTP support for NetworkManager
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-23 10:00 EDT by Lubomir Kundrak
Modified: 2008-05-16 02:03 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-04-27 11:25:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dan: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lubomir Kundrak 2008-04-23 10:00:44 EDT
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 10:03:25 EDT
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 10:12:22 EDT
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 06:07:16 EDT
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 17:07:19 EDT
(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 03:51:01 EDT
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 04:17:19 EDT
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 11:14:37 EDT
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 11:25:59 EDT
All done. Thanks Kevin, Thanks Dan.
Comment 9 Adam Sobotka 2008-05-16 02:03:11 EDT
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.