Spec URL: https://pagure.io/kata-runtime/blob/master/f/kata-runtime.spec SRPM URL: TBD Description: TBD Fedora Account System Username: lsm5
The spec file seems to have been moved to https://pagure.io/kata-rpm-reviews/blob/master/f/kata-runtime/kata-runtime.spec, along with other rpms related to kata.
(In reply to Lokesh Mandvekar from comment #0) > Spec URL: https://pagure.io/kata-runtime/blob/master/f/kata-runtime.spec > SRPM URL: TBD > Description: TBD > Fedora Account System Username: lsm5 Still need a review?
(In reply to Robert-André Mauchin from comment #2) > (In reply to Lokesh Mandvekar from comment #0) > > Spec URL: https://pagure.io/kata-runtime/blob/master/f/kata-runtime.spec > > SRPM URL: TBD > > Description: TBD > > Fedora Account System Username: lsm5 > > Still need a review? I've just restarted the work. Spec URL: https://pagure.io/fork/ddd/kata-rpm-reviews/raw/master/f/kata-runtime/kata-runtime.spec SRPM URL: http://blackbox.dinechin.org/fedora/kata-runtime-1.7.3-5.src.rpm Description: Kata runtime to run containers in virtual machines Fedora Account System Username: ddd Copr build: https://copr.fedorainfracloud.org/coprs/ddd/kata-runtime/build/969024/
I guess Cole Robinson is also interested in this. cc-ing him as well..
(In reply to Christophe de Dinechin from comment #3) > (In reply to Robert-André Mauchin from comment #2) > > (In reply to Lokesh Mandvekar from comment #0) > > > Spec URL: https://pagure.io/kata-runtime/blob/master/f/kata-runtime.spec > > > SRPM URL: TBD > > > Description: TBD > > > Fedora Account System Username: lsm5 > > > > Still need a review? > > I've just restarted the work. > > Spec URL: > https://pagure.io/fork/ddd/kata-rpm-reviews/raw/master/f/kata-runtime/kata- > runtime.spec > SRPM URL: http://blackbox.dinechin.org/fedora/kata-runtime-1.7.3-5.src.rpm > Description: Kata runtime to run containers in virtual machines > Fedora Account System Username: ddd > Copr build: > https://copr.fedorainfracloud.org/coprs/ddd/kata-runtime/build/969024/ No need for %defattr(-,root,root,-) We don't use Group: in Fedora. Why are you not using goprep, gopkg, gometa? F31 is already there to use them. Try go2rpm in updates-testing to create a base SPEC.
(In reply to Robert-André Mauchin from comment #5) > No need for %defattr(-,root,root,-) Will remove > > We don't use Group: in Fedora. Will remove > > > Why are you not using goprep, gopkg, gometa? F31 is already there to use them. > Try go2rpm in updates-testing to create a base SPEC. For portability reasons. I started with existing spec files that did not use them, and decided I would use the go-rpm-macros only in the F31 branch. There are comments throughout the spec file referring to F31.
Spec file updated to 1.8.0 here: https://pagure.io/fork/ddd/kata-rpm-reviews/c/af6d31a0979561884afd90cb73b447efb474fd3a?branch=master. Review comments (except use of go-rpm-macros) addressed here: https://pagure.io/fork/ddd/kata-rpm-reviews/c/f896aa194ff9f5833f8e333000cd5e1e7d4cdabf?branch=master
- What are you doing in %check? There's no test being run here %check export http_proxy=http://127.0.0.1:9/ export https_proxy=http://127.0.0.1:9/ export no_proxy=localhost - PREFIX=/usr → PREFIX={_prefix} - Escape %gopkg, %goprep %gopkginstall otherwise it triggers an error: # F31: Use %%gopkg # Future: Use %%goprep for F31 # Future: Use %%gopkginstall - Also this doesn't seem to use the default Fedora build flags for Go gobuild(o:) %{expand: # https://bugzilla.redhat.com/show_bug.cgi?id=995136#c12 %global _dwz_low_mem_die_limit 0 %ifnarch ppc64 %{?gobuilddir:GOPATH="%{gobuilddir}:${GOPATH:+${GOPATH}:}%{?gopath}"} GO111MODULE=off \ go build -buildmode pie -compiler gc -tags="rpm_crashtraceback ${BUILDTAGS:-}" -ldflags "${LDFLAGS:-}%{?currentgoldflags} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '%__global_ldflags %{?__golang_extldflags}'" -a -v -x %{?**}; %else %{?gobuilddir:GOPATH="%{gobuilddir}:${GOPATH:+${GOPATH}:}%{?gopath}"} GO111MODULE=off \ go build -compiler gc -tags="rpm_crashtraceback ${BUILDTAGS:-}" -ldflags "${LDFLAGS:-}%{?currentgoldflags} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '%__global_ldflags %{?__golang_extldflags}'" -a -v -x %{?**}; %endif } From the Makefile script you should be able to pass the flags though the BUILDFLAGS var: # go build common flags BUILDFLAGS := -buildmode=pie %ifnarch ppc64 export BUILDFLAGS="-buildmode pie -compiler gc -tags=\"rpm_crashtraceback ${BUILDTAGS:-}\" -ldflags \"${LDFLAGS:-}%{?currentgoldflags} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '%__global_ldflags %{?__golang_extldflags}'\" -a -v -x" %else export BUILDFLAGS="-compiler gc -tags=\"rpm_crashtraceback ${BUILDTAGS:-}\" -ldflags \"${LDFLAGS:-}%{?currentgoldflags} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \n') -extldflags '%__global_ldflags %{?__golang_extldflags}'\" -a -v -x" %endif - Package is not installable: DEBUG util.py:585: BUILDSTDERR: Error: DEBUG util.py:585: BUILDSTDERR: Problem: conflicting requests DEBUG util.py:585: BUILDSTDERR: - nothing provides kata-containers-image = 1.8.0 needed by kata-runtime-1.8.0-1.fc31.x86_64 DEBUG util.py:585: BUILDSTDERR: - nothing provides kata-ksm-throttler = 1.8.0 needed by kata-runtime-1.8.0-1.fc31.x86_64 DEBUG util.py:585: BUILDSTDERR: - nothing provides kata-linux-container = 1.8.0 needed by kata-runtime-1.8.0-1.fc31.x86_64 DEBUG util.py:585: BUILDSTDERR: - nothing provides kata-proxy = 1.8.0 needed by kata-runtime-1.8.0-1.fc31.x86_64 DEBUG util.py:585: BUILDSTDERR: - nothing provides kata-shim = 1.8.0 needed by kata-runtime-1.8.0-1.fc31.x86_64 DEBUG util.py:587: (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages) - Completion file shouldn't have a shebang: kata-runtime.x86_64: E: non-executable-script /usr/share/bash-completion/completions/kata-runtime 644 /bin/bash - The whole package should be unbundled: that is, remove vendor and BR the necessary golang libraries Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Package installs properly. Note: Installation errors (see attachment) See: https://docs.fedoraproject.org/en-US/packaging-guidelines/ ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [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. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "*No copyright* Apache License (v2.0)", "Apache License (v2.0)", "Expat License", "BSD 3-clause "New" or "Revised" License", "ISC License", "BSD 2-clause "Simplified" License", "*No copyright* Mozilla Public License (v2.0)", "Creative Commons Attribution-ShareAlike Public License (v4.0)". 1321 files have unknown license. Detailed output of licensecheck in /home/bob/packaging/review/kata-runtime/review-kata- runtime/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/libexec/kata- containers(kata-osbuilder, kata-shim, kata-proxy) [!]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: 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). [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. [-]: 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. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [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 %license. [x]: Package requires other packages for directories it uses. [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]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [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]: Package does not use a name that already exists. [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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: Uses parallel make %{?_smp_mflags} macro. [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [-]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in kata- runtime [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [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]: 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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [!]: Rpmlint is run on all installed packages. Note: Mock build failed See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_use_rpmlint [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: kata-runtime-1.8.0-1.fc31.x86_64.rpm kata-runtime-debuginfo-1.8.0-1.fc31.x86_64.rpm kata-runtime-debugsource-1.8.0-1.fc31.x86_64.rpm kata-runtime-1.8.0-1.fc31.src.rpm kata-runtime.x86_64: W: no-documentation kata-runtime.x86_64: E: non-executable-script /usr/share/bash-completion/completions/kata-runtime 644 /bin/bash kata-runtime.x86_64: W: no-manual-page-for-binary containerd-shim-kata-v2 kata-runtime.x86_64: W: no-manual-page-for-binary kata-collect-data.sh kata-runtime.x86_64: W: no-manual-page-for-binary kata-runtime 4 packages and 0 specfiles checked; 1 errors, 4 warnings.
Updated spec file here: https://pagure.io/fork/ddd/kata-rpm-reviews/blob/go-rpm-macros/f/kata-runtime/kata-runtime.spec Copr build: https://copr.fedorainfracloud.org/coprs/ddd/kata/build/1050710/ This new spec file uses go rpm macros and no longer builds with f29 or f30.
Using the SRPM URL: and Spec URL: format in Comment #3 helps simplify the review process, fedora-review tool can just be pointed at the bug. So for next posting please provide that as well * The kata-runtime binary should be in /usr/bin, which is where upstream puts it. I don't see any reason to deviate * godocs/golicense/files: only a single LICENSE file ends up in the rpm. I don't think the godocs/golicense stuff is playing well with the %files section, but I'm not sure. either way, I don't think we need to put anything more than the top level LICENSE and README.md into the RPM. The LICENSEs are all the same apache version, and most of of the other docs seem fine to just exist in upstream git. * The BuildRequires: compiler(go-compiler) is not required, go-rpm-macros does that for us * The comment about a 'rediculous' name from goname is not necessary. IMO it's self evident why goname does not apply here, it doesn't need the commentary Trimmed fedora-review output below, with some comments. [ ]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/defaults * Yes, seems like this should be a %dir in the %files list. This is a non-standard FHS directory I believe, but it will take work with kata upstream to correct it, so it should stay for now IMO> [ ]: %build honors applicable compiler flags or justifies otherwise. * We talked about this offline. Use of %gobuild is not easy in this package because 'make' handles a bunch of other stuff. So distro go build flags aren't making it into the build. Next release of go-rpm-macros should have a %gobuildflags macro that will help here, but it isn't available yet. Christophe, please add a comment to that effect in the spec, so we don't forget, and optionally link to the go-rpm-macros commit: https://pagure.io/go-rpm-macros/c/67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2?branch=master [ ]: Package is not known to require an ExcludeArch tag. * ExcludeArch is necessary as dependent kata components do not build on 32 bit archs. This is a known issue that isn't kata-runtimes fault. Doesn't block this review [!]: Reviewer should test that the package builds in mock. * worked fine for me [!]: Uses parallel make %{?_smp_mflags} macro. * indeed, spec should use %make_build and %make_install macros in place of the direct 'make' calls. But if this causes build issues, just drop them and mention it in the next submission Rpmlint ------- Checking: kata-runtime-1.8.2-2.fc32.x86_64.rpm kata-runtime-debuginfo-1.8.2-2.fc32.x86_64.rpm kata-runtime-debugsource-1.8.2-2.fc32.x86_64.rpm kata-runtime-1.8.2-2.fc32.src.rpm kata-runtime.x86_64: W: no-manual-page-for-binary kata-collect-data.sh 4 packages and 0 specfiles checked; 0 errors, 1 warnings. * This issue can be safely ignored. I think we may not want to ship this script, or move it elsewhere, but we can figure it out after package import
(In reply to Cole Robinson from comment #10) > Using the SRPM URL: and Spec URL: format in Comment #3 helps simplify the > review process, fedora-review tool can just be pointed at the bug. So for > next posting please provide that as well OK, sorry. > > * The kata-runtime binary should be in /usr/bin, which is where upstream > puts it. I don't see any reason to deviate One arguable reason is that historical docker put its runtime there, specifically in /usr/libexec/docker/docker-runc-current. A second one is that I don't like polluting /usr/bin with commands that are not intended for end-users. But that's just me. A third reason is lack of manpages and silencing a warning from fedora-review. The first argument is quite weak, based on other container software: The moby-engine package has only docker and dockerd in /usr/bin, and then has: /usr/libexec/docker/docker-init /usr/libexec/docker/docker-proxy but I believe it relies on runc, which is /usr/bin/runc. Podman is looking for runc all over the place: # Paths to look for a valid OCI runtime (runc, runv, etc) [runtimes] runc = [ "/usr/bin/runc", "/usr/sbin/runc", "/usr/local/bin/runc", "/usr/local/sbin/runc", "/sbin/runc", "/bin/runc", "/usr/lib/cri-o-runc/sbin/runc" ] crun = [ "/usr/bin/crun", "/usr/local/bin/crun", ] So there does not seem to be any settled theory. I will do the change back to /usr/bin, but reluctantly :-) > > * godocs/golicense/files: only a single LICENSE file ends up in the rpm. I > don't think the godocs/golicense stuff is > playing well with the %files section, but I'm not sure. either way, I > don't think we need to put anything more > than the top level LICENSE and README.md into the RPM. The LICENSEs are > all the same apache version, and most of > of the other docs seem fine to just exist in upstream git. I pasted there the output of go2rpm here. I assume they have a rationale for this, and I don't want to break their approach unnecessarily. Who knows if they have some scripts scourging rpms for these macros, etc. In short, I don't understand the implications, but at least regarding license, I suspect this may be related to the Tech Talk given by David Fontana recently, regarding the problem with the historical license compliance model for Fedora and Red Hat when confronted with languages that have their own package manager. To make a long story short, the way I understand it is that historically, we complied by shipping the source, assuming that the license was in the source. For languages such as go or nodejs or Java, this may no longer be the case, i.e. you may actually pull some additional code (thus some extra licenses) in binary format without knowing it. I imagine that this whole license fishing done by go2rpm may be an attempt to address that problem. So for now, I would prefer to leave it as is, unless it triggers a warning. However, I did add a big fat comment explaining where this came from. We can remove it later if the go folks confirm it is useless. > > * The BuildRequires: compiler(go-compiler) is not required, go-rpm-macros > does that for us I think this is true for go-rpm-macros (part of %gometa) I don't think this is true for compiler(go-compiler), rpm -qR on src rpm does not show compiler(go-compiler) anymore if I remove it. > > * The comment about a 'rediculous' name from goname is not necessary. IMO > it's self evident why > goname does not apply here, it doesn't need the commentary Was not obvious to me ;-) Rephrased the comment to avoid offensive mention of "ridiculous" > > Trimmed fedora-review output below, with some comments. > > [ ]: Package must own all directories that it creates. > Note: Directories without known owners: /usr/share/defaults > > * Yes, seems like this should be a %dir in the %files list. This is > a non-standard FHS directory I believe, but it will take work with > kata upstream to correct it, so it should stay for now IMO> Moved it to /usr/share/kata-containers/defaults (just needed to pass a variable to the makefile) We already have a /usr/share/kata-containers for images. Which package(s) need a %dir for it? I added one, not sure it's correct. > > [ ]: %build honors applicable compiler flags or justifies otherwise. > > * We talked about this offline. Use of %gobuild is not easy in this > package because 'make' handles a bunch of other stuff. So distro > go build flags aren't making it into the build. Next release > of go-rpm-macros should have a %gobuildflags macro that will help > here, but it isn't available yet. Christophe, please add a comment > to that effect in the spec, so we don't forget, and optionally > link to the go-rpm-macros commit: > https://pagure.io/go-rpm-macros/c/ > 67b4fbbbfce0986ac46cd1329bf85a18ea7a43d2?branch=master I tried this macro (by locally patching the macro files), but I get errors using it. It expands to: 'BUILDFLAGS=%{gocompilerflags} -tags="rpm_crashtraceback " -ldflags "-X github.com/kata-containers/runtime/version=1.8.2 -X github.com/kata-containers/runtime/version.tag=1.8.2 -B 0x60831d87667855a13661dfaf1b99ee957b4de2a4 -extldflags '\''-Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld '\''" -a -v -x' The %{gocompilerflags} at the beginning causes problems. Gave up for now. > > > [ ]: Package is not known to require an ExcludeArch tag. > > * ExcludeArch is necessary as dependent kata components do not build on > 32 bit archs. This is a known issue that isn't kata-runtimes fault. > Doesn't block this review There is also an ExcludeArch that comes from %gometa. > > [!]: Reviewer should test that the package builds in mock. > > * worked fine for me > > > [!]: Uses parallel make %{?_smp_mflags} macro. > > * indeed, spec should use %make_build and %make_install macros in place of > the direct > 'make' calls. But if this causes build issues, just drop them and mention > it in > the next submission Done. > > > Rpmlint > ------- > Checking: kata-runtime-1.8.2-2.fc32.x86_64.rpm > kata-runtime-debuginfo-1.8.2-2.fc32.x86_64.rpm > kata-runtime-debugsource-1.8.2-2.fc32.x86_64.rpm > kata-runtime-1.8.2-2.fc32.src.rpm > kata-runtime.x86_64: W: no-manual-page-for-binary kata-collect-data.sh > 4 packages and 0 specfiles checked; 0 errors, 1 warnings. > > * This issue can be safely ignored. I think we may not want to ship this > script, or move it elsewhere, but we can figure it out after package import You'll have more warnings about missing man pages after reverting the binaries from /usr/libexec to /usr/bin.
Spec URL: https://pagure.io/fork/ddd/kata-rpm-reviews/raw/review-v2/f/kata-runtime/kata-runtime.spec SRPM URL: http://blackbox.dinechin.org/fedora/kata-runtime-1.8.2-3.fc32.src.rpm Description: Kata runtime to run containers in virtual machines Fedora Account System Username: ddd Copr build: https://copr.fedorainfracloud.org/coprs/ddd/kata/build/1052606/ Integrate updates from #c10 with some caveats descibed in #c11.
The license/doc stuff is not doing what you think it's doing. There's only a single LICENSE file that ends up in the RPM: $ rpm -qpl ./x86_64/kata-runtime-1.8.2-3.fc31.x86_64.rpm | grep LICENSE /usr/share/licenses/kata-runtime/LICENSE Which LICENSE of the 3 mentioned? Not sure, maybe the first, but I didn't confirm. And it does produce warnings: $ rpmbuild --rebuild ./srpm/kata-runtime-1.8.2-3.fc32.src.rpm ... warning: File listed twice: /usr/share/doc/kata-runtime/README.md warning: File listed twice: /usr/share/doc/kata-runtime/README.md warning: File listed twice: /usr/share/doc/kata-runtime/README.md warning: File listed twice: /usr/share/doc/kata-runtime/README.md warning: File listed twice: /usr/share/doc/kata-runtime/README.md warning: File listed twice: /usr/share/doc/kata-runtime/README.md warning: File listed twice: /usr/share/doc/kata-runtime/README.md warning: File listed twice: /usr/share/licenses/kata-runtime/LICENSE warning: File listed twice: /usr/share/licenses/kata-runtime/LICENSE ... For some reason the review is also spitting out an Issue that wasn't there before, but I can't see what changed: Issues: ======= - Package does not contain duplicates in %files. Note: warning: File listed twice: /usr/share/doc/kata-runtime/README.md See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_duplicate_files But it seems related. Otherwise this looks good to me. I'm approving the review but please find a solution to those issues before performing a first build. Next steps on your side are listed here: https://fedoraproject.org/wiki/Package_Review_Process#Contributor
Lokesh originally submitted this review request, but he handed off work to Christophe, which I guess complicates the initial package setup. So I'm closing this request. Christophe submitted a new review request for kata-runtime here: https://bugzilla.redhat.com/show_bug.cgi?id=1761451