https://bugzilla.redhat.com/show_bug.cgi?id=1590425 Christophe de Dinechin <dinechin@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(dinechin@redhat.c | |om) | --- Comment #11 from Christophe de Dinechin <dinechin@xxxxxxxxxx> --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
