Fedora Merge Review: cups http://cvs.fedora.redhat.com/viewcvs/devel/cups/ Initial Owner: twaugh
Review of cups-1.4.2-17.fc13: Legend: "+" means OK, "-" means not OK. + MUST: The spec file name must match the base package %{name}, in the format %{name}.spec - MUST(1): The package must meet the Packaging Guidelines + MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . + MUST: The License field in spec match the actual license + MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file must be included in %doc + MUST: The spec file written in American English - MUST(2): The spec file for the package is legible + MUST: The sources used to build the package must match the upstream source, as provided in the spec URL MUST: The package successfully compile + MUST: All build dependencies must be listed in BuildRequires - MUST(3): The spec file handle locales properly + MUST: Every package which stores shared library files in any of the dynamic linker's default paths, must call ldconfig in %post and %postun - MUST(4): Packages does not bundle copies of system libraries + MUST: Package own all directories that it creates + MUST: Package does not list a file more than once in the spec file - MUST(5): Permissions on files must be set properly. Every %files section must include a %defattr(...) line + MUST: Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT) + MUST: Package use macros consistently + MUST: Package contains code, or permissable content + MUST: Large documentation files must go in a -doc subpackage + MUST: If a package includes something as %doc, it must not affect the runtime of the application + MUST: Header files in a -devel package + MUST: Static libraries in a -static package + MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' + MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package + MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} + MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built + MUST: Packages containing GUI applications must include a %{name}.desktop file + MUST: Packages must not own files or directories already owned by other packages + MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) + MUST: All filenames in rpm packages must be valid UTF-8. 2: - don't define initdir macro, use existing %{_initddir} macro - don't use /usr/lib/cups for binaries. Directory /usr/libexec/cups should be used instead. - don't use obsolete Prereq and BuildReq. Use modern Requires and BuildRequires instead. - remove unneeded versioned BuildRequires (gcc, libselinux, audit-libs, dbus) - drop -fstack-protector-all gcc flag. -fstack-protector is sufficient, I think. Consider to build cupsd (and other programs exposed on the network) as PIEs. - append %{?_smp_mflags} to "make" command - don't use hardcoded paths like /usr/bin, /usr/share, /etc. Use appropriate rpm macro (%{_bindir}, %{_datadir}, %{_sysconfdir}) instead. 3: - use %find_lang to package locale files. Check https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files for more info. 4: - fire away Source4 and use system wide pstopdf, please. 5: - vast majority of binaries have incorrect perms. Please use 755 permissions for all shared libraries and binaries. There are so many rpmlint errors. Please fix them or explain why are they invalid.
(In reply to comment #1) > - don't use /usr/lib/cups for binaries. Directory /usr/libexec/cups should be > used instead. I asked upstream whether they would change the default to be /usr/libexec, and the answer was that it was not sufficiently standard for them. As there are plenty of 3rd party CUPS packages, and they need to know where to put their binaries, we deliberately stick to the upstream path for this to avoid incompatibilities. > - remove unneeded versioned BuildRequires (gcc, libselinux, audit-libs, dbus) > - drop -fstack-protector-all gcc flag. -fstack-protector is sufficient, I > think. Consider to build cupsd (and other programs exposed on the network) > as PIEs. So, firstly, cupsd is already built as a position-independent executable. That patch was integrated upstream, and we use the --enable-pie option when calling configure. As for the -fstack-protector-all flag: are you sure -fstack-protector is sufficient? It seems to offer less security. The versioned gcc requirement is for -fstack-protector-all. > 3: > - use %find_lang to package locale files. Check > https://fedoraproject.org/wiki/Packaging:Guidelines#Handling_Locale_Files > for more info. Unfortunately CUPS doesn't use gettext and instead has its own mechanism for installing language files. It is not compatible with %find_lang. > 4: > - fire away Source4 and use system wide pstopdf, please. There is no system wide pstopdf. Note that this is not a generic program but a CUPS filter and must operate in a particular way (see man 7 filter). Working on the other fixes, thanks for pointing them out.
(In reply to comment #2) > I asked upstream whether they would change the default to be /usr/libexec, and > the answer was that it was not sufficiently standard for them. As there are > plenty of 3rd party CUPS packages, and they need to know where to put their > binaries, we deliberately stick to the upstream path for this to avoid > incompatibilities. OK, I'm not going to block review due this. > > As for the -fstack-protector-all flag: are you sure -fstack-protector is > sufficient? It seems to offer less security. The versioned gcc requirement is > for -fstack-protector-all. Well, -fstack-protector is less secure than -fstack-protector-all but -all causes bigger overhead. It is choice between security and performace. No stack protector means the best performace but the worst security and oppositely. Vast majority of network daemons in Fedora are compiled with -fstack-protector but if you prefer stack-protector-all, use it.
Please take another look at cups-1.4.2-24.fc13. I've added lots of fixes for the issues raised. As for rpmlint errors: cups.x86_64: E: file-in-usr-marked-as-conffile /usr/share/cups/templates/es/class-added.tmpl [...] All of these are for bug #441719. cups.x86_64: E: non-readable /usr/lib/cups/backend/lpd 0700 cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/lpd 0700 cups.x86_64: E: non-readable /usr/lib/cups/backend/serial 0700 cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/serial 0700 cups.x86_64: E: non-readable /usr/lib/cups/backend/dnssd 0700 cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/dnssd 0700 cups.x86_64: E: non-readable /usr/lib/cups/backend/ipp 0700 cups.x86_64: E: non-standard-executable-perm /usr/lib/cups/backend/ipp 0700 All these backends must be run as root. Backends with execute permission only for the owner need to be run as root. See the PERMISSIONS section of 'man 7 backend'. cups.x86_64: E: non-readable /etc/cups/classes.conf 0600 cups.x86_64: E: zero-length /etc/cups/classes.conf cups.x86_64: E: non-readable /etc/cups/printers.conf 0600 cups.x86_64: E: zero-length /etc/cups/printers.conf cups.x86_64: E: non-readable /etc/cups/cupsd.conf 0640 These files may contain security-sensitive information. cups.x86_64: E: non-readable /etc/cups/cupsd.conf.default 0640 cups.x86_64: W: non-conffile-in-etc /etc/cups/cupsd.conf.default To have the same permissions as the cupsd.conf file it has default content for. Also, this is *the* default content and is not intended to be configured. (Should it live elsewhere?) cups.x86_64: E: zero-length /etc/cups/lpoptions cups.x86_64: E: zero-length /etc/cups/subscriptions.conf cups.x86_64: E: zero-length /etc/cups/client.conf Making sure that these files exist in the manifest to ensure they are created with the correct SELinux file contexts. cups.x86_64: E: non-standard-dir-perm /var/run/cups/certs 0511 cups.x86_64: E: non-standard-dir-perm /etc/cups/ssl 0700 cups.x86_64: E: non-standard-dir-perm /var/spool/cups 0710 cups.x86_64: E: non-standard-dir-perm /var/spool/cups/tmp 01770 These are all correct and reflect their intended use. cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/label.h cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/escp.h cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/pcl.h cups.x86_64: W: devel-file-in-non-devel-package /usr/share/cups/ppdc/hp.h These are not devel files in fact and are required at run-time by CUPS DDK drivers (e.g. /usr/share/cups/drv/sample.drv). cups.x86_64: E: non-readable /usr/sbin/cupsd 0500 cups.x86_64: E: non-standard-executable-perm /usr/sbin/cupsd 0500 See http://cups.org/str.php?L3459 and bug #546004.
rpmlint output is so long so it will be confusing to paste it here. There are many false positives, as written in comment #4, because cups is very specific package. Following rpmlint issues should be addressed: cups.src:259: W: macro-in-comment %patch28 cups.src: W: patch-not-applied Patch28: cups-gnutls-gcrypt-threads.patch ^^^ please apply the patch or remove it cups.x86_64: W: obsolete-not-provided LPRng ^^^ I think LPRng should be provided cups.x86_64: W: file-not-utf8 /usr/share/doc/cups-1.4.3/CREDITS.txt ^^^ Please convert this file to UTF-8 during build process. `man 1 iconv` is your friend cups-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libcupscgi.so.1 exit.5 ^^^ Please explain why it is acceptable (for example self-implemented stack protector is a good reason to call exit inside library) cups-lpd.x86_64: W: spelling-error %description -l en_US provices cups-php.x86_64: W: spelling-error %description -l en_US provices ^^^ Probably typos Remaining issues from comment #1: The PIE problem: - per buildlog --enable-pie does nothing: ./configure ... ... configure: WARNING: unrecognized options: ..., --enable-pie, ... ... - it seems you use -pie/-fpie flags wrongly. You use only -fPIE during linking which is wrong. You should use -fPIE during compilation and -pie during linking. `man 1 gcc` clearly says: " -pie Produce a position independent executable on targets which support it. For predictable results, you must also specify the same set of options that were used to generate code (-fpie, -fPIE, or model suboptions) when you specify this option. " - to be precise please use -fpie during compilation on all platforms except sparcv9 sparc64 s390 and s390x. In "bind" specfile there are following lines: # Sparc and s390 arches need to use -fPIE %ifarch sparcv9 sparc64 s390 s390x for i in bin/named{,-sdb}/{,unix}/Makefile.in; do sed -i 's|fpie|fPIE|g' $i done %endif I don't see other problems.
(In reply to comment #5) > cups.src:259: W: macro-in-comment %patch28 > cups.src: W: patch-not-applied Patch28: cups-gnutls-gcrypt-threads.patch > ^^^ please apply the patch or remove it Gone. > cups.x86_64: W: obsolete-not-provided LPRng > ^^^ I think LPRng should be provided The history behind this is in bug #148757. > cups.x86_64: W: file-not-utf8 /usr/share/doc/cups-1.4.3/CREDITS.txt > ^^^ Please convert this file to UTF-8 during build process. `man 1 iconv` is > your friend I believe it is now UTF-8 upstream. > cups-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libcupscgi.so.1 > exit.5 > ^^^ Please explain why it is acceptable (for example self-implemented stack > protector > is a good reason to call exit inside library) This shared library is only used internally by CUPS (it is used by several of the CGI handlers). It is simply to share code between CUPS programs. > cups-lpd.x86_64: W: spelling-error %description -l en_US provices > cups-php.x86_64: W: spelling-error %description -l en_US provices > ^^^ Probably typos Both fixed. > The PIE problem: I've removed --enable-pie from the configure options as this is (sort of) done automatically upstream. The upstream build process is not sophisticated enough to handle passing exactly the right parameters. It uses -fPIC (not -fPIE) when compiling, which Jakub assures me is simply slightly less efficient that -fPIE in some cases. Indeed, the resulting executables behave as PIEs and seem to have the correct ELF flags set. The upstream discussion about it is here: http://cups.org/str.php?L3691 Please take a look at cups-1.4.6-6.fc14.
Ok, I checked cups-1.4.6-13.fc16 and it seems fine for me. You can close the review now.
Thanks!