[Bug 1720377] New: Review Request: crun - A fast and lightweight fully featured OCI runtime and C library for running containers
https://bugzilla.redhat.com/show_bug.cgi?id=1720377
Bug ID: 1720377 Summary: Review Request: crun - A fast and lightweight fully featured OCI runtime and C library for running containers Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: gscrivan@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://github.com/giuseppe/crun/blob/master/rpm/crun.spec.template
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/gscrivano/crun/srpm-builds/0...
Description: an OCI runtime for running containers written in C. It can be used as a replacement for runc. It works with Podman, CRI-O and Moby.
Fedora Account System Username: gscrivano
https://bugzilla.redhat.com/show_bug.cgi?id=1720377
Debarshi Ray debarshir@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |debarshir@redhat.com Assignee|nobody@fedoraproject.org |debarshir@redhat.com Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=1720377
--- Comment #1 from Debarshi Ray debarshir@redhat.com --- Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=35614001
MUST items ----------
rpmlint output:
$ rpmlint crun-0.6-1.fc29.src.rpm crun.src: E: no-changelogname-tag crun.src: W: invalid-url Source0: %{url}/archive/crun-0.6.tar.gz 1 packages and 0 specfiles checked; 1 errors, 1 warnings.
$ rpmlint crun-0.6-1.fc29.x86_64.rpm crun.x86_64: E: no-changelogname-tag crun.x86_64: E: missing-call-to-chdir-with-chroot /usr/bin/crun crun.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libcrun.a 1 packages and 0 specfiles checked; 2 errors, 1 warnings.
YES - package follows Naming Guidelines YES - spec file name matches base package %{name}
NO - package does not meet Packaging Guidelines
+ %changelog stanza is missing.
+ Just %make_install should be sufficient. It's unnecessary to set INSTALL to "install -p". $ rpm --eval "%make_install" /usr/bin/make install INSTALL="/usr/bin/install -p ..."
+ The autogen.sh script doesn't actually use a NOCONFIGURE variable.
YES - package is under a Fedora approved license and meets Licensing Guidelines
YES - License field meets actual license
+ libcrun.a uses src/libcrun/cloned_binary.c and src/libcrun/chroot_realpath.c which are under ASL 2.0 and LGPLv2+ respectively. Pedantically speaking, the License tag for the library would be "LGPLv3+ and LGPLv2+ and ASL 2.0". See:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licens...
+ libocispec/COPYING contains the GPLv3, but it seems that the only piece of GPLv3-ed code is libocispec/src/validate.c which isn't really the primary artifact.
NO - source package includes license text, which is included in %license
+ Should also include the LGPLv3+ text in COPYING.libcrun in %license, if the library is meant to be packaged.
YES - spec file written in American English YES - spec file is legible
NO - sources match upstream source
+ The URL field is wrong and leads to a 404 error: $ wget https://github.com/giuseppe/crun/archive/crun-0.6.tar.gz ...
It could be changed to:
https://github.com/giuseppe/crun/releases/download/%%7Bversion%7D/%%7Bname%7...
The SHA256 hash of the tarball in the SRPM doesn't match any of the tarballs listed on the GitHub releases page. It's likely that the auto-generated GitHub tarballs don't have a stable hash, in which case the manually uploaded tarballs are better.
YES - package compiles on all primary architectures YES - there is no need for ExcludeArch
YES - all build dependencies in BuildRequires
+ Duplicated BuildRequires for autoconf, automake and gcc.
YES - handles locales properly YES - no need for ldconfig
??? - doesn't bundle system libraries
+ Is glibc-static really needed? /usr/bin/crun seems to work without having it as a BuildRequire.
YES - package is not relocatable YES - package owns all directories that it creates YES - files are listed only once in %files YES - file permissions set properly YES - consistent use of macros YES - contains code and permissable content YES - no need for doc subpackage YES - no chance of items marked as %doc affecting runtime
NO - static libraries must be in a -static package
+ Should the /usr/lib64/libcrun.a static archive really be packaged? I don't see any HEADERS in the build scripts, which makes me think that it's only meant to be consumed by /usr/bin/crun itself.
+ If it's meant to be packaged, then should it really be a static library? If so, then there should be a crun-static sub-package.
See:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-...
NO - development files in devel subpackage
+ If the library is meant to be packaged, then the accompanying header files should be in a crun-devel sub-package.
??? - devel subpackage requires base package
+ There's no -devel subpackage currently, but it's not clear if there should be one.
NO - package removes all libtool archives
+ /usr/lib64/libcrun.la should be removed in the %install stanza with: find $RPM_BUILD_ROOT -name '*.la' -delete
See:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-...
YES - package doesn't need a .desktop file YES - doesn't own files or directories owned by other packages YES - all filenames are valid UTF-8 YES - package doesn't have deprecated dependencies
SHOULD items: -------------
YES - upstream provides license text NO - description and summary doesn't have translations YES - package builds in Koji YES - builds on all primary architectures YES - package functions as described YES - package doesn't use scriptlets
??? - subpackages should require base package using fully versioned dependency
+ It's not clear if sub-packages are needed.
YES - no pkgconfig files YES - no file dependencies outside of /etc/, /bin/, /sbin, etc. YES - contains man pages
https://bugzilla.redhat.com/show_bug.cgi?id=1720377
--- Comment #2 from Giuseppe Scrivano gscrivan@redhat.com --- thanks for the review. I've dropped the libraries from the package, as they are not really needed and addressed the other comments:
https://www.scrivano.org/static/crun-rpm/
https://bugzilla.redhat.com/show_bug.cgi?id=1720377
Debarshi Ray debarshir@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #3 from Debarshi Ray debarshir@redhat.com --- Great! Thanks for updating the package.
Maybe you could expand the %description a bit to match up against runc, etc..? Not a blocker, though.
There's this one rpmlint error, but it might be a false positive, and I will leave it to your judgement.
$ rpmlint -i /home/rishi/devel/rpmbuild/RPMS/x86_64/crun-0.6-1.fc29.x86_64.rpm crun.x86_64: E: missing-call-to-chdir-with-chroot /usr/bin/crun This executable appears to call chroot without using chdir to change the current directory. This is likely an error and permits an attacker to break out of the chroot by using fchdir. While that's not always a security issue, this has to be checked.
1 packages and 0 specfiles checked; 1 errors, 0 warnings.
Again, not a blocker, and you can suppress it in Taskotron tests run by Bodhi by adding a crun.rpmlintrc file: https://fedoraproject.org/wiki/Taskotron/Tasks/dist.rpmlint
ACCEPTED
https://bugzilla.redhat.com/show_bug.cgi?id=1720377
--- Comment #4 from Giuseppe Scrivano gscrivan@redhat.com --- thanks for the review, indeed the chroot seems to be a false negative as the chdir is performed later on. I'll take a look at it through and see if I can change the code to not trigger the warning
https://bugzilla.redhat.com/show_bug.cgi?id=1720377
Jens Petersen petersen@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution|--- |CURRENTRELEASE Last Closed| |2020-02-27 05:42:50
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com