diff mbox series

[bug#69131,2/2] gnu: podman: Update to 4.9.3.

Message ID 564fa27db3acf9ab25ebb8887023c34a0f41930c.1707938222.git.~@wolfsden.cz
State New
Headers show
Series [bug#69131,1/2] gnu: Add passt. | expand

Commit Message

Tomas Volf Feb. 14, 2024, 7:19 p.m. UTC
* gnu/packages/containers.scm (podman): Update to 4.9.3.

Change-Id: If764e8456a697d16b76cd4ba1243cc5f633a6049
---
 gnu/local.mk                                  |   1 +
 gnu/packages/containers.scm                   |  24 ++--
 ...earch-for-binaries-to-fit-Guix-model.patch | 120 ++++++++++++++++++
 3 files changed, 137 insertions(+), 8 deletions(-)
 create mode 100644 gnu/packages/patches/podman-Modify-search-for-binaries-to-fit-Guix-model.patch

Comments

Skyler Ferris Feb. 16, 2024, 3:01 a.m. UTC | #1
Hi Tomas,

Thanks for submitting this patch! I am adding some review notes to this 
issue in hopes of making the process more efficient for the committer. I 
noticed that `guix style` applies some minor changes to the definition 
of passt which should ideally be followed, but I don't expect that this 
would hold up the commit (and I'm not sure if it would be appropriate or 
intrusive to add a new patch to this thread with the style changes 
applied). Everything else looks good! I will add a couple more messages 
to this thread which contain full notes of items I reviewed, for 
transparency and clarity. Please let me know if there is anything I 
could do to improve my review process!

Regards,
Skyler
Skyler Ferris Feb. 16, 2024, 3:03 a.m. UTC | #2
"Update podman" review notes:

The following checklist is based on the items listed in the manual (22.7 
"Submitting Patches"). Each checklist item may have one of the following 
statuses:

C: Commitable. This item looks as expected for a final commit.
CC: Commitable with Context. Like commitable, but some notes have been 
added providing context about anything notable or unusual. These notes 
are provided for transparency or to set expectations for the committer 
and should not be interpreted as meaning that I object to the patch.
MR: Minor Repair. There is something about the patch which I think 
should ideally be changed, but I do not think that the commit should be 
held up for this reason alone.
A: Attention. Something in the patch needs attention from the author, 
the committer, or both. Resolving the matter may or may not require a 
change to the patch.
Blank: I did not review this aspect of the patch.

[CC] Applies cleanly (commit bd87416648929f38c0173f047776d7675ea8a10d, 
after applying the previous commit in this series)
[CC] Cryptographic hash
[C ] Synopsis & Description
[C ] Package license matches source license
[C ] No compiler warnings
[CC] `guix lint` output
[CC] `guix style` output
[C ] Package builds (my host is an x86_64 Xen guest)
[  ] Builds for other architectures
[  ] No bundled software
[CC] Works in container with minimal inputs
[C ] `guix size` output
[CC] Dependent packages (`guix refresh --list-dependent PACKAGE`)
[C ] `guix build --rounds=2`
[C ] Gender-neutral wording
[C ] One set of related changes
[  ] Simulated guix pull

# Applies Cleanly
Git reported that there were some "whitespace errors" due to spaces 
before tabs. This is because the commit includes a new patch and the 
patch applies to a file that uses tabs. The diff format requires that a 
space appears before the tabs, so this error is erroneous.

# Cryptographic hash
A key with a fingerprint of 968479a1aff927e37d1a566bb5690eeebb952194 
verified a checkout that hashes to the same value as found in the 
package. This key is listed on the Ubuntu keyserver 
(https://keyserver.ubuntu.com/pks/lookup?search=B5690EEEBB952194&fingerprint=on&op=index) 
with two signatures from other keys. One of them is a key that only has 
a self-signature, the other has a large number of other signatures and 
claims to belong to Andy Brody 
(https://keyserver.ubuntu.com/pks/lookup?op=vindex&search=0xc7090b1a5f57cdc5). 
The key was not found on the OpenPGP keyserver. The only UID attached to 
the key is "GitHub <noreply@github.com>", implying that the GitHub 
server is ultimately responsible for maintaining source integrity.

# guix lint output
Guix lint reports 2 issues neither of which require action.

It states that the new version is "probably vulnerable to 
CVE-2022-2989". However, the discussion at 
https://bugzilla.redhat.com/show_bug.cgi?id=2121445 indicates that fix 
is implemented by 
https://github.com/containers/podman/pull/15696/commits/21540161f20daffd884eba99b2cc31373c9a0ec4. 
My copy of the checkout contains commit 
d82a41687e614d9ac8b2d169dee47fe226835e4c which has an identical diff.

It also states that line 396 is too long, but this is due to a long 
string which consists of a single "word" (a path).

# guix style output
The tool wants to change the formatting of the majority of lines in this 
package, which were not changed in this patch, leading to a much larger 
diff than what has been submitted.

# Container with minimal inputs
I ran the following command:

```
$ ./pre-inst-env guix shell -C podman -- podman ps
```

This caused podman to print 2 warnings and 1 error in addition to the 
header line for containers (as I do not have any containers on my 
system, I expected the remaining output to be blank). The warnings & 
error were related to "/" not being a shared mount (it's not) and 
operating in rootless mode (the output made sense based on rootless.md 
in the root of the repository).

# Dependent packages
There is one dependent package, distrobox, which continues to build 
successfully after applying this patch.
Tomas Volf Feb. 16, 2024, 9:16 p.m. UTC | #3
Hi,

thanks for the review :)

Skyler Ferris <skyvine@protonmail.com> writes:

> Hi Tomas,
>
> Thanks for submitting this patch! I am adding some review notes to this 
> issue in hopes of making the process more efficient for the committer. I 
> noticed that `guix style` applies some minor changes to the definition 
> of passt which should ideally be followed, but I don't expect that this 
> would hold up the commit (and I'm not sure if it would be appropriate or 
> intrusive to add a new patch to this thread with the style changes 
> applied). Everything else looks good! I will add a couple more messages 
> to this thread which contain full notes of items I reviewed, for 
> transparency and clarity. Please let me know if there is anything I 
> could do to improve my review process!

Only thing I would maybe mention is that as far as I can tell, I did not
receive any emails regarding your comments.  I am not sure if that is
just how debbugs works, but if I did not open the bug for unrelated
reasons, I would have never noticed you commented.

Have a nice day,
Tomas Volf
diff mbox series

Patch

diff --git a/gnu/local.mk b/gnu/local.mk
index ab690795a7..bdd9338ebc 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1785,6 +1785,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/plasp-fix-normalization.patch \
   %D%/packages/patches/plasp-include-iostream.patch \
   %D%/packages/patches/pocketfft-cpp-prefer-preprocessor-if.patch			\
+  %D%/packages/patches/podman-Modify-search-for-binaries-to-fit-Guix-model.patch \
   %D%/packages/patches/pokerth-boost.patch			\
   %D%/packages/patches/ppsspp-disable-upgrade-and-gold.patch		\
   %D%/packages/patches/procps-strtod-test.patch                 \
diff --git a/gnu/packages/containers.scm b/gnu/packages/containers.scm
index 5cd81e85a6..f958f46fca 100644
--- a/gnu/packages/containers.scm
+++ b/gnu/packages/containers.scm
@@ -354,7 +354,7 @@  (define-public cni-plugins
 (define-public podman
   (package
     (name "podman")
-    (version "4.4.1")
+    (version "4.9.3")
     (source
      (origin
        (method git-fetch)
@@ -365,8 +365,11 @@  (define-public podman
        ;; FIXME: Btrfs libraries not detected by these scripts.
        (snippet '(substitute* "Makefile"
                    ((".*hack/btrfs.*") "")))
+       (patches
+        (search-patches
+         "podman-Modify-search-for-binaries-to-fit-Guix-model.patch"))
        (sha256
-        (base32 "0qbr6rbyig3c2hvdvmd94jjkg820hpdz6j7dgyv62dl6wfwvj5jj"))
+        (base32 "17g7n09ndxhpjr39s9qwxdcv08wavjj0g5nmnrvrkz2wgdqigl1x"))
        (file-name (git-file-name name version))))
 
     (build-system gnu-build-system)
@@ -393,10 +396,11 @@  (define-public podman
                 (invoke "make" "remotesystem"))))
           (add-after 'unpack 'fix-hardcoded-paths
             (lambda _
-              (substitute* (find-files "libpod" "\\.go")
-                (("exec.LookPath[(][\"]slirp4netns[\"][)]")
-                 (string-append "exec.LookPath(\""
-                                (which "slirp4netns") "\")")))
+              (substitute* "vendor/github.com/containers/common/pkg/config/config.go"
+                (("@SLIRP4NETNS_DIR@")
+                 (string-append #$slirp4netns "/bin"))
+                (("@PASST_DIR@")
+                 (string-append #$passt "/bin")))
               (substitute* "hack/install_catatonit.sh"
                 (("CATATONIT_PATH=\"[^\"]+\"")
                  (string-append "CATATONIT_PATH=" (which "true"))))
@@ -426,11 +430,12 @@  (define-public podman
            libassuan
            libseccomp
            libselinux
+           passt
            slirp4netns))
     (native-inputs
      (list bats
            git
-           go-1.19
+           go-1.21
            ; strace ; XXX debug
            pkg-config
            python))
@@ -439,7 +444,10 @@  (define-public podman
     (description
      "Podman (the POD MANager) is a tool for managing containers and images,
 volumes mounted into those containers, and pods made from groups of
-containers.")
+containers.
+
+The @code{machine} subcommand is not supported due to gvproxy not being
+packaged.")
     (license license:asl2.0)))
 
 (define-public buildah
diff --git a/gnu/packages/patches/podman-Modify-search-for-binaries-to-fit-Guix-model.patch b/gnu/packages/patches/podman-Modify-search-for-binaries-to-fit-Guix-model.patch
new file mode 100644
index 0000000000..27a9421285
--- /dev/null
+++ b/gnu/packages/patches/podman-Modify-search-for-binaries-to-fit-Guix-model.patch
@@ -0,0 +1,120 @@ 
+From 914aed3e04f71453fbdc30f4287e13ca3ce63a36 Mon Sep 17 00:00:00 2001
+From: Tomas Volf <~@wolfsden.cz>
+Date: Wed, 14 Feb 2024 20:02:03 +0100
+Subject: [PATCH] Modify search for binaries to fit Guix model
+
+Podman basically looked into the $PATH and into its libexec.  That does not fit
+Guix's model very well, to an additional option to specify additional
+directories during compilation was added.
+
+* pkg/rootless/rootless_linux.go
+(tryMappingTool): Also check /run/setuid-programs.
+* vendor/github.com/containers/common/pkg/config/config.go
+(extraGuixDir): New function.
+(FindHelperBinary): Use it.
+* vendor/github.com/containers/storage/pkg/unshare/unshare_linux.go
+(guixLookupSetuidPath): New function.
+(Start): Use it.
+---
+ pkg/rootless/rootless_linux.go                |  3 +++
+ .../containers/common/pkg/config/config.go    | 23 +++++++++++++++++++
+ .../storage/pkg/unshare/unshare_linux.go      | 14 +++++++++--
+ 3 files changed, 38 insertions(+), 2 deletions(-)
+
+diff --git a/pkg/rootless/rootless_linux.go b/pkg/rootless/rootless_linux.go
+index d303c8b..0191d90 100644
+--- a/pkg/rootless/rootless_linux.go
++++ b/pkg/rootless/rootless_linux.go
+@@ -102,6 +102,9 @@ func tryMappingTool(uid bool, pid int, hostID int, mappings []idtools.IDMap) err
+ 		idtype = "setgid"
+ 	}
+ 	path, err := exec.LookPath(tool)
++	if err != nil {
++		path, err = exec.LookPath("/run/setuid-programs/" + tool)
++	}
+ 	if err != nil {
+ 		return fmt.Errorf("command required for rootless mode with multiple IDs: %w", err)
+ 	}
+diff --git a/vendor/github.com/containers/common/pkg/config/config.go b/vendor/github.com/containers/common/pkg/config/config.go
+index 75b917f..ed2f131 100644
+--- a/vendor/github.com/containers/common/pkg/config/config.go
++++ b/vendor/github.com/containers/common/pkg/config/config.go
+@@ -1102,6 +1102,24 @@ func findBindir() string {
+ 	return bindirCached
+ }
+ 
++func extraGuixDir(bin_name string) string {
++	if (bin_name == "slirp4netns") {
++		return "@SLIRP4NETNS_DIR@";
++	} else if (bin_name == "pasta") {
++		return "@PASST_DIR@";
++	} else if (strings.HasPrefix(bin_name, "qemu-")) {
++		return "@QEMU_DIR@";
++	} else if (bin_name == "gvproxy") {
++		return "@GVPROXY_DIR@";
++	} else if (bin_name == "netavark") {
++		return "@NETAVARK_DIR@";
++	} else if (bin_name == "aardvark-dns") {
++		return "@AARDVARK_DNS_DIR@";
++	} else {
++		return "";
++	}
++}
++
+ // FindHelperBinary will search the given binary name in the configured directories.
+ // If searchPATH is set to true it will also search in $PATH.
+ func (c *Config) FindHelperBinary(name string, searchPATH bool) (string, error) {
+@@ -1109,6 +1127,11 @@ func (c *Config) FindHelperBinary(name string, searchPATH bool) (string, error)
+ 	bindirPath := ""
+ 	bindirSearched := false
+ 
++	if dir := extraGuixDir(name); dir != "" {
++		/* If there is a Guix dir, skip the PATH search. */
++		dirList = append([]string{dir}, dirList...)
++	}
++
+ 	// If set, search this directory first. This is used in testing.
+ 	if dir, found := os.LookupEnv("CONTAINERS_HELPER_BINARY_DIR"); found {
+ 		dirList = append([]string{dir}, dirList...)
+diff --git a/vendor/github.com/containers/storage/pkg/unshare/unshare_linux.go b/vendor/github.com/containers/storage/pkg/unshare/unshare_linux.go
+index a8dc1ba..0b0d755 100644
+--- a/vendor/github.com/containers/storage/pkg/unshare/unshare_linux.go
++++ b/vendor/github.com/containers/storage/pkg/unshare/unshare_linux.go
+@@ -26,6 +26,16 @@ import (
+ 	"github.com/syndtr/gocapability/capability"
+ )
+ 
++func guixLookupSetuidPath(prog string) (string, error) {
++	path, err := exec.LookPath(prog)
++	if err != nil {
++		path, err = exec.LookPath("/run/setuid-programs/" + prog)
++	}
++	return path, err
++}
++
++
++
+ // Cmd wraps an exec.Cmd created by the reexec package in unshare(), and
+ // handles setting ID maps and other related settings by triggering
+ // initialization code in the child.
+@@ -237,7 +247,7 @@ func (c *Cmd) Start() error {
+ 			gidmapSet := false
+ 			// Set the GID map.
+ 			if c.UseNewgidmap {
+-				path, err := exec.LookPath("newgidmap")
++				path, err := guixLookupSetuidPath("newgidmap")
+ 				if err != nil {
+ 					return fmt.Errorf("finding newgidmap: %w", err)
+ 				}
+@@ -297,7 +307,7 @@ func (c *Cmd) Start() error {
+ 			uidmapSet := false
+ 			// Set the UID map.
+ 			if c.UseNewuidmap {
+-				path, err := exec.LookPath("newuidmap")
++				path, err := guixLookupSetuidPath("newuidmap")
+ 				if err != nil {
+ 					return fmt.Errorf("finding newuidmap: %w", err)
+ 				}
+-- 
+2.41.0
+