[libvirt] [jenkins-ci PATCH v2 7/9] lcitool: avoid using an env var to store package list

[Andrea Bolognani]

On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote:
[...]
> -        flattened = []
> +        pkgs = []
>          for item in temp:
> -            if temp[item] is not None and temp[item] not in flattened:
> -                flattened += [temp[item]]
> +            pkgname = temp[item]
> +            if pkgname is None:
> +                continue
> +            if pkgname not in pkgs:
> +                pkgs.append(pkgname)

Nothing against refactoring the code this way, but such a change
belongs in its own patch.

[...]
> -        sys.stdout.write("ENV PACKAGES ")
> -        sys.stdout.write(" \\\n             ".join(sorted(flattened)))
> -
> +        varmap = {}
> +        varmap["pkgs"] = "".join([" \\\n            " + pkgname
> +                                  for pkgname in sorted(pkgs)])

Unlike the original, this pointlessly loops through sorted(pkgs)
one additional time and adds a phantom element to the beginning of
the list. So, once you put everything together...

>          if package_format == "deb":
>              sys.stdout.write(textwrap.dedent("""
>                  RUN DEBIAN_FRONTEND=noninteractive && \\
>                      ( \\
>                          apt-get update && \\
>                          apt-get dist-upgrade -y && \\
> -                        apt-get install --no-install-recommends -y ${PACKAGES} && \\
> +                        apt-get install --no-install-recommends -y %(pkgs)s && \\
>                          apt-get autoremove -y && \\
>                          apt-get autoclean -y \\
>                      )

... the result looks like

  apt-get install --no-install-recommends -y  \
      augeas-tools \
      autoconf \
      automake \
      ...

Notice the double space between '-y' and '\' as well as the weird
indentation. A better solution would be to do

  varmap["pkgs"] = " \\\n                ".join(sorted(pkgs))

followed by

  sys.stdout.write(textwrap.dedent("""
       ...
       apt-get install --no-install-recommends -y \\
               %(pkgs)s && \\
       apt-get autoremove -y && \\
       ...

which is clearer and results in the more pleasant

  apt-get install --no-install-recommends -y \
          augeas-tools \
          autoconf \
          automake \
          ...

> -            """))
> +            """) % varmap )

Again, I don't have a problem with using this syntax for string
formatting (and here it's actually much clearer than the one we
are currently using), but I don't want the script to become
inconsistent so we have to stick to a single style.

-- 
Andrea Bolognani / Red Hat / Virtualization