Re: [PATCH v4 2/4] Jobs based on custom runners: build environment docs and playbook

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4317 bytes --]

On Tue, Oct 20, 2020 at 07:52:43PM +0200, Andrea Bolognani wrote:
> On Sun, 2020-10-18 at 21:50 -0400, Cleber Rosa wrote:
> > +++ b/scripts/ci/setup/build-environment.yml
> > @@ -0,0 +1,233 @@
> > +---
> > +- name: Installation of basic packages to build QEMU
> > +  hosts: all
> > +  tasks:
> 
> My Ansible-fu is a bit rusty at the moment, so please double-check my
> claims and apologies in advance for the ones that I will get wrong O:-)
> 
> > +    - name: Install basic packages to build QEMU on Ubuntu 18.04/20.04
> > +      apt:
> > +        update_cache: yes
> > +        # Originally from tests/docker/dockerfiles/ubuntu1804.docker
> > +        pkg:
> > +          - ccache
> > +          - clang
> 
> Instead of using the 'apt' module here, and the equivalent module
> further down, you could just do
> 
>   package:
>     name:
>       - pkg1
>       - pkg2
>         ...
>     state: present
> 
> every single time and let Ansible take care of the differences for
> you.
>

I'm almost sure that this was a conscious decision.  I remeber it had
to do with not being able to set `update_cache`, and failures on
recently installed systems and containers that did not update the APT
cache.  There may be something else, but I'll have to give it another
round of testing.

FIY, under the hood, package is not really a module, but an action
plugin that forwards these very limited options to the set or detected
package manager, so it brings uniformity in the playbook, but limits
the control too.  IMO, it's very low impact to leave it AS IS.

> > +      when: "ansible_facts['distribution'] == 'Ubuntu'"
> 
> Quoting the entire condition is not necessary, you can just have
> 
>   when: ansible_facts['distribution'] == 'Ubuntu'
> 
> or, my preference,
> 
>   when:
>     - ansible_facts['distribution'] == 'Ubuntu'
>

Yep, I've used the explicit lists when there was more than one
condition, but having a standard style is better indeed.

> which results in a nicer diff when you add/remove conditions.
> 
> > +    - name: Install packages to build QEMU on Ubuntu 18.04/20.04 on non-s390x
> > +      apt:
> > +        update_cache: yes
> > +        pkg:
> > +         - libspice-server-dev
> > +         - libxen-dev
> 
> Indentation of list items is inconsistent here.
>

True.  Fixed, thanks!

> > +    - name: Install basic packages to build QEMU on FreeBSD 12.x
> > +      pkgng:
> > +        # Originally from packages on .cirrus.yml under the freebsd_12_task
> > +        name: bash,curl,cyrus-sasl,git,glib,gmake,gnutls,gsed,nettle,ninja,perl5,pixman,pkgconf,png,usbredir
>
> See above for 'pkgng' vs 'package', but at the very least this should
> be
> 
>   pkgng:
>     name:
>       - bash
>       - curl
>         ...
> 
> or each time the list is touched the resulting diff is going to be
> unmanageable.
>

The documentation suggests a comma separated list of package names:

   https://docs.ansible.com/ansible/2.8/modules/pkgng_module.html#pkgng-module

And the reason is that this module is not as smart as others, and will
run one separate command for each individual package name value:

   https://github.com/ansible/ansible/blob/v2.10.3/test/support/integration/plugins/modules/pkgng.py#L214

It's a tradeoff indeed, but I think we're aligned with the docs.

> > +    - name: Enable PowerTools repo on CentOS 8
> > +      ini_file:
> > +        path: /etc/yum.repos.d/CentOS-PowerTools.repo
> > +        section: PowerTools
> > +        option: enabled
> > +        value: "1"
> > +      when:
> > +        - "ansible_facts['distribution'] == 'CentOS'"
> > +        - "ansible_facts['distribution_major_version'] == '8'"
> 
> Another option would be to use
> 
>   command: 'dnf config-manager --set-enabled Stream-PowerTools -y'
>   args:
>     warn: no
> 
> but I have to admit the way you're doing it is very clever ;)
>

Yeah, that would require another package to be installed, and then the
command executed... So I think this is cheaper and eaiser indeed.

> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

Thanks for the review, I'll report on the additional points as soon as
I test them.  If appropriate, I'll put notes on the v5.

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]