Bug 632853 - Review Request: pptpd - PoPToP Point to Point Tunneling Server
Summary: Review Request: pptpd - PoPToP Point to Point Tunneling Server
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mathieu Bridon
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-09-11 13:25 UTC by Dave Ludlow
Modified: 2014-03-27 04:47 UTC (History)
10 users (show)

Fixed In Version: pptpd-1.4.0-2.fc20
Clone Of:
Environment:
Last Closed: 2013-11-21 04:38:07 UTC
Type: ---
Embargoed:
bochecha: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
Current upstream spec (6.17 KB, text/plain)
2012-09-06 06:42 UTC, Paul Howarth
no flags Details

Description Dave Ludlow 2010-09-11 13:25:31 UTC
Spec URL: http://adsllc.fedorapeople.org/rpmbuild/SPECS/pptpd.spec
SRPM URL: http://adsllc.fedorapeople.org/rpmbuild/SRPMS/pptpd-1.3.4-1.fc13.src.rpm
Description:
This implements a Virtual Private Networking Server (VPN) that is
compatible with Microsoft VPN clients. It allows windows users to
connect to an internal firewalled network using their dialup.

Comment 1 Dave Ludlow 2010-09-11 16:00:03 UTC
RPMLINT

$ rpmlint SPECS/pptpd.spec RPMS/x86_64/pptpd-* SRPMS/pptpd-1.3.4-1.fc13.src.rpm 
pptpd.x86_64: W: spelling-error %description -l en_US firewalled -> fire walled, fire-walled, firewall ed
pptpd.x86_64: W: spelling-error %description -l en_US dialup -> dial up, dial-up, dialogue
pptpd.x86_64: W: no-manual-page-for-binary vpnstats.pl
pptpd.x86_64: W: no-manual-page-for-binary vpnuser
pptpd.x86_64: W: no-manual-page-for-binary bcrelay
pptpd.x86_64: W: no-manual-page-for-binary pptp-portslave
pptpd.src: W: spelling-error %description -l en_US firewalled -> fire walled, fire-walled, firewall ed
pptpd.src: W: spelling-error %description -l en_US dialup -> dial up, dial-up, dialogue
3 packages and 1 specfiles checked; 0 errors, 8 warnings.

Warnings discussion:
I believe that "firewalled" and "dialup" are acceptable American English words, and they are provided by upstream.

Yep, there are no man pages provided by upstream for some files.

KOJI

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

Comment 2 Jason Tibbitts 2012-04-24 21:26:30 UTC
I have no hope of testing this, but I'm going through old package review tickets and figured I could make a few comments.

Besides the no-manual-page-for-binary stuff, I get:

  pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd-1.3.4/README.bcrelay
  pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd-1.3.4/COPYING
  pptpd-debuginfo.x86_64: E: incorrect-fsf-address
   /usr/src/debug/pptpd-1.3.4/our_getopt.h
These should be reported upstream.

  pptpd.x86_64: W: install-file-in-docs /usr/share/doc/pptpd-1.3.4/INSTALL
There is generally no need to package INSTALL files; they are not generally useful, especially when they contain just the generic autoconf installation instructions.  Our users just use yum install or one of the GUIs to install things.

I'm not sure of the releases for which you're targeting this, but Fedora requires new packages to support systemd and so this will need to be converted.  I can help with that if you need it.  The initscript seems mostly trivial, although systemd has no concept of the custom restart-kill action.  I'm pretty sure that it will simply always kill existing connections.

Comment 3 Jaroslav Škarvada 2012-04-25 07:53:20 UTC
Hi Dave,  

I took this review and I will respond till end of this week. I am highly interested in this package and I can also help you co-maintain or even take this package later as maintainer. I am running the custom build on f16 for some time without problems.

Dave, are you still interested / responsive regarding this review?

Comment 4 Charlie Brady 2012-08-30 18:13:43 UTC
You need to change this if you want the logtmp plugin for pppd to work:

- (echo '#undef VERSION'; echo '#define VERSION "2.4.3"') >> plugins/patchlevel.h
+ (echo '#undef VERSION'; echo '#define VERSION "2.4.5"') >> plugins/patchlevel.h

Or I would replace that with this in the %setup:

date +'#define DATE "%d %b %y"' > plugins/patchlevel.h
# Note that the '' here is to prevent version interpolation by rpmbuild
rpm -q --qf='#define VERSION "%''{version}"\n' ppp >> plugins/patchlevel.h

You'll need to add 'BuildRequires: ppp' for the latter to work.

Comment 5 Charlie Brady 2012-08-30 18:15:54 UTC
There should be a very big warning somewhere that pptp should only be used with EAP/TLS authentication, and not MS CHAPv2:

https://www.cloudcracker.com/blog/2012/07/29/cracking-ms-chap-v2/

Comment 6 Charlie Brady 2012-08-30 18:16:34 UTC
> Dave, are you still interested / responsive regarding this review?

The answer seems to be a clear "no". :-)

Comment 7 Jaroslav Škarvada 2012-09-03 09:44:15 UTC
Well, in case Dave is no longer interested in this package, I can take it and maintain, but we need to find another reviewer :)

Comment 8 Charlie Brady 2012-09-03 14:38:07 UTC
I'd recommend you you collaborate with Paul Howarth, who maintains the RPM spec file which is distributed by upstream:

http://poptop.sourceforge.net/yum/stable/

[My bug reported in comment #4 above was fixed by Paul in his spec file in Jan 2007!]


* Fri May 21 2010 Paul Howarth <paul> - 1.3.4-2
- Define RPM macros in global scope
- Clarify license as GPL version 2 or later
- Add betabuild suffix to dist tag

* Fri Apr 20 2007 Paul Howarth <paul> - 1.3.4-1.1
- Rebuild against ppp 2.4.4

* Fri Apr 20 2007 Paul Howarth <paul> - 1.3.4-1
- Update to 1.3.4
- Use downloads.sf.net URL instead of dl.sf.net for source
- Use "install -p" to try to preserve upstream timestamps
- Remove bsdppp and slirp build options (package requires standard ppp)
- Remove ipalloc build option (not supported by upstream configure script)
- Use enable/disable rather than with/without for bcrelay configure option

* Wed Jan 10 2007 Paul Howarth <paul> - 1.3.3-2
- Use file-based build dependency on /usr/include/tcpd.h instead of
  tcp_wrappers package, since some distributions have this file in
  tcp_wrappers-devel
- Set VERSION using pppd's patchlevel.h rather than using the constant "2.4.3"
- Buildrequire /usr/include/pppd/patchlevel.h (recent-ish pppd)
- Add dependency on the exact version of ppp that pptpd is built against
- Use tabs rather than spaces for indentation

* Tue Sep 05 2006 Paul Howarth <paul> - 1.3.3-1
- Update to 1.3.3
- Add dist tag
- Add %postun scriptlet dependency for /sbin/service
- Fix doc permissions

[etc]

Comment 9 Jaroslav Škarvada 2012-09-03 16:15:45 UTC
(In reply to comment #8)
Thanks Charlie, CCed Paul. I would like to move this forward :)

Comment 10 Paul Howarth 2012-09-03 17:09:25 UTC
Hi Jaroslav,

please feel free to progress this yourself; I've never had an interest in pptpd and only took over the maintenance of the upstream package because there was nobody else to do it and I was looking after the upstream client package. Since that time, I don't even use the client any more...

Comment 11 Charlie Brady 2012-09-06 01:40:10 UTC
(In reply to comment #10)
> Hi Jaroslav,
> 
> please feel free to progress this yourself;

Paul, could you please attach your most recent spec file here? I don't see it in your repo.

The spec file bundled with pptpd-1.3.4.tar.gz isn't even current for 1.3.4:

...
%changelog
* Tue Sep  5 2006 Paul Howarth <paul> - 1.3.3-1
- Update to 1.3.3
- Add dist tag
- Add %%postun scriptlet dependency for /sbin/service
...

Comment 12 Paul Howarth 2012-09-06 06:42:39 UTC
Created attachment 610182 [details]
Current upstream spec

Yuk, this one shows its age. Macros for commands, still has %defattr. Lots of clean-up needed here!

Comment 13 Charlie Brady 2013-02-07 15:48:45 UTC
(In reply to comment #10)

> Hi Jaroslav,
> 
> please feel free to progress this yourself; I've never had an interest in
> pptpd and only took over the maintenance of the upstream package because
> there was nobody else to do it and I was looking after the upstream client
> package.

Upstream maintainer is:

James Cameron
quozl

He will probably be happy to accept patches.

Comment 14 Charlie Brady 2013-02-07 15:50:20 UTC
(In reply to comment #13)

> He will probably be happy to accept patches.

But probably would prefer to receive patches via:

poptop-server.net

Comment 15 Mathieu Bridon 2013-10-03 08:09:55 UTC
(In reply to Jaroslav Škarvada from comment #7)
> Well, in case Dave is no longer interested in this package, I can take it
> and maintain, but we need to find another reviewer :)

Hi there! :)

Jaroslav, I'm willing to review this package as I need it at $dayjob.

Can you submit a new spec/srpm?

Comment 16 Jaroslav Škarvada 2013-10-07 15:19:42 UTC
Hi Mathieu,

please take the review (edit the "assigned to:" field to yourself). I will try to prepare the new srpm during this week.

Comment 17 Mathieu Bridon 2013-10-07 15:24:28 UTC
Taken.

Comment 18 Paul Howarth 2013-10-07 15:30:08 UTC
Anyone here interested in maintaining the pptp client package? I'm currently still the maintainer of it but I don't use it any more myself.

Comment 19 Jaroslav Škarvada 2013-10-18 11:49:53 UTC
(In reply to Paul Howarth from comment #18)
> Anyone here interested in maintaining the pptp client package? I'm currently
> still the maintainer of it but I don't use it any more myself.

In case nobody is willing, I could take it to have it all :), but I am using it very rarely :)

Comment 20 Paul Howarth 2013-10-18 12:37:33 UTC
(In reply to Jaroslav Škarvada from comment #19)
> (In reply to Paul Howarth from comment #18)
> > Anyone here interested in maintaining the pptp client package? I'm currently
> > still the maintainer of it but I don't use it any more myself.
> 
> In case nobody is willing, I could take it to have it all :), but I am using
> it very rarely :)

Well "very rarely" is still more than me. If you want it, I'll orphan it and you can take it. Let me know, along with which branches you'd like.

Comment 21 Jaroslav Škarvada 2013-10-18 14:18:41 UTC
(In reply to Paul Howarth from comment #20)
> Well "very rarely" is still more than me. If you want it, I'll orphan it and
> you can take it. Let me know, along with which branches you'd like.

OK, go ahead. I can take all branches.

Comment 22 Paul Howarth 2013-10-18 14:21:18 UTC
(In reply to Jaroslav Škarvada from comment #21)
> (In reply to Paul Howarth from comment #20)
> > Well "very rarely" is still more than me. If you want it, I'll orphan it and
> > you can take it. Let me know, along with which branches you'd like.
> 
> OK, go ahead. I can take all branches.

OK, done.

Comment 23 Jaroslav Škarvada 2013-10-18 14:29:35 UTC
(In reply to Paul Howarth from comment #22)
> OK, done.
Taken.

Comment 24 Jaroslav Škarvada 2013-10-18 15:23:43 UTC
New version (based on the Paul's spec):
SPEC: http://jskarvad.fedorapeople.org/pptpd/pptpd.spec
SRPM: http://jskarvad.fedorapeople.org/pptpd/pptpd-1.3.4-3.fc19.src.rpm

Comment 25 Mathieu Bridon 2013-10-22 04:32:22 UTC
First, a couple of things outside of what fedora-review found.

Nothing in Fedora will read the init scripts any more, so please drop the
pptpd-sysvinit subpackage.  Eventually, you might want to wrap the sysv
initscript in el conditionals, but then the script must be shipped
**instead of** the systemd unit, not in addition to it.

You can pretty much remove the After=syslog.target, it became obsolete a while
ago.

Please drop the sysconfig file entirely. It is entirely unused by default, and
is completely unnecessary anyway as admins can just drop an ExecStart snippet
with the right options in /etc/systemd/system/pptpd.service.d/ if they need to
change the options.

Why the conditionals on libwrap and bcrelay? The Fedora buildsystem doesn't
really allow you to pass them, so that seems like useless complexity for
Fedora. Or is there a reason that I missed?

The TODO file doesn't seem useful as %doc in a Fedora context, please consider
dropping it.

Now to the fedora-review output...

Summary
=======

[!]: Package must own all directories that it creates.

    => pptpd installs %{_libdir}/pptpd/pptpd-logwtmp.so, but doesn't own
       %{_libdir}/pptpd

[!]: Each %files section contains %defattr if rpm < 4.4

    => Unless you expect the package to work on EL 5 (which might require
       quite a few other changes), please drop the %defattr line.

[!]: Package consistently uses macros (instead of hard-coded directory names).

    => In the %install section and the %files manifest, you have:

           /lib/systemd/system/pptpd.service
           %config(noreplace) /etc/pptpd.conf
           %config(noreplace) /etc/ppp/options.pptpd

       Please replace these by the proper macros.

[!]: Package contains the mandatory BuildRequires and Requires:.

    => Please add the mandatory requirement for Perl stuff:

        Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

[!]: Uses parallel make %{?_smp_mflags} macro.

    => Please fix.

[!]: Fully versioned dependency in subpackages if applicable.

    => The requirement on the base package in the pptpd-sysvinit subpackage
       should be explicitly arched, as you didn't make the subpackage noarch.

       That being said, please just drop this subpackage entirely.

[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see attached
     diff).

    => I only considered the spec file which was part of the SRPM for this
       review, in the future please make sure the two match.

[!]: Rpmlint is run on all installed packages.

     => Most of rpmlint's output can be safely ignored, there are just a few
        lines I want to single out.

     pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING

         => Please ask upstream to fix this.

     pptpd.x86_64: W: no-manual-page-for-binary foo

         => Please ask upstream to consider adding some. I will not block the
            review on this, though.

     pptpd.src:57: W: macro-in-comment %{_libdir}

         => Please use either %%, or make it "distros with libdir = ..." as
            the macro is not necessary for comprehension here.

     pptpd.src:74: E: hardcoded-library-path in %{buildroot}/lib/systemd/system

         => As mentioned previously, please use %{_unitdir} instead.

     pptpd.src: W: invalid-url Source0: http://downloads.sf.net/poptop/pptpd-1.3.4.tar.gz <urlopen error [Errno 101] Network is unreachable>

         => Ignore, I was having network problems at the time.

[!]: License field in the package spec file matches the actual license.

    The following files are GPLv2+:

    - bcrelay.c
    - plugins/pptpd-logwtmp.c
    - tools/vpnstats.pl

    The following files are under the LGPLv2+ license. (they are copied from
    the glibc)

    - getopt1.c
    - getopt.c
    - our_getopt.h

    The following files are under a BSD that I've never seen before, and isn't
    even in the wiki page:

    - plugins/pppd.h

        => This is copy-pasted from the ppp project, which is in Fedora. So
           you will have to either unbundle or ask FESCo for an exception.
           Note that there are differences with the file from the ppp-devel
           package.

        => Irrelevant from whether you can keep this bundled, I'm a bit
           worried about this BSD-like license. It's not in the Fedora wiki,
           so at the very least I'd like to ask Fedora Legal to take note of
           it. Also, it is similar to this one, which is said to be
           incompatible with the GPL:
            https://fedoraproject.org/wiki/Licensing:BSD#Advertising_Variant

    The following files are said to be copied from RFC 1662:

    - ppphdlc.c
    - ppphdlc.h

        => There are no license for these files, neither in pptpd nor in the
           text of the RFC (at least I couldn't find any). I'm not sure what
           that means, so I'd like to have Fedora Legal's advice on this.

    As for the rest of the files, they just don't have any copyright header.

    However, the man pages for pptpd(8) and pptpctrl(8) both state that pptpd
    is GPLv2+, so at the very least we can assume that the intent of the
    author is for the source files which get built as /usr/sbin/pptpd and
    /usr/sbin/pptpctrl to be GPLv2+. Looking at how the stuff is built, that
    would be:

    - compat.c
    - compat.h
    - configfile.c
    - configfile.h
    - ctrlpacket.c
    - ctrlpacket.h
    - defaults.h
    - inststr.c
    - inststr.h
    - our_syslog.h
    - pptpctrl.c
    - pptpctrl.c
    - pptpctrl.h
    - pptpd.c
    - pptpdefs.h
    - pptpgre.h
    - pptpmanager.c
    - pptpmanager.h
    - pqueue.c
    - pqueue.h

    Remaining are 3 files:

    - tools/pptp-portslave
    - tools/vpnstats
    - tools/vpnuser

        => Given that they come from the same authors, I'd be happy to assume
           that they are intended to be GPLv2+ like the rest of pptpd.

   => So, given all that, I'm honestly not sure what the right License tag
      should be for this package, or even if it's really acceptable in Fedora,
      so I'm blocking FE-LEGAL to ask for guidance.

[!]: Package contains no bundled libraries without FPC exception.

    => As mentioned in the licensing comments above, the package bundles
       plugins/pppd.h which comes from pppd-devel.

       Please try unbundling, or request an exception from FESCo.

       Note that there are differences between this file and the original one
       from the ppp-devel package.


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

MUST items
----------

C/C++:

[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[-]: Development (unversioned) .so files in -devel subpackage, if present.

    => The unversioned so file is some kind of internal plugin, and is not in
       the ld path.

[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:

[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.

    The following files are GPLv2+:

    - bcrelay.c
    - plugins/pptpd-logwtmp.c
    - tools/vpnstats.pl

    The following files are under the LGPLv2+ license. (they are copied from
    the glibc)

    - getopt1.c
    - getopt.c
    - our_getopt.h

    The following files are under a BSD that I've never seen before, and isn't
    even in the wiki page:

    - plugins/pppd.h

        => This is copy-pasted from the ppp project, which is in Fedora. So
           you will have to either unbundle or ask FESCo for an exception.
           Note that there are differences with the file from the ppp-devel
           package.

        => Irrelevant from whether you can keep this bundled, I'm a bit
           worried about this BSD-like license. It's not in the Fedora wiki,
           so at the very least I'd like to ask Fedora Legal to take note of
           it. Also, it is similar to this one, which is said to be
           incompatible with the GPL:
            https://fedoraproject.org/wiki/Licensing:BSD#Advertising_Variant

    The following files are said to be copied from RFC 1662:

    - ppphdlc.c
    - ppphdlc.h

        => There are no license for these files, neither in pptpd nor in the
           text of the RFC (at least I couldn't find any). I'm not sure what
           that means, so I'd like to have Fedora Legal's advice on this.

    As for the rest of the files, they just don't have any copyright header.

    However, the man pages for pptpd(8) and pptpctrl(8) both state that pptpd
    is GPLv2+, so at the very least we can assume that the intent of the
    author is for the source files which get built as /usr/sbin/pptpd and
    /usr/sbin/pptpctrl to be GPLv2+. Looking at how the stuff is built, that
    would be:

    - compat.c
    - compat.h
    - configfile.c
    - configfile.h
    - ctrlpacket.c
    - ctrlpacket.h
    - defaults.h
    - inststr.c
    - inststr.h
    - our_syslog.h
    - pptpctrl.c
    - pptpctrl.c
    - pptpctrl.h
    - pptpd.c
    - pptpdefs.h
    - pptpgre.h
    - pptpmanager.c
    - pptpmanager.h
    - pqueue.c
    - pqueue.h

    Remaining are 3 files:

    - tools/pptp-portslave
    - tools/vpnstats
    - tools/vpnuser

        => Given that they come from the same authors, I'd be happy to assume
           that they are intended to be GPLv2+ like the rest of pptpd.

   => So, given all that, I'm honestly not sure what the right License tag
      should be for this package, or even if it's really acceptable in Fedora,
      so I'm blocking FE-LEGAL.


[x]: License file installed when any subpackage combination is installed.
[x]: Package requires other packages for directories it uses.
[!]: Package must own all directories that it creates.

    => pptpd installs %{_libdir}/pptpd/pptpd-logwtmp.so, but doesn't own
       %{_libdir}/pptpd

[x]: %build honors applicable compiler flags or justifies otherwise.
[!]: Package contains no bundled libraries without FPC exception.

    => As mentioned in the licensing comments above, the package bundles
       plugins/pppd.h which comes from pppd-devel.

       Please try unbundling, or request an exception from FESCo.

       Note that there are differences between this file and the original one
       from the ppp-devel package.

[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[!]: Each %files section contains %defattr if rpm < 4.4

    => Unless you expect the package to work on EL 5 (which might require
       quite a few other changes), please drop the %defattr line.

[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[!]: Package consistently uses macros (instead of hard-coded directory names).

    => In the %install section and the %files manifest, you have:

           /lib/systemd/system/pptpd.service
           %config(noreplace) /etc/pptpd.conf
           %config(noreplace) /etc/ppp/options.pptpd

       Please replace these by the proper macros.

[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 81920 bytes in 14 files.
[!]: Package complies to the Packaging Guidelines

    => See all the stuff pointed out.

[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 is included in %doc.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[-]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Perl:

[!]: Package contains the mandatory BuildRequires and Requires:.

    => Please add the mandatory requirement for Perl stuff:

        Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))


SHOULD items
------------

Generic:

[x]: Sources can be downloaded from URI in Source: tag
[!]: Uses parallel make %{?_smp_mflags} macro.

    => Please fix.

[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[!]: Final provides and requires are sane (see attachments).

    => See the missing Perl requirement I mentioned above.

[!]: Fully versioned dependency in subpackages if applicable.

    => The requirement on the base package in the pptpd-sysvinit subpackage
       should be explicitly arched, as you didn't make the subpackage noarch.

       That being said, please just drop this subpackage entirely.

[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: SourceX tarball generation or download is documented.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.


EXTRA items
-----------

Generic:

[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see attached
     diff).

    => I only considered the spec file which was part of the SRPM for this
       review, in the future please make sure the two match.

[!]: Rpmlint is run on all installed packages.

     => Most of rpmlint's output can be safely ignored, there are just a few
        lines I want to single out.

     pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING

         => Please ask upstream to fix this.

     pptpd.x86_64: W: no-manual-page-for-binary vpnstats.pl

         => Please ask upstream to consider adding some. I will not block the
            review on this, though.

     pptpd.src:57: W: macro-in-comment %{_libdir}

         => Please use either %%, or make it "distros with libdir = ..." as
            the macro is not necessary for comprehension here.

     pptpd.src:74: E: hardcoded-library-path in %{buildroot}/lib/systemd/system

         => As mentioned previously, please use %{_unitdir} instead.

     pptpd.src: W: invalid-url Source0: http://downloads.sf.net/poptop/pptpd-1.3.4.tar.gz <urlopen error [Errno 101] Network is unreachable>
     
         => Ignore, I was having network problems at the time.

[-]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.


Rpmlint
-------

Checking: pptpd-1.3.4-3.fc21.x86_64.rpm
          pptpd-sysvinit-1.3.4-3.fc21.x86_64.rpm
          pptpd-1.3.4-3.fc21.src.rpm
pptpd.x86_64: W: spelling-error %description -l en_US firewalled -> fire walled, fire-walled, firewall ed
pptpd.x86_64: W: spelling-error %description -l en_US dialup -> dial up, dial-up, Dial
pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING
pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/README.bcrelay
pptpd.x86_64: W: no-manual-page-for-binary vpnstats.pl
pptpd.x86_64: W: no-manual-page-for-binary vpnuser
pptpd.x86_64: W: no-manual-page-for-binary bcrelay
pptpd.x86_64: W: no-manual-page-for-binary pptp-portslave
pptpd-sysvinit.x86_64: W: spelling-error %description -l en_US initscript -> inscription, postscript
pptpd-sysvinit.x86_64: W: no-documentation
pptpd.src: W: spelling-error %description -l en_US firewalled -> fire walled, fire-walled, firewall ed
pptpd.src: W: spelling-error %description -l en_US dialup -> dial up, dial-up, Dial
pptpd.src:57: W: macro-in-comment %{_libdir}
pptpd.src:74: E: hardcoded-library-path in %{buildroot}/lib/systemd/system
pptpd.src:91: E: hardcoded-library-path in %{buildroot}/lib/systemd/system
pptpd.src:128: E: hardcoded-library-path in /lib/systemd/system/pptpd.service
pptpd.src: W: invalid-url Source0: http://downloads.sf.net/poptop/pptpd-1.3.4.tar.gz <urlopen error [Errno 101] Network is unreachable>
3 packages and 0 specfiles checked; 5 errors, 12 warnings.


Rpmlint (installed packages)
----------------------------

# rpmlint pptpd-sysvinit pptpd
pptpd-sysvinit.x86_64: W: spelling-error %description -l en_US initscript -> inscription, postscript
pptpd-sysvinit.x86_64: W: no-documentation
pptpd.x86_64: W: spelling-error %description -l en_US firewalled -> fire walled, fire-walled, firewall ed
pptpd.x86_64: W: spelling-error %description -l en_US dialup -> dial up, dial-up, dial
pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING
pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/README.bcrelay
pptpd.x86_64: W: no-manual-page-for-binary vpnstats.pl
pptpd.x86_64: W: no-manual-page-for-binary vpnuser
pptpd.x86_64: W: no-manual-page-for-binary bcrelay
pptpd.x86_64: W: no-manual-page-for-binary pptp-portslave
2 packages and 0 specfiles checked; 2 errors, 8 warnings.
# echo 'rpmlint-done:'


Requires
--------

pptpd-sysvinit (rpmlib, GLIBC filtered):
    /bin/sh
    /sbin/service
    pptpd

pptpd (rpmlib, GLIBC filtered):
    /bin/bash
    /bin/sh
    /usr/bin/perl
    config(pptpd)
    libc.so.6()(64bit)
    libutil.so.1()(64bit)
    libwrap.so.0()(64bit)
    perl(strict)
    ppp
    rtld(GNU_HASH)
    systemd


Provides
--------

pptpd-sysvinit:
    pptpd-sysvinit
    pptpd-sysvinit(x86-64)

pptpd:
    config(pptpd)
    pptpd
    pptpd(x86-64)


Diff spec file in url and in SRPM
---------------------------------

--- /home/mathieu/Workspace/reviews/632853-pptpd/srpm/pptpd.spec	2013-10-21 12:51:26.620943163 +0800
+++ /home/mathieu/Workspace/reviews/632853-pptpd/srpm-unpacked/pptpd.spec	2013-10-18 23:03:41.000000000 +0800
@@ -75,5 +75,5 @@
 mkdir -p %{buildroot}%{_sysconfdir}/sysconfig
 
-make %{?_smp_mflags} \
+make \
 	DESTDIR=%{buildroot} \
 	INSTALL="install -p" \


Unversioned so-files
--------------------

pptpd: /usr/lib64/pptpd/pptpd-logwtmp.so

Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 632853
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++, Perl
Disabled plugins: Java, Python, SugarActivity, R, PHP, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG

Comment 26 Jaroslav Škarvada 2013-10-22 13:24:38 UTC
(In reply to Mathieu Bridon from comment #25)
Thank you for the detailed review.

> First, a couple of things outside of what fedora-review found.
> 
> Nothing in Fedora will read the init scripts any more, so please drop the
> pptpd-sysvinit subpackage.  Eventually, you might want to wrap the sysv
> initscript in el conditionals, but then the script must be shipped
> **instead of** the systemd unit, not in addition to it.
> 
Could you point me to the guidelines where is this literally written or is it only your personal opinion? Please note in [1] there is written something else.

> You can pretty much remove the After=syslog.target, it became obsolete a
> while
> ago.
> 
Removed.

> Please drop the sysconfig file entirely. It is entirely unused by default,
> and
> is completely unnecessary anyway as admins can just drop an ExecStart snippet
> with the right options in /etc/systemd/system/pptpd.service.d/ if they need
> to
> change the options.
> 
Any guidelines talking about this? Many packages do it this way. For me this is better option (and also backward compatible) and there is no need to edit service files.

> Why the conditionals on libwrap and bcrelay? The Fedora buildsystem doesn't
> really allow you to pass them, so that seems like useless complexity for
> Fedora. Or is there a reason that I missed?
> 
This is for people rebuilding the package for their needs. It's common practice and AFAIK it's not against guidelines.

> The TODO file doesn't seem useful as %doc in a Fedora context, please
> consider
> dropping it.
> 
It can stop users reporting known bugs. According to guidelines [2] the file doesn't qualify as irrelevant documentation.

> Now to the fedora-review output...
> 
> Summary
> =======
> 
> [!]: Package must own all directories that it creates.
> 
>     => pptpd installs %{_libdir}/pptpd/pptpd-logwtmp.so, but doesn't own
>        %{_libdir}/pptpd
> 
Fixed.

> [!]: Each %files section contains %defattr if rpm < 4.4
> 
>     => Unless you expect the package to work on EL 5 (which might require
>        quite a few other changes), please drop the %defattr line.
> 
Harmless and not against guidelines, however dropped.

> [!]: Package consistently uses macros (instead of hard-coded directory
> names).
> 
>     => In the %install section and the %files manifest, you have:
> 
>            /lib/systemd/system/pptpd.service
>            %config(noreplace) /etc/pptpd.conf
>            %config(noreplace) /etc/ppp/options.pptpd
> 
>        Please replace these by the proper macros.
Fixed.

> 
> [!]: Package contains the mandatory BuildRequires and Requires:.
> 
>     => Please add the mandatory requirement for Perl stuff:
> 
>         Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo
> $version))
> 
Added.

> [!]: Uses parallel make %{?_smp_mflags} macro.
> 
>     => Please fix.
Added previously, but not updated in the srpm.

> 
> [!]: Fully versioned dependency in subpackages if applicable.
> 
>     => The requirement on the base package in the pptpd-sysvinit subpackage
>        should be explicitly arched, as you didn't make the subpackage noarch.
> 
>        That being said, please just drop this subpackage entirely.
> 
Made it noarch.

> [!]: Spec file according to URL is the same as in SRPM.
>      Note: Spec file as given by url is not the same as in SRPM (see attached
>      diff).
> 
>     => I only considered the spec file which was part of the SRPM for this
>        review, in the future please make sure the two match.
> 
It seems older version of SRPM was uploaded.

> [!]: Rpmlint is run on all installed packages.
> 
>      => Most of rpmlint's output can be safely ignored, there are just a few
>         lines I want to single out.
> 
>      pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING
> 
>          => Please ask upstream to fix this.
I will forward this upstream.
> 
>      pptpd.x86_64: W: no-manual-page-for-binary foo
> 
>          => Please ask upstream to consider adding some. I will not block the
>             review on this, though.
Maybe todo/rfe.
> 
>      pptpd.src:57: W: macro-in-comment %{_libdir}
> 
>          => Please use either %%, or make it "distros with libdir = ..." as
>             the macro is not necessary for comprehension here.
> 
Fixed.

>      pptpd.src:74: E: hardcoded-library-path in
> %{buildroot}/lib/systemd/system
> 
>          => As mentioned previously, please use %{_unitdir} instead.
> 
Fixed.
>      pptpd.src: W: invalid-url Source0:
> http://downloads.sf.net/poptop/pptpd-1.3.4.tar.gz <urlopen error [Errno 101]
> Network is unreachable>
> 
>          => Ignore, I was having network problems at the time.
> 
URL not re-checked, I guess it's OK.

> [!]: License field in the package spec file matches the actual license.
> 
>     The following files are GPLv2+:
> 
>     - bcrelay.c
>     - plugins/pptpd-logwtmp.c
>     - tools/vpnstats.pl
> 
>     The following files are under the LGPLv2+ license. (they are copied from
>     the glibc)
> 
>     - getopt1.c
>     - getopt.c
>     - our_getopt.h
> 
IMHO the effective license of the resulting product can be GPLv2+ and this is compatible with the upstream.

>     The following files are under a BSD that I've never seen before, and
> isn't
>     even in the wiki page:
> 
>     - plugins/pppd.h
> 
>         => This is copy-pasted from the ppp project, which is in Fedora. So
>            you will have to either unbundle or ask FESCo for an exception.
>            Note that there are differences with the file from the ppp-devel
>            package.
>
Unbundled, I think we can and should do it. Thanks for the catch, I will point this upstream.

> 
>         => Irrelevant from whether you can keep this bundled, I'm a bit
>            worried about this BSD-like license. It's not in the Fedora wiki,
>            so at the very least I'd like to ask Fedora Legal to take note of
>            it. Also, it is similar to this one, which is said to be
>            incompatible with the GPL:
>             https://fedoraproject.org/wiki/Licensing:BSD#Advertising_Variant
>
Thanks for doing this. The last clause seems suspicious. Debian called this BSD-4-clause-like in their package (IMHO i.e. advertising like). But there is no way this should affect us now as it is unbundled

> 
>     The following files are said to be copied from RFC 1662:
> 
>     - ppphdlc.c
>     - ppphdlc.h
> 
>         => There are no license for these files, neither in pptpd nor in the
>            text of the RFC (at least I couldn't find any). I'm not sure what
>            that means, so I'd like to have Fedora Legal's advice on this.
>
Thanks. IMHO code from RFCs can be reused (otherwise the RFCs wouldn't make sense). The [3] says it explicitly together with the requirements for the RFCs published since 2005. This is very old RFC (from 1994), so I don't know what particularly apply to it, but I guess it wouldn't be illegal to make it part of the GPLv2+ licensed package especially if the origin of the code is clearly stated. But it is only my personal opinion, let's wait for the FL statement.

> 
>     As for the rest of the files, they just don't have any copyright header.
> 
>     However, the man pages for pptpd(8) and pptpctrl(8) both state that pptpd
>     is GPLv2+, so at the very least we can assume that the intent of the
>     author is for the source files which get built as /usr/sbin/pptpd and
>     /usr/sbin/pptpctrl to be GPLv2+. Looking at how the stuff is built, that
>     would be:
> 
>     - compat.c
>     - compat.h
>     - configfile.c
>     - configfile.h
>     - ctrlpacket.c
>     - ctrlpacket.h
>     - defaults.h
>     - inststr.c
>     - inststr.h
>     - our_syslog.h
>     - pptpctrl.c
>     - pptpctrl.c
>     - pptpctrl.h
>     - pptpd.c
>     - pptpdefs.h
>     - pptpgre.h
>     - pptpmanager.c
>     - pptpmanager.h
>     - pqueue.c
>     - pqueue.h
> 
>     Remaining are 3 files:
> 
>     - tools/pptp-portslave
>     - tools/vpnstats
>     - tools/vpnuser
> 
>         => Given that they come from the same authors, I'd be happy to assume
>            that they are intended to be GPLv2+ like the rest of pptpd.
> 
Correct.

>    => So, given all that, I'm honestly not sure what the right License tag
>       should be for this package, or even if it's really acceptable in
> Fedora,
>       so I'm blocking FE-LEGAL to ask for guidance.
> 
> [!]: Package contains no bundled libraries without FPC exception.
> 
>     => As mentioned in the licensing comments above, the package bundles
>        plugins/pppd.h which comes from pppd-devel.
> 
>        Please try unbundling, or request an exception from FESCo.
> 
>        Note that there are differences between this file and the original one
>        from the ppp-devel package.

It is the same version as ppp upstream used. The only difference is in the version. It bundles the header from the ppp-2.4.2, but the latest is ppp-2.4.5. As it is used as a plug-in to be loaded into ppp, I think it needs to be the same version as the ppp. I unbundled it and will point this upstream.

> 
> 
> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> MUST items
> ----------
> 
> C/C++:
> 
> [x]: Package does not contain kernel modules.
> [x]: Package contains no static executables.
> [-]: Development (unversioned) .so files in -devel subpackage, if present.
> 
>     => The unversioned so file is some kind of internal plugin, and is not in
>        the ld path.
Correct.
> 
> [x]: Package does not contain any libtool archives (.la)
> [x]: Rpath absent or only used for internal libs.
> 
> Generic:
> 
> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.
> [!]: License field in the package spec file matches the actual license.
> 
>     The following files are GPLv2+:
> 
>     - bcrelay.c
>     - plugins/pptpd-logwtmp.c
>     - tools/vpnstats.pl
> 
>     The following files are under the LGPLv2+ license. (they are copied from
>     the glibc)
> 
>     - getopt1.c
>     - getopt.c
>     - our_getopt.h
> 
>     The following files are under a BSD that I've never seen before, and
> isn't
>     even in the wiki page:
> 
>     - plugins/pppd.h
> 
>         => This is copy-pasted from the ppp project, which is in Fedora. So
>            you will have to either unbundle or ask FESCo for an exception.
>            Note that there are differences with the file from the ppp-devel
>            package.
> 
>         => Irrelevant from whether you can keep this bundled, I'm a bit
>            worried about this BSD-like license. It's not in the Fedora wiki,
>            so at the very least I'd like to ask Fedora Legal to take note of
>            it. Also, it is similar to this one, which is said to be
>            incompatible with the GPL:
>             https://fedoraproject.org/wiki/Licensing:BSD#Advertising_Variant
> 
>     The following files are said to be copied from RFC 1662:
> 
>     - ppphdlc.c
>     - ppphdlc.h
> 
>         => There are no license for these files, neither in pptpd nor in the
>            text of the RFC (at least I couldn't find any). I'm not sure what
>            that means, so I'd like to have Fedora Legal's advice on this.
> 
>     As for the rest of the files, they just don't have any copyright header.
> 
>     However, the man pages for pptpd(8) and pptpctrl(8) both state that pptpd
>     is GPLv2+, so at the very least we can assume that the intent of the
>     author is for the source files which get built as /usr/sbin/pptpd and
>     /usr/sbin/pptpctrl to be GPLv2+. Looking at how the stuff is built, that
>     would be:
> 
>     - compat.c
>     - compat.h
>     - configfile.c
>     - configfile.h
>     - ctrlpacket.c
>     - ctrlpacket.h
>     - defaults.h
>     - inststr.c
>     - inststr.h
>     - our_syslog.h
>     - pptpctrl.c
>     - pptpctrl.c
>     - pptpctrl.h
>     - pptpd.c
>     - pptpdefs.h
>     - pptpgre.h
>     - pptpmanager.c
>     - pptpmanager.h
>     - pqueue.c
>     - pqueue.h
> 
>     Remaining are 3 files:
> 
>     - tools/pptp-portslave
>     - tools/vpnstats
>     - tools/vpnuser
> 
>         => Given that they come from the same authors, I'd be happy to assume
>            that they are intended to be GPLv2+ like the rest of pptpd.
> 
>    => So, given all that, I'm honestly not sure what the right License tag
>       should be for this package, or even if it's really acceptable in
> Fedora,
>       so I'm blocking FE-LEGAL.
> 
Comments are above.

> 
> [x]: License file installed when any subpackage combination is installed.
> [x]: Package requires other packages for directories it uses.
> [!]: Package must own all directories that it creates.
> 
>     => pptpd installs %{_libdir}/pptpd/pptpd-logwtmp.so, but doesn't own
>        %{_libdir}/pptpd
> 
Comment above.

> [x]: %build honors applicable compiler flags or justifies otherwise.
> [!]: Package contains no bundled libraries without FPC exception.
> 
>     => As mentioned in the licensing comments above, the package bundles
>        plugins/pppd.h which comes from pppd-devel.
> 
>        Please try unbundling, or request an exception from FESCo.
> 
>        Note that there are differences between this file and the original one
>        from the ppp-devel package.
Comments above.

> 
> [x]: Changelog in prescribed format.
> [x]: Sources contain only permissible code or content.
> [!]: Each %files section contains %defattr if rpm < 4.4
> 
>     => Unless you expect the package to work on EL 5 (which might require
>        quite a few other changes), please drop the %defattr line.
> 
Comment above.

> [-]: Package contains desktop file if it is a GUI application.
> [-]: Development files must be in a -devel package
> [x]: Package uses nothing in %doc for runtime.
> [!]: Package consistently uses macros (instead of hard-coded directory
> names).
> 
>     => In the %install section and the %files manifest, you have:
> 
>            /lib/systemd/system/pptpd.service
>            %config(noreplace) /etc/pptpd.conf
>            %config(noreplace) /etc/ppp/options.pptpd
> 
>        Please replace these by the proper macros.
> 
Comments above.

> [x]: Package is named according to the Package Naming Guidelines.
> [x]: Package does not generate any conflict.
> [x]: Package obeys FHS, except libexecdir and /usr/target.
> [-]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> [x]: Requires correct, justified where necessary.
> [x]: Spec file is legible and written in American English.
> [x]: Package contains systemd file(s) if in need.
> [x]: Useful -debuginfo package or justification otherwise.
> [x]: Package is not known to require an ExcludeArch tag.
> [-]: Large documentation must go in a -doc subpackage. Large could be size
>      (~1MB) or number of files.
>      Note: Documentation size is 81920 bytes in 14 files.
> [!]: Package complies to the Packaging Guidelines
> 
>     => See all the stuff pointed out.
> 
> [x]: Package successfully compiles and builds into binary rpms on at least
> one
>      supported primary architecture.
> [x]: Package installs properly.
> [x]: Rpmlint is run on all rpms the build produces.
>      Note: There are rpmlint messages (see attachment).
> [x]: 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 is included in %doc.
> [x]: Package does not own files or directories owned by other packages.
> [x]: All build dependencies are listed in BuildRequires, except for any that
>      are listed in the exceptions section of Packaging Guidelines.
> [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
> [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
> [x]: %config files are marked noreplace or the reason is justified.
> [-]: Macros in Summary, %description expandable at SRPM build time.
> [x]: Package does not contain duplicates in %files.
> [x]: Permissions on files are set properly.
> [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
>      work.
> [x]: Package is named using only allowed ASCII characters.
> [x]: No %config files under /usr.
> [x]: Package do not use a name that already exist
> [x]: Package is not relocatable.
> [x]: Sources used to build the package match the upstream source, as provided
>      in the spec URL.
> [x]: Spec file name must match the spec package %{name}, in the format
>      %{name}.spec.
> [x]: File names are valid UTF-8.
> [x]: Packages must not store files under /srv, /opt or /usr/local
> 
> Perl:
> 
> [!]: Package contains the mandatory BuildRequires and Requires:.
> 
>     => Please add the mandatory requirement for Perl stuff:
> 
>         Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo
> $version))
> 
Comments above.

> 
> SHOULD items
> ------------
> 
> Generic:
> 
> [x]: Sources can be downloaded from URI in Source: tag
> [!]: Uses parallel make %{?_smp_mflags} macro.
> 
>     => Please fix.
Comment above.

> 
> [-]: If the source package does not include license text(s) as a separate
> file
>      from upstream, the packager SHOULD query upstream to include it.
> [!]: Final provides and requires are sane (see attachments).
> 
>     => See the missing Perl requirement I mentioned above.
> 
Comment above.

> [!]: Fully versioned dependency in subpackages if applicable.
> 
>     => The requirement on the base package in the pptpd-sysvinit subpackage
>        should be explicitly arched, as you didn't make the subpackage noarch.
> 
>        That being said, please just drop this subpackage entirely.
> 
Comments above.

> [?]: Package functions as described.
> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [x]: Scriptlets must be sane, if used.
> [-]: SourceX tarball generation or download is documented.
> [x]: Package should compile and build into binary rpms on all supported
>      architectures.
> [-]: %check is present and all tests pass.
> [x]: Packages should try to preserve timestamps of original installed files.
> [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: Reviewer should test that the package builds in mock.
> [x]: Buildroot is not present
> [x]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
> [x]: Dist tag is present (not strictly required in GL).
> [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
> [x]: SourceX is a working URL.
> [x]: Spec use %global instead of %define unless justified.
> 
> 
> EXTRA items
> -----------
> 
> Generic:
> 
> [!]: Spec file according to URL is the same as in SRPM.
>      Note: Spec file as given by url is not the same as in SRPM (see attached
>      diff).
> 
>     => I only considered the spec file which was part of the SRPM for this
>        review, in the future please make sure the two match.
> 
> [!]: Rpmlint is run on all installed packages.
> 
>      => Most of rpmlint's output can be safely ignored, there are just a few
>         lines I want to single out.
> 
>      pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING
> 
>          => Please ask upstream to fix this.
> 
>      pptpd.x86_64: W: no-manual-page-for-binary vpnstats.pl
> 
>          => Please ask upstream to consider adding some. I will not block the
>             review on this, though.
> 
>      pptpd.src:57: W: macro-in-comment %{_libdir}
> 
>          => Please use either %%, or make it "distros with libdir = ..." as
>             the macro is not necessary for comprehension here.
> 
>      pptpd.src:74: E: hardcoded-library-path in
> %{buildroot}/lib/systemd/system
> 
>          => As mentioned previously, please use %{_unitdir} instead.
> 
>      pptpd.src: W: invalid-url Source0:
> http://downloads.sf.net/poptop/pptpd-1.3.4.tar.gz <urlopen error [Errno 101]
> Network is unreachable>
>      
>          => Ignore, I was having network problems at the time.
> 
> [-]: Large data in /usr/share should live in a noarch subpackage if package
> is
>      arched.
All commented above.

> 
> 
> Rpmlint
> -------
> 
> Checking: pptpd-1.3.4-3.fc21.x86_64.rpm
>           pptpd-sysvinit-1.3.4-3.fc21.x86_64.rpm
>           pptpd-1.3.4-3.fc21.src.rpm
> pptpd.x86_64: W: spelling-error %description -l en_US firewalled -> fire
> walled, fire-walled, firewall ed
> pptpd.x86_64: W: spelling-error %description -l en_US dialup -> dial up,
> dial-up, Dial
> pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING
> pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/README.bcrelay
> pptpd.x86_64: W: no-manual-page-for-binary vpnstats.pl
> pptpd.x86_64: W: no-manual-page-for-binary vpnuser
> pptpd.x86_64: W: no-manual-page-for-binary bcrelay
> pptpd.x86_64: W: no-manual-page-for-binary pptp-portslave
> pptpd-sysvinit.x86_64: W: spelling-error %description -l en_US initscript ->
> inscription, postscript
> pptpd-sysvinit.x86_64: W: no-documentation
> pptpd.src: W: spelling-error %description -l en_US firewalled -> fire
> walled, fire-walled, firewall ed
> pptpd.src: W: spelling-error %description -l en_US dialup -> dial up,
> dial-up, Dial
> pptpd.src:57: W: macro-in-comment %{_libdir}
> pptpd.src:74: E: hardcoded-library-path in %{buildroot}/lib/systemd/system
> pptpd.src:91: E: hardcoded-library-path in %{buildroot}/lib/systemd/system
> pptpd.src:128: E: hardcoded-library-path in /lib/systemd/system/pptpd.service
> pptpd.src: W: invalid-url Source0:
> http://downloads.sf.net/poptop/pptpd-1.3.4.tar.gz <urlopen error [Errno 101]
> Network is unreachable>
> 3 packages and 0 specfiles checked; 5 errors, 12 warnings.
> 
> 
> Rpmlint (installed packages)
> ----------------------------
> 
> # rpmlint pptpd-sysvinit pptpd
> pptpd-sysvinit.x86_64: W: spelling-error %description -l en_US initscript ->
> inscription, postscript
> pptpd-sysvinit.x86_64: W: no-documentation
> pptpd.x86_64: W: spelling-error %description -l en_US firewalled -> fire
> walled, fire-walled, firewall ed
> pptpd.x86_64: W: spelling-error %description -l en_US dialup -> dial up,
> dial-up, dial
> pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/COPYING
> pptpd.x86_64: E: incorrect-fsf-address /usr/share/doc/pptpd/README.bcrelay
> pptpd.x86_64: W: no-manual-page-for-binary vpnstats.pl
> pptpd.x86_64: W: no-manual-page-for-binary vpnuser
> pptpd.x86_64: W: no-manual-page-for-binary bcrelay
> pptpd.x86_64: W: no-manual-page-for-binary pptp-portslave
> 2 packages and 0 specfiles checked; 2 errors, 8 warnings.
> # echo 'rpmlint-done:'
> 
> 
> Requires
> --------
> 
> pptpd-sysvinit (rpmlib, GLIBC filtered):
>     /bin/sh
>     /sbin/service
>     pptpd
> 
> pptpd (rpmlib, GLIBC filtered):
>     /bin/bash
>     /bin/sh
>     /usr/bin/perl
>     config(pptpd)
>     libc.so.6()(64bit)
>     libutil.so.1()(64bit)
>     libwrap.so.0()(64bit)
>     perl(strict)
>     ppp
>     rtld(GNU_HASH)
>     systemd
> 
> 
> Provides
> --------
> 
> pptpd-sysvinit:
>     pptpd-sysvinit
>     pptpd-sysvinit(x86-64)
> 
> pptpd:
>     config(pptpd)
>     pptpd
>     pptpd(x86-64)
> 
> 
> Diff spec file in url and in SRPM
> ---------------------------------
> 
> --- /home/mathieu/Workspace/reviews/632853-pptpd/srpm/pptpd.spec	2013-10-21
> 12:51:26.620943163 +0800
> +++ /home/mathieu/Workspace/reviews/632853-pptpd/srpm-unpacked/pptpd.spec
> 2013-10-18 23:03:41.000000000 +0800
> @@ -75,5 +75,5 @@
>  mkdir -p %{buildroot}%{_sysconfdir}/sysconfig
>  
> -make %{?_smp_mflags} \
> +make \
>  	DESTDIR=%{buildroot} \
>  	INSTALL="install -p" \
> 
> 
> Unversioned so-files
> --------------------
> 
> pptpd: /usr/lib64/pptpd/pptpd-logwtmp.so
> 
> Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
> Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 632853
> Buildroot used: fedora-rawhide-x86_64
> Active plugins: Generic, Shell-api, C/C++, Perl
> Disabled plugins: Java, Python, SugarActivity, R, PHP, Ruby
> Disabled flags: EPEL5, EXARCH, DISTTAG

SPEC: http://jskarvad.fedorapeople.org/pptpd/pptpd.spec
SRPM: http://jskarvad.fedorapeople.org/pptpd/pptpd-1.3.4-4.fc19.src.rpm


[1] https://fedoraproject.org/wiki/Packaging:SysVInitScript#InitscriptsInAdditionToSystemdUnitFiles

[2] https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Documentation

[3] http://trustee.ietf.org/docs/Copyright-FAQ-2010-6-22.pdf

Comment 27 Paul Howarth 2013-10-22 13:52:23 UTC
(In reply to Jaroslav Škarvada from comment #26)
> (In reply to Mathieu Bridon from comment #25)
> > [!]: Each %files section contains %defattr if rpm < 4.4
> > 
> >     => Unless you expect the package to work on EL 5 (which might require
> >        quite a few other changes), please drop the %defattr line.
> > 
> Harmless and not against guidelines, however dropped.

EL-5 has rpm ≥ 4.4 and does not need %defattr anyway. It was last needed for EPEL-4.

> > [!]: Package contains the mandatory BuildRequires and Requires:.
> > 
> >     => Please add the mandatory requirement for Perl stuff:
> > 
> >         Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo
> > $version))
> > 
> Added.

I don't think this is necessary. The perl(:MODULE_COMPAT_*) requirement is to ensure that packages shipping perl modules will require a version of perl that can find those modules and, in the case of arch-specific modules, be ABI-compliant with them. Packages just shipping perl scripts like this one don't have such a tight version dependency and shouldn't need it to be specified like this. On the other hand. it's harmless to include it, apart from the dependency bloat.

Comment 28 Charlie Brady 2013-10-22 14:17:38 UTC
> On the other hand. it's harmless to include it, apart from the dependency bloat.

And dependency bloat is a bad thing - it can only hurt.

Comment 29 Mathieu Bridon 2013-10-23 08:22:11 UTC
Note: Summary of remaining issues at the end of this comment.

(In reply to Paul Howarth from comment #27)
> (In reply to Jaroslav Škarvada from comment #26)
> > (In reply to Mathieu Bridon from comment #25)
> > > [!]: Package contains the mandatory BuildRequires and Requires:.
> > > 
> > >     => Please add the mandatory requirement for Perl stuff:
> > > 
> > >         Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo
> > > $version))
> > > 
> > Added.
> 
> I don't think this is necessary. The perl(:MODULE_COMPAT_*) requirement is
> to ensure that packages shipping perl modules will require a version of perl
> that can find those modules and, in the case of arch-specific modules, be
> ABI-compliant with them. Packages just shipping perl scripts like this one
> don't have such a tight version dependency and shouldn't need it to be
> specified like this. On the other hand. it's harmless to include it, apart
> from the dependency bloat.

Thanks Paul, I did not know about that.

Jaroslav, I'm sorry I made you add something unneeded, feel free to remove it.

(In reply to Jaroslav Škarvada from comment #26)
> (In reply to Mathieu Bridon from comment #25)
> > First, a couple of things outside of what fedora-review found.
> > 
> > Nothing in Fedora will read the init scripts any more, so please drop the
> > pptpd-sysvinit subpackage.  Eventually, you might want to wrap the sysv
> > initscript in el conditionals, but then the script must be shipped
> > **instead of** the systemd unit, not in addition to it.
> > 
> Could you point me to the guidelines where is this literally written or is
> it only your personal opinion? Please note in [1] there is written something
> else.

True, but that guideline was written at a time where we still had upstart in
the distribution as an alternative init system.

That's not the case any more with at least Fedora >= 18, i.e all currently
supported releases of Fedora. So really, that -sysvinit subpackage is just
useless bloat.

> > Please drop the sysconfig file entirely. It is entirely unused by default,
> > and
> > is completely unnecessary anyway as admins can just drop an ExecStart snippet
> > with the right options in /etc/systemd/system/pptpd.service.d/ if they need
> > to
> > change the options.
> > 
> Any guidelines talking about this? Many packages do it this way. For me this
> is better option (and also backward compatible) and there is no need to edit
> service files.

Backwards compatible with what? The package is not in Fedora yet.

Also, /etc/sysconfig is a RHEL/Fedora-ism, so it's not compatible with other
distributions.

I think you're confusing "backwards-compatible" with "legacy distro-specific".

Again, the solution I suggested does not require you to edit service files, it
would just require the admin to put the following in
/etc/systemd/system/pptpd.service.d/foobar.conf :

    [Service]
    ExecStart=/usr/sbin/pptpd --my-super-option

That's not harder at all than the sysconfig file, and it's the preferred
cross-platform way of doing local configurations.

It also allows much more than what the sysconfig file allows.

Say your local deployment needs to add a dependency to another unit for some
reason, you'd add the appropriate After=/Requires= to your
/etc/systemd/system/pptpd.service.d/foobar.conf. Then if you also need to pass
an option the service in the sysconfig file, you end up with two locations for
local configuration of the service instead of just one.

Really, in many cases, /etc/sysconfig is not useful any more and should be
avoided to prevent confusion. This is one of these cases.

> > Why the conditionals on libwrap and bcrelay? The Fedora buildsystem doesn't
> > really allow you to pass them, so that seems like useless complexity for
> > Fedora. Or is there a reason that I missed?
> > 
> This is for people rebuilding the package for their needs. It's common
> practice and AFAIK it's not against guidelines.

Agreed, I was just asking because the need was not obvious to me. :)

But I'm certainly fine with it.

> > The TODO file doesn't seem useful as %doc in a Fedora context, please
> > consider
> > dropping it.
> > 
> It can stop users reporting known bugs. According to guidelines [2] the file
> doesn't qualify as irrelevant documentation.

Heh, I guess.

----------

Only in 632853-pptpd/srpm-unpacked/: pptpd-1.3.4-pppd-unbundle.patch

    => Patch looks good. It would be great to have a link to the upstream bug
       report or mailing-list discussion about it, but I won't block the
       review on this.

diff -ur 632853-pptpd.c24/srpm-unpacked/pptpd.spec 632853-pptpd/srpm-unpacked/pptpd.spec
--- 632853-pptpd.c24/srpm-unpacked/pptpd.spec	2013-10-18 23:03:41.000000000 +0800
+++ 632853-pptpd/srpm-unpacked/pptpd.spec	2013-10-22 21:17:46.000000000 +0800
@@ -14,17 +14,20 @@
 Summary:	PoPToP Point to Point Tunneling Server
 Name:		pptpd
 Version:	1.3.4
-Release:	3%{?dist}
+Release:	4%{?dist}
 License:	GPLv2+
 Group:		Applications/Internet
+BuildRequires:	ppp-devel, systemd
 URL:		http://poptop.sourceforge.net/
 Source0:	http://downloads.sf.net/poptop/pptpd-%{version}.tar.gz
 Source1:	pptpd.service
 Source2:	pptpd.sysconfig
-BuildRequires:	ppp-devel, systemd
-
+# Unbundle pppd.h
+Patch0:		pptpd-1.3.4-pppd-unbundle.patch

    => Thanks for unbundling.

 %global pppver %((%{__awk} '/^#define VERSION/ { print $NF }' /usr/include/pppd/patchlevel.h 2>/dev/null||echo none)|/usr/bin/tr -d '"')
 Requires:	ppp = %{pppver}
+Requires:	perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

    => As Paul mentioned, this was not useful. Sorry again to have made you
       add it. :(

       I won't block the review on this, just drop it before importing the
       package in Fedora.

+BuildRequires:	ppp-devel, systemd

    => You've already wrote that line above, once should be enough? :)
 
 %if %{?_without_libwrap:0}%{!?_without_libwrap:1}
 BuildRequires:	tcp_wrappers-devel
@@ -42,6 +45,7 @@
 %package sysvinit
 Summary: PoPToP Point to Point Tunneling Server
 Group: Applications/Internet
+BuildArch: noarch

    => That fixes the incorrect arch-specific requirement.

       I still think the subpackage should be dropped entirely.

 Requires: %{name} = %{version}-%{release}
 Requires(preun): /sbin/service
 
@@ -51,10 +55,14 @@
 %prep
 %setup -q
 
+# Unbundle pppd.h
+rm -f plugins/pppd.h
+%patch0 -p1 -b .pppd-unbundle
+

    => Ack.

 # Fix permissions
 chmod 644 *.[ch] samples/pptpd.conf AUTHORS
 
-# Fix for distros with %{_libdir} = /usr/lib64
+# Fix for distros with %%{_libdir} = /usr/lib64

    => Ack.

 perl -pi -e 's,/usr/lib/pptpd,%{_libdir}/pptpd,;' pptpctrl.c
 
 %build
@@ -67,28 +75,28 @@
 make CFLAGS='-fno-builtin -fPIC -DSBINDIR=\"%{_sbindir}\" %{optflags} %{?harden}'
 
 %install
-mkdir -p %{buildroot}/etc/rc.d/init.d
-mkdir -p %{buildroot}/etc/ppp
+mkdir -p %{buildroot}%{_sysconfdir}/rc.d/init.d
+mkdir -p %{buildroot}%{_sysconfdir}//ppp

    => This fixes the non-usage of macros.

       Note that you wrote "//" above. I won't block the review on this, just
       change it the before importing the package in Fedora.

 mkdir -p %{buildroot}%{_bindir}
 mkdir -p %{buildroot}%{_mandir}/man{5,8}
-mkdir -p %{buildroot}/lib/systemd/system
+mkdir -p %{buildroot}%{_unitdir}

    => This fixes the non-usage of macros.

 mkdir -p %{buildroot}%{_sysconfdir}/sysconfig
 
-make \
+make %{?_smp_mflags} \

    => This fixes the non-parallel build.

 	DESTDIR=%{buildroot} \
 	INSTALL="install -p" \
 	LIBDIR=%{buildroot}%{_libdir}/pptpd \
 	install
-install -pm 0755 pptpd.init %{buildroot}/etc/rc.d/init.d/pptpd
-install -pm 0644 samples/pptpd.conf %{buildroot}/etc/pptpd.conf
-install -pm 0644 samples/options.pptpd %{buildroot}/etc/ppp/options.pptpd
+install -pm 0755 pptpd.init %{buildroot}%{_sysconfdir}/rc.d/init.d/pptpd
+install -pm 0644 samples/pptpd.conf %{buildroot}%{_sysconfdir}/pptpd.conf
+install -pm 0644 samples/options.pptpd %{buildroot}%{_sysconfdir}/ppp/options.pptpd

    => This fixes the non-usage of macros.

 install -pm 0755 tools/vpnuser %{buildroot}%{_bindir}/vpnuser
 install -pm 0755 tools/vpnstats.pl %{buildroot}%{_bindir}/vpnstats.pl
 install -pm 0755 tools/pptp-portslave %{buildroot}%{_sbindir}/pptp-portslave
 install -pm 0644 pptpd.conf.5 %{buildroot}%{_mandir}/man5/pptpd.conf.5
 install -pm 0644 pptpd.8 %{buildroot}%{_mandir}/man8/pptpd.8
 install -pm 0644 pptpctrl.8 %{buildroot}%{_mandir}/man8/pptpctrl.8
-install -pm 0644 %{SOURCE1} %{buildroot}/lib/systemd/system
+install -pm 0644 %{SOURCE1} %{buildroot}%{_unitdir}

    => This fixes the non-usage of macros.

 install -pm 0644 %{SOURCE2} %{buildroot}%{_sysconfdir}/sysconfig/pptpd
 
 %post
@@ -113,27 +121,31 @@
 [ "$1" -ge 1 ] && %{_initrddir}/pptpd condrestart >/dev/null 2>&1 ||:
 
 %files
-%defattr(-,root,root,-)

    => Ack.

 %doc AUTHORS COPYING README* TODO ChangeLog* samples
 %{_sbindir}/pptpd
 %{_sbindir}/pptpctrl
 %{_sbindir}/pptp-portslave
 %{!?_without_bcrelay:%{_sbindir}/bcrelay}
+%dir %{_libdir}/pptpd

    => This fixes the unowned dir.

 %{_libdir}/pptpd/pptpd-logwtmp.so
 %{_bindir}/vpnuser
 %{_bindir}/vpnstats.pl
 %{_mandir}/man5/pptpd.conf.5*
 %{_mandir}/man8/pptpd.8*
 %{_mandir}/man8/pptpctrl.8*
-/lib/systemd/system/pptpd.service
+%{_unitdir}/pptpd.service

    => This fixes the non-usage of macros.

 %config(noreplace) %{_sysconfdir}/sysconfig/pptpd
-%config(noreplace) /etc/pptpd.conf
-%config(noreplace) /etc/ppp/options.pptpd
+%config(noreplace) %{_sysconfdir}/pptpd.conf
+%config(noreplace) %{_sysconfdir}/ppp/options.pptpd

    => This fixes the non-usage of macros.

 %files sysvinit
 %attr(0755,root,root) %{_sysconfdir}/rc.d/init.d/pptpd
 
 %changelog
+* Tue Oct 22 2013 Jaroslav Škarvada <jskarvad> - 1.3.4-4
+- Various fixes according to Fedora review
+  Related: rhbz#632853
+
 * Fri Oct 18 2013 Jaroslav Škarvada <jskarvad> - 1.3.4-3
 - Modified for Fedora
   Resolves: rhbz#632853

----------

In summary, the remaining issues I have with the packaging are the following.

1. The -sysvinit subpackage should just be dropped.

I won't fight for this, but once again that's just useless bloat.

In the future, -sysvinit subpackages might be forbidden by the packaging
guidelines, at which point you'd have to drop it anyway.

For what is worth, I opened a request for a change of the relevant packaging
guidelines:

    https://fedorahosted.org/fpc/ticket/359

In any case, I won't block the review on this, because the current guidelines
don't forbid it explicitly.

2. The sysconfig file should just be dropped.

Here as well I won't fight for it or block the review on this, because the
guidelines don't explicitly forbid it.

But again, this is just legacy Fedora/RHEL-specific stuff which is entirely
unneeded and confusing.

3. The double BuildRequires: ppp-devel, systemd

I won't block the review on this, just drop one of them when you import the
package in Fedora.

4. The explicit perl requirement I made you add.

Not blocking on this one since it's my fault, just drop it when you import the
package in Fedora.

5. The bad FSF address.

You said you'll notify upstream about it, I'd appreciate a comment in the spec
file above the License tag with a link to the upstream bug report or mailing
list archive.

I won't block the review on this, though, feel free to add it when importing
the package in Fedora. ;)

6. The legal situation.

We're just waiting on Fedora Legal to confirm that there are no problems here,
I just don't feel confident in my legal analysis of this package.

This is the only thing blocking the approval of this package as far as I'm
concerned.

Comment 30 Paul Howarth 2013-10-23 08:54:43 UTC
Upstream has just released version 1.4.0 by the way.

Comment 31 Jaroslav Škarvada 2013-10-23 09:31:16 UTC
(In reply to Paul Howarth from comment #30)
> Upstream has just released version 1.4.0 by the way.

OK, thanks for info, I will update the package.

All the requests (unbundling and FSF address change) were upstreamed (the former a while ago).

Comment 32 Paul Howarth 2013-10-23 15:30:48 UTC
In the service file, why not use "pptpd --fg" and then "Type=forking" wouldn't be needed?

Comment 33 Jaroslav Škarvada 2013-10-25 09:57:44 UTC
New version:
- rebased to 1.4.0 sources
- running pptpd in foreground too see its output in the journal
- used requires: perl
- fixed typos (double slash and double buildrequires)

SPEC: http://jskarvad.fedorapeople.org/pptpd/pptpd.spec
SRPM: http://jskarvad.fedorapeople.org/pptpd/pptpd-1.4.0-1.fc19.src.rpm

Comment 34 Mathieu Bridon 2013-10-29 07:41:30 UTC
(In reply to Jaroslav Škarvada from comment #33)
> New version:
> - rebased to 1.4.0 sources
> - running pptpd in foreground too see its output in the journal
> - used requires: perl
> - fixed typos (double slash and double buildrequires)
> 
> SPEC: http://jskarvad.fedorapeople.org/pptpd/pptpd.spec
> SRPM: http://jskarvad.fedorapeople.org/pptpd/pptpd-1.4.0-1.fc19.src.rpm

--- pptpd.spec.old	2013-10-22 21:17:46.000000000 +0800
+++ pptpd.spec	2013-10-25 17:53:13.000000000 +0800
@@ -13,8 +13,8 @@
 
 Summary:	PoPToP Point to Point Tunneling Server
 Name:		pptpd
-Version:	1.3.4
-Release:	4%{?dist}
+Version:	1.4.0
+Release:	1%{?dist}

    => Ack.

 License:	GPLv2+
 Group:		Applications/Internet
 BuildRequires:	ppp-devel, systemd
@@ -22,12 +22,9 @@
 Source0:	http://downloads.sf.net/poptop/pptpd-%{version}.tar.gz
 Source1:	pptpd.service
 Source2:	pptpd.sysconfig
-# Unbundle pppd.h
-Patch0:		pptpd-1.3.4-pppd-unbundle.patch

    => Very cool that you managed to get that upstreamed, one less downstream patch in Fedora. :)

 %global pppver %((%{__awk} '/^#define VERSION/ { print $NF }' /usr/include/pppd/patchlevel.h 2>/dev/null||echo none)|/usr/bin/tr -d '"')
 Requires:	ppp = %{pppver}
-Requires:	perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

    => Once again, apologies about that. :x

-BuildRequires:	ppp-devel, systemd
+Requires:	perl

    => Ack.

 %if %{?_without_libwrap:0}%{!?_without_libwrap:1}
 BuildRequires:	tcp_wrappers-devel
@@ -55,13 +52,6 @@
 %prep
 %setup -q
 
-# Unbundle pppd.h
-rm -f plugins/pppd.h
-%patch0 -p1 -b .pppd-unbundle
-
-# Fix permissions
-chmod 644 *.[ch] samples/pptpd.conf AUTHORS
-

    => Ack.

 # Fix for distros with %%{_libdir} = /usr/lib64
 perl -pi -e 's,/usr/lib/pptpd,%{_libdir}/pptpd,;' pptpctrl.c
 
@@ -76,7 +66,7 @@
 
 %install
 mkdir -p %{buildroot}%{_sysconfdir}/rc.d/init.d
-mkdir -p %{buildroot}%{_sysconfdir}//ppp
+mkdir -p %{buildroot}%{_sysconfdir}/ppp

    => Ack.

 mkdir -p %{buildroot}%{_bindir}
 mkdir -p %{buildroot}%{_mandir}/man{5,8}
 mkdir -p %{buildroot}%{_unitdir}
@@ -131,8 +121,7 @@
 %{_bindir}/vpnuser
 %{_bindir}/vpnstats.pl
 %{_mandir}/man5/pptpd.conf.5*
-%{_mandir}/man8/pptpd.8*
-%{_mandir}/man8/pptpctrl.8*
+%{_mandir}/man8/*.8*

    => I'd personally prefer the slightly stricter "%{_mandir}/man8/pptp*.8*", but that's purely my preference, not something you need to fix for the review.

 %{_unitdir}/pptpd.service
 %config(noreplace) %{_sysconfdir}/sysconfig/pptpd
 %config(noreplace) %{_sysconfdir}/pptpd.conf
@@ -142,6 +131,10 @@
 %attr(0755,root,root) %{_sysconfdir}/rc.d/init.d/pptpd
 
 %changelog
+* Fri Oct 25 2013 Jaroslav Škarvada <jskarvad> - 1.4.0-1
+- New version
+- Dropped pppd-unbundle patch (upstreamed)
+
 * Tue Oct 22 2013 Jaroslav Škarvada <jskarvad> - 1.3.4-4
 - Various fixes according to Fedora review
   Related: rhbz#632853

----------

As far as I'm concerned, the only remaining question is the licensing, other than that the package is good to go.

Comment 35 Mathieu Bridon 2013-11-08 10:26:39 UTC
Copy-pasting my licensing doubts in this comment, to make it easier for the
legal folks to review.

Note that the ppp header has been unbundled.

[!]: License field in the package spec file matches the actual license.

    The following files are GPLv2+:

    - bcrelay.c
    - plugins/pptpd-logwtmp.c
    - tools/vpnstats.pl

    The following files are under the LGPLv2+ license. (they are copied from
    the glibc)

    - getopt1.c
    - getopt.c
    - our_getopt.h

    The following files are under a BSD that I've never seen before, and isn't
    even in the wiki page:

    - plugins/pppd.h

        => This is copy-pasted from the ppp project, which is in Fedora. So
           you will have to either unbundle or ask FESCo for an exception.
           Note that there are differences with the file from the ppp-devel
           package.

        => Irrelevant from whether you can keep this bundled, I'm a bit
           worried about this BSD-like license. It's not in the Fedora wiki,
           so at the very least I'd like to ask Fedora Legal to take note of
           it. Also, it is similar to this one, which is said to be
           incompatible with the GPL:
            https://fedoraproject.org/wiki/Licensing:BSD#Advertising_Variant

    The following files are said to be copied from RFC 1662:

    - ppphdlc.c
    - ppphdlc.h

        => There are no license for these files, neither in pptpd nor in the
           text of the RFC (at least I couldn't find any). I'm not sure what
           that means, so I'd like to have Fedora Legal's advice on this.

    As for the rest of the files, they just don't have any copyright header.

    However, the man pages for pptpd(8) and pptpctrl(8) both state that pptpd
    is GPLv2+, so at the very least we can assume that the intent of the
    author is for the source files which get built as /usr/sbin/pptpd and
    /usr/sbin/pptpctrl to be GPLv2+. Looking at how the stuff is built, that
    would be:

    - compat.c
    - compat.h
    - configfile.c
    - configfile.h
    - ctrlpacket.c
    - ctrlpacket.h
    - defaults.h
    - inststr.c
    - inststr.h
    - our_syslog.h
    - pptpctrl.c
    - pptpctrl.c
    - pptpctrl.h
    - pptpd.c
    - pptpdefs.h
    - pptpgre.h
    - pptpmanager.c
    - pptpmanager.h
    - pqueue.c
    - pqueue.h

    Remaining are 3 files:

    - tools/pptp-portslave
    - tools/vpnstats
    - tools/vpnuser

        => Given that they come from the same authors, I'd be happy to assume
           that they are intended to be GPLv2+ like the rest of pptpd.

   => So, given all that, I'm honestly not sure what the right License tag
      should be for this package, or even if it's really acceptable in Fedora,
      so I'm blocking FE-LEGAL to ask for guidance.

Comment 36 Rex Dieter 2013-11-08 18:27:27 UTC
about lacking headers, see:

https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F

In short,

1.  What does the code say?
...
3.  What does the documentation say? 
...
If you get (here), you definitely want to let upstream know that you are unable to determine the applicable licensing (and/or license versioning) from the source and documentation. They'll almost certainly let you know what their intended license version is, and (hopefully) correct it in the upstream source.

Comment 37 Tom "spot" Callaway 2013-11-08 19:16:30 UTC
(In reply to Mathieu Bridon from comment #35)
> Copy-pasting my licensing doubts in this comment, to make it easier for the
> legal folks to review.
> 
> Note that the ppp header has been unbundled.
> 
> [!]: License field in the package spec file matches the actual license.
> 
>     The following files are GPLv2+:
> 
>     - bcrelay.c
>     - plugins/pptpd-logwtmp.c
>     - tools/vpnstats.pl
> 
>     The following files are under the LGPLv2+ license. (they are copied from
>     the glibc)
> 
>     - getopt1.c
>     - getopt.c
>     - our_getopt.h
> 
>     The following files are under a BSD that I've never seen before, and
> isn't
>     even in the wiki page:
> 
>     - plugins/pppd.h

I don't actually see this file in the pptpd package, but I looked at the one in the ppp-devel package and it is a variant of the "BSD with Attribution" license, so just add that to the License tag if it is actually in the pptpd package.

>     The following files are said to be copied from RFC 1662:
> 
>     - ppphdlc.c
>     - ppphdlc.h

These are clearly derived from the pptpclient source by C. S. Ananian, which is GPLv2+, so these files can be considered to be under that license as well.
 
Assuming that the pppd.h file isn't in pptpd, the license tag should be:

GPLv2+ and LGPLv2+

Lifting FE-Legal.

Comment 38 Mathieu Bridon 2013-11-09 04:28:56 UTC
Thanks Rex, I had missed this part of the guidelines. :)

Thanks for the review, Tom. The pppd.h file is indeed not in the pptpd package any more, as Jaroslav unbundled it (and that made it to the latest upstream release).

Jaroslav, can you fix the license tag as Tom indicated?

Comment 40 Mathieu Bridon 2013-11-11 08:50:23 UTC
Thanks Jaroslav, package is approved.

Comment 41 Jaroslav Škarvada 2013-11-11 08:58:50 UTC
New Package SCM Request
=======================
Package Name: pptpd
Short Description: PoPToP Point to Point Tunneling Server
Owners: jskarvad
Branches: f19 f20
InitialCC:

Comment 42 Gwyn Ciesla 2013-11-11 12:56:55 UTC
Git done (by process-git-requests).

Comment 43 Jaroslav Škarvada 2013-11-11 13:09:23 UTC
Thanks.

Comment 44 Fedora Update System 2013-11-11 13:37:37 UTC
pptpd-1.4.0-2.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/pptpd-1.4.0-2.fc20

Comment 45 Fedora Update System 2013-11-11 13:40:56 UTC
pptpd-1.4.0-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/pptpd-1.4.0-2.fc19

Comment 46 Fedora Update System 2013-11-12 00:27:11 UTC
pptpd-1.4.0-2.fc20 has been pushed to the Fedora 20 testing repository.

Comment 47 Fedora Update System 2013-11-21 04:38:07 UTC
pptpd-1.4.0-2.fc19 has been pushed to the Fedora 19 stable repository.

Comment 48 Jaroslav Škarvada 2014-03-26 11:01:39 UTC
Package Change Request
======================
Package Name: pptpd
New Branches: epel6 epel7
Owners: jskarvad

Comment 49 Gwyn Ciesla 2014-03-26 11:58:47 UTC
Git done (by process-git-requests).

Comment 50 Jaroslav Škarvada 2014-03-26 12:30:27 UTC
(In reply to Jon Ciesla from comment #49)
> Git done (by process-git-requests).

Thanks.

Comment 51 Fedora Update System 2014-03-27 04:47:01 UTC
pptpd-1.4.0-2.fc20 has been pushed to the Fedora 20 stable repository.


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