Merge ~xnox/launchpad-buildd:image-targets-fix into launchpad-buildd:master

[Dimitri John Ledkov]

target/chroot: fix passing space separate env variable values

[Colin Watson (cjwatson)]

You've misunderstood what lpbuildd/target/lxd.py is doing here. It has to quote arguments in the case where cwd is not None because it's explicitly constructing a shell command line. The rule is that there must be exactly one layer of quoting per layer of parsing, so because the shell invocation adds another layer of parsing, it has to add another layer of quoting to match. The lxd backend is correct.

The problem here is that the call to shell_escape in the lines you're modifying is flat-out wrong (which seems to have been my fault). There is no parsing involved here: args is being constructed as an argument list rather than something that's passed to a shell, and adding extra quoting to those is incorrect. The correct fix would be simply to remove the call to shell_escape on line 65. Could you please do that instead? It doesn't need a comment - it just needs to have the incorrect function call removed.

It would also be helpful to add a test for this, which could go alongside the other test_run* methods in lpbuildd/target/tests/test_chroot.py.

review: Needs Fixing

[Colin Watson (cjwatson)]

We've landed https://code.launchpad.net/~cjwatson/launchpad-buildd/+git/launchpad-buildd/+merge/387221 instead of this. Thanks for the suggestion.

review: Disapprove

Unmerged commits

6ea1902... by Dimitri John Ledkov on 2020-06-24

target/chroot: fix passing space separate env variable values

image_targets list is encoded as space separated environment variable,
which is encoded in pylxd backend correctly, but not in the chroot
one.

Maybe lists should be encoded as comma separated, at lease
livecd-rootfs supports that.

LP: https://bugs.launchpad.net/launchpad-buildd/+bug/1884936

diff --git a/lpbuildd/target/chroot.py b/lpbuildd/target/chroot.py
index 203142c..cbed3c7 100644
--- a/lpbuildd/target/chroot.py
+++ b/lpbuildd/target/chroot.py
@@ -61,9 +61,27 @@ class Chroot(Backend):
             echo=False, **kwargs):
         """See `Backend`."""
         if env:
-            args = ["env"] + [
-                "%s=%s" % (key, shell_escape(value))
-                for key, value in env.items()] + args
+            # Currently pylxd backend does not quote the k=v pairs
+            # when passing them to the lxc executor, but it does quote
+            # every arg if cwd is changed.  Match that behaviour in
+            # chroot executor too, and do not quote the value, of the
+            # key, if shell_escape will happen later when cwd is set.
+            # Otherwise instead of the expected 'IMAGE_TARGETS=tarball
+            # squashfs' arg, it becomes \'IMAGE_TARGET\'"\'"\'tarball
+            # squashfs\'"\'"\'\', and when decoded by make-config
+            # python script in livecd-rootfs it ends up getting parsed
+            # as ["'tarball", "squashfs'"], as in first value prefixed
+            # and last value suffixed with ' Alternatively,
+            # make-config script in livecd-rootfs can add a workaround
+            # to parse such bad values.
+            if cwd:
+                args = ["env"] + [
+                    "%s=%s" % (key, value)
+                    for key, value in env.items()] + args
+            else:
+                args = ["env"] + [
+                    "%s=%s" % (key, shell_escape(value))
+                    for key, value in env.items()] + args
         if self.arch is not None:
             args = set_personality(args, self.arch, series=self.series)
         if cwd is not None: