Spec URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp.spec SRPM URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-0.fc18.src.rpm Description: The libseccomp library provides an easy to use interface to the Linux Kernel's syscall filtering mechanism, seccomp. The libseccomp API allows an application to specify which syscalls, and optionally which syscall arguments, the application is allowed to execute, all of which are enforced by the Linux Kernel. Fedora Account System Username: pcmoore
This package is part of the QEMU syscall filtering feature which is currently proposed for Fedora 18: https://fedoraproject.org/wiki/Features/Syscall_Filters
The rpmlint is shown below; the spelling errors are not actual spelling errors (seccomp, syscall) and the URL (http://libseccomp.sourceforge.net) does exist. libseccomp.x86_64: W: spelling-error Summary(en_US) seccomp -> sec comp, sec-comp, compose libseccomp.x86_64: W: spelling-error %description -l en_US syscall -> scallop libseccomp.x86_64: W: spelling-error %description -l en_US seccomp -> sec comp, sec-comp, compose libseccomp.x86_64: W: spelling-error %description -l en_US syscalls -> miscalls libseccomp.x86_64: W: invalid-url URL: http://libseccomp.sourceforge.net HTTP Error 403: Forbidden libseccomp-debuginfo.x86_64: W: invalid-url URL: http://libseccomp.sourceforge.net HTTP Error 403: Forbidden libseccomp-devel.x86_64: W: spelling-error %description -l en_US syscall -> scallop libseccomp-devel.x86_64: W: spelling-error %description -l en_US seccomp -> sec comp, sec-comp, compose libseccomp-devel.x86_64: W: spelling-error %description -l en_US syscalls -> miscalls 3 packages and 0 specfiles checked; 0 errors, 9 warnings.
Koji scratch build for Rawhide/F18: http://koji.fedoraproject.org/koji/taskinfo?taskID=4152796
This isn't a review, just some comments. I find spec files much easier to read if the fields are lined up, like this example: http://oirase.annexia.org/reviews/whenjobs/whenjobs.spec Modern spec files *don't* need: * BuildRoot * %clean * rm -rf buildroot * defattr All of the above can be removed. Is the license LGPLv2 or LGPLv2+? Instead of ./configure + options, use %configure macro. Instead of CFLAGS=..., use 'make %{_smp_mflags}'
Building should be verbose: Non-verbose build logs like this are not-helpful when building in batch-jobs (such as in mock): ... INFO: building in directory src/ ... CC api.o CC db.o CC arch.o CC arch-i386.o ...
(In reply to comment #4) > This isn't a review, just some comments. Regardless, thanks for the feedback. > I find spec files much easier to read if the fields > are lined up, like this example: > > http://oirase.annexia.org/reviews/whenjobs/whenjobs.spec Noted for future requests, as this is a style issue I'll leave it as-is right now pending the favorite style of the reviewer. > Modern spec files *don't* need: > > * BuildRoot > * %clean > * rm -rf buildroot > * defattr > > All of the above can be removed. Done. > Is the license LGPLv2 or LGPLv2+? LGPLv2. LGPLv2.1 to be specific, but from what I read on the Fedora wiki regarding packaging the LGPLv2 tag applies to both LGPLv2 and LGPLv2.1. > Instead of ./configure + options, use %configure macro. While the libseccomp configure script mimics a few of the autoconf options, it only supports a select few. I think we are better off specifying those few options by hand then risking a change in the %configure macro causing the configure/build to fail for this package. However, if I'm looking at it the wrong way please let me know. > Instead of CFLAGS=..., use 'make %{_smp_mflags}' Noted, but unfortunately the package doesn't build correctly with simultaneous make jobs, e.g. "-j<N>" with "<N>" greater than one. We'll work on this upstream but until we have it sorted we can't use "%{_smp_mflags}"; I tried previously and the results were not good.
(In reply to comment #5) > Building should be verbose: Is this a requirement for acceptance? I ask because to make this change with the current release would either require changes upstream or a patch carried with the RPM.
I've updated the specfile and SRPM to incorporate the feedback from Richard W.M. Jones and I've submitted a new scratch build; links to all are below: Spec URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp.spec SRPM URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-0.fc18.src.rpm Scratch Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4155147
(In reply to comment #6) > LGPLv2. LGPLv2.1 to be specific, but from what I read on the Fedora wiki > regarding packaging the LGPLv2 tag applies to both LGPLv2 and LGPLv2.1. What I mean is, is it LGPLv2(.1) "only", or "any later version"? (In reply to comment #7) > > Building should be verbose: > > Is this a requirement for acceptance? I ask because to make this change > with the current release would either require changes upstream or a patch > carried with the RPM. Whether or not it's a requirement, Ralf is right that it makes debugging a whole lot easier. When all you have to go on is a Koji logfile, you'll be grateful that you enabled as much verbosity as possible.
(In reply to comment #9) > What I mean is, is it LGPLv2(.1) "only", or "any later version"? LGPLv2.1 "only". > (In reply to comment #7) > > > Building should be verbose: > > > > Is this a requirement for acceptance? I ask because to make this change > > with the current release would either require changes upstream or a patch > > carried with the RPM. > > Whether or not it's a requirement, Ralf is right that it > makes debugging a whole lot easier. When all you have to go on > is a Koji logfile, you'll be grateful that you enabled as much > verbosity as possible. I've made it note of it for future upstream development efforts, but I just wanted to know if it was a requirement which would block the acceptance of the package.
(In reply to comment #9) > (In reply to comment #7) > > > Building should be verbose: > > > > Is this a requirement for acceptance? Off-head, I don't know if it's formally mandated. However without it, it's effectively impossible to review a package without major efforts, because the logs don't tell whether a package receives all CFLAGS/CPPFLAGS etc. correctly. > > I ask because to make this change > > with the current release would either require changes upstream or a patch > > carried with the RPM. Are you upstream? Openly said, I'd recommend upstream to switch to a more standardized build/make framework (be it autotools, cmake or what ever), because hand-written makefiles/makefile-fragments, like the ones being using by this package tend to become non-understood and unmaintainable in longer terms.
(In reply to comment #11) > Off-head, I don't know if it's formally mandated. However without it, it's > effectively impossible to review a package without major efforts, because > the logs don't tell whether a package receives all CFLAGS/CPPFLAGS etc. > correctly. As mentioned in comment 10, we'll work on a verbose build for a future upstream release but it would be very helpful to get this package added to Rawhide now (as long as it meets all of the established requirements) so that we can make progress in other areas which depend on this package simultaneously. > Are you upstream? I've contributed the bulk of the code and cut the initial release. > Openly said, I'd recommend upstream to switch to a more standardized > build/make framework (be it autotools, cmake or what ever), because > hand-written makefiles/makefile-fragments, like the ones being using by this > package tend to become non-understood and unmaintainable in longer terms. I understand the concern but I am not currently a fan of the build frameworks that I've seen thus far and I'm unlikely to convert to them in the near term unless their use is a requirement for acceptance into Fedora. Looking towards the future, I'll need to spend more time understanding the build frameworks before I can give a definite "yes" or "no" regarding their inclusion in libseccomp.
Although autotools sucks greatly, it's a known quantity amongst developers and for that reason generally better than hand-hacked Makefiles. Here is a minimal autotools setup you can take inspiration from: http://git.annexia.org/?p=virt-hostinfo.git;a=tree;h=858cd733f328e2ffc961a28bbd4c362061bd2e6f;hb=56a3b79fbdcb80ef68f0311ca1d6af8b0789bff1
Thanks for the example, I'll reconsider autotools for a future release.
I've updated the libseccomp package for version 0.1.0 to include a patch which enables a verbose build as requested. Links to the patch, specfile, SRPM and scratch build as below: Spec URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp.spec Patch URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-build_verbose.patch SRPM URL: http://people.redhat.com/~pmoore/review/libseccomp/libseccomp-0.1.0-0.fc18.src.rpm Scratch Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4159695 Scratch Build Log (x86_64): http://koji.fedoraproject.org/koji/getfile?taskID=4159696&name=build.log
Just as a point of reference, libseccomp packages are now included in Debian/Ubuntu and Gentoo. Debian/Ubuntu: http://packages.debian.org/sid/libseccomp0 Gentoo: http://packages.gentoo.org/package/sys-libs/libseccomp?arches=prefix
I'll take the case!
Firstly, I agree with recommendations up thread to convert to autotools, the 'known quantity' aspect is hugely beneficial if other packagers ever need to step in a make build or specfile tweaks. But not a blocker here. fedora-review reported issues: Issues: [!]: MUST Package requires pkgconfig, if .pc files are present. (EPEL5) Note: Only applicable for EL-5 Not relevant since this requires a sufficiently new kernel that RHEL5 will never have. See: http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#EL5 [!]: MUST Rpmlint output is silent. rpmlint libseccomp-0.1.0-0.fc18.src.rpm libseccomp.src: W: spelling-error Summary(en_US) seccomp -> sec comp, sec-comp, compose libseccomp.src: W: spelling-error %description -l en_US syscall -> scallop libseccomp.src: W: spelling-error %description -l en_US seccomp -> sec comp, sec-comp, compose libseccomp.src: W: spelling-error %description -l en_US syscalls -> miscalls Not relevant (these errors were also repeated for each sub package, output snipped). libseccomp.src: W: invalid-url URL: http://libseccomp.sourceforge.net HTTP Error 403: Forbidden 1 packages and 0 specfiles checked; 0 errors, 5 warnings. Certainly works for me, so not sure what rpmlint is hitting here. Manual inspection of the spec and upstream sources didn't spot any other problems. Looks good to me!
When the package is imported, please list me as a co-maintainer, and the virtmaint as watchbugzilla + watchcommits. Also, Tomas Mraz agreed offline to sponsor Paul.
Paul, you're sponsored now.
Great, thank you to everyone for your comments/help/sponsorship!
New Package SCM Request ======================= Package Name: libseccomp Short Description: Enhanced seccomp library Owners: pcmoore Branches: f18 InitialCC:
Git done (by process-git-requests). Don't request f18 yet, ==devel.
Great, thank you. Cole, unfortunately I made a mistake and didn't list you as a co-maintainer (comment #20). If you know how to fix this let me know. Same thing applies to virtmaint and watchbugzilla + watchcommits.
According to the Fedora Wiki instructions I'm now closing out this BZ as "NEXTRELEASE" based on a successful Rawhide build. * Rawhide Build http://koji.fedoraproject.org/koji/buildinfo?buildID=327750