Red Hat Bugzilla – Bug 632853
Review Request: pptpd - PoPToP Point to Point Tunneling Server
Last modified: 2014-03-27 00:47:01 EDT
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.
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
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.
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?
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.
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/
> Dave, are you still interested / responsive regarding this review? The answer seems to be a clear "no". :-)
Well, in case Dave is no longer interested in this package, I can take it and maintain, but we need to find another reviewer :)
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@city-fan.org> - 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@city-fan.org> - 1.3.4-1.1 - Rebuild against ppp 2.4.4 * Fri Apr 20 2007 Paul Howarth <paul@city-fan.org> - 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@city-fan.org> - 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@city-fan.org> - 1.3.3-1 - Update to 1.3.3 - Add dist tag - Add %postun scriptlet dependency for /sbin/service - Fix doc permissions [etc]
(In reply to comment #8) Thanks Charlie, CCed Paul. I would like to move this forward :)
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...
(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@city-fan.org> - 1.3.3-1 - Update to 1.3.3 - Add dist tag - Add %%postun scriptlet dependency for /sbin/service ...
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!
(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@laptop.org He will probably be happy to accept patches.
(In reply to comment #13) > He will probably be happy to accept patches. But probably would prefer to receive patches via: poptop-server@lists.sourceforge.net
(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?
Hi Mathieu, please take the review (edit the "assigned to:" field to yourself). I will try to prepare the new srpm during this week.
Taken.
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 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 :)
(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.
(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.
(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.
(In reply to Paul Howarth from comment #22) > OK, done. Taken.
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
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
(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
(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.
> 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.
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@redhat.com> - 1.3.4-4 +- Various fixes according to Fedora review + Related: rhbz#632853 + * Fri Oct 18 2013 Jaroslav Škarvada <jskarvad@redhat.com> - 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.
Upstream has just released version 1.4.0 by the way.
(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).
In the service file, why not use "pptpd --fg" and then "Type=forking" wouldn't be needed?
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
(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@redhat.com> - 1.4.0-1 +- New version +- Dropped pppd-unbundle patch (upstreamed) + * Tue Oct 22 2013 Jaroslav Škarvada <jskarvad@redhat.com> - 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.
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.
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.
(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.
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?
SPEC: http://jskarvad.fedorapeople.org/pptpd/pptpd.spec SRPM: http://jskarvad.fedorapeople.org/pptpd/pptpd-1.4.0-2.fc19.src.rpm
Thanks Jaroslav, package is approved.
New Package SCM Request ======================= Package Name: pptpd Short Description: PoPToP Point to Point Tunneling Server Owners: jskarvad Branches: f19 f20 InitialCC:
Git done (by process-git-requests).
Thanks.
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
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
pptpd-1.4.0-2.fc20 has been pushed to the Fedora 20 testing repository.
pptpd-1.4.0-2.fc19 has been pushed to the Fedora 19 stable repository.
Package Change Request ====================== Package Name: pptpd New Branches: epel6 epel7 Owners: jskarvad
(In reply to Jon Ciesla from comment #49) > Git done (by process-git-requests). Thanks.
pptpd-1.4.0-2.fc20 has been pushed to the Fedora 20 stable repository.