CI: Add RPM distributions and use containers

[Conan-Kudo]

This will ensure we have a reasonable ability to catch issues that affect Fedora/CentOS/RHEL/openSUSE before it gets shipped in a release.

[herostrat]

How difficult would it be to add a BSD, e.g. FreeBSD?
Even just compile testing would be very beneficial I think.

[marcdeop]

How difficult would it be to add a BSD, e.g. FreeBSD? Even just compile testing would be very beneficial I think.

FreeBSD is not officially supported by Github Actions as of now. See request (here possible solutions in the discussion but... well, they are not the best ;--)

I would keep FreeBSD out for now (at least not in this PR).

[Vogtinator]

Could you add registry.opensuse.org/opensuse/tumbleweed as well? I could open a separate PR but it would depend on this anyway.

[Vogtinator]

IMO we don't need the full matrix of clang/gcc and USE_PAM OFF/ON for every OS.

[Conan-Kudo]

Could you add registry.opensuse.org/opensuse/tumbleweed as well? I could open a separate PR but it would depend on this anyway.

I've added tumbleweed-dnf since that would work with the steps we have here.

[Conan-Kudo]

@Vogtinator why is curl, tar, and gzip missing from the openSUSE containers? Without them, GitHub Actions can't download the sources into the environment...

[Conan-Kudo]

Anyway, made an SR to fix that for the Tumbleweed DNF image: https://build.opensuse.org/request/show/1066820

[Vogtinator]

@Vogtinator why is curl, tar, and gzip missing from the openSUSE containers? Without them, GitHub Actions can't download the sources into the environment...

They're meant to be minimal. For actually using the containers, you'll have to install some packages anyway. This is true here as well: Just move the step for installing dependencies before the checkout action and install curl etc. as well.

[aleixpol]

Can somebody explain to me how it knows to do the for deb/rpm accordingly?

[marcdeop]

Can somebody explain to me how it knows to do the for deb/rpm accordingly?

It's based on the matrix and the ifs:

if: ${{ contains(matrix.container-image, 'ubuntu') }}

In the original submission, those ifs where at the end of the array which made readability bad (my opinion). This has been fixed now.

[marcdeop]

Is there anything really blocking this from being merged?

[Conan-Kudo]

Yup, and I'm working on that.

We could also comment out/remove the failing builds and leave them for another iteration.

You know, baby steps :-)

Meh. I know what's wrong and how to fix them.

[Conan-Kudo]

This is finally now good to land!

[Vogtinator]

The only remaining topic then is the build matrix. IMO {fedora,centos,tumbleweed,ubuntu}-{clang,gcc}-{pam,no-pam} is a bit too much.

What about disabling PAM OFF builds on most of them? (I wonder what the code is for anyway)

[Conan-Kudo]

That actually adds complexity to the pipeline logic. I deliberately didn't bother because it's more work to make that possible.

[Vogtinator]

That actually adds complexity to the pipeline logic. I deliberately didn't bother because it's more work to make that possible.

Might be as simple as this:

        container-image:
          - "quay.io/fedora/fedora:rawhide"
          - "quay.io/centos/centos:stream9"
          - "registry.opensuse.org/opensuse/tumbleweed-dnf"
          - "docker.io/library/ubuntu:22.04"
        compiler:
          - gcc
          - clang
        pam:
          - ON
        include:
          - container-image: "docker.io/library/ubuntu:22.04"
            pam: OFF

(Somehow I hit Ctrl-Return instead of Shift-Return, that is apparently the shortcut for Comment and close. TIL.)

[Conan-Kudo]

Who uses SDDM without PAM, by the way? Even Slackware adopted PAM in 2020: https://alien.slackbook.org/blog/slackware-introduces-pam-into-its-core/

[Vogtinator]

Yeah, maybe we can just drop that. I doubt that there is any practical relevance and I don't think it's useful for test/dev setups either.

[Conan-Kudo]

@Vogtinator I've added a commit to drop the PAM build modes.

[Vogtinator]

IMO that should be done together with removing the code itself, which is something for a separate PR. But maybe the other maintainers don't mind? @aleixpol @davidedmundson @plfiorini

[Conan-Kudo]

Then do you want me to drop that commit and we can merge this, or what?

[Vogtinator]

As long as the PAM code is there, it should be built tested, but IMO not 8 times.

[aleixpol]

@Conan-Kudo remove the last commit, then we (I?) can add the commit that removes PAM, so we don't need to be building it 8 times for too long.

[Conan-Kudo]

@aleixpol Done.

[aleixpol]

This makes PAM mandatory as discussed #1685