Improve Linux distribution detection

[Regrau]

This will close #983. I took the liberty to reorder the file a little so one does not need to parse the whole file to understand what will happen. As it is now, this will implement both ways of checking for the distribution as discussed. It will check for the distribution and the package manager. If those checks fail it will try to work with the package manager only on a best effort basis.

Now you only need to parse the main function to understand the intent of the file. Addition of new distributions should hopefully be easier as well since one won't need to parse the long if elif else decision tree.

I'd like to simplify the new install_$DISTRO_dependencies functions a little more too but I'd need input on if it is the right course to take.

[jandubois]

I wonder if this code should be more robust against various conditions:

• What if /etc/os-release doesn't exist?

  E.g. use [ -f /etc/os-release ] && source /etc/os-release ?

• What if ID or ID_LIKE are not set?

  This would throw an error because we have set -u at the top. Maybe preset the variables to the empty string before sourcing /etc/os-release?

• What if ID_LIKE lists multiple ids (e.g. rhel fedora instead of just fedora?

  Maybe do some regular expression match? /bin/sh doesn't support this though, and we can't rely on bash. Can we rely on expr being available, so we can do something like expr "$ID_LIKE" : ".*fedora" >/dev/null?

[Regrau]

You are absolutely right the code should be made more robust. I implemented all of your suggestions.

As for the availability of expr we should be able to rely on that since it [seems to be part of the standard].(https://pubs.opengroup.org/onlinepubs/9699919799/)

Let me know if you think there is anything else I should add/remove/rework.

[jandubois]

Please fix shellcheck errors and remove trailing spaces from commands:

--- pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh
+++ pkg/cidata/cidata.TEMPLATE.d/boot/30-install-packages.sh
@@ -6,6 +6,7 @@ DISTRIBUTION=""
 INSTALL_IPTABLES=0
 ID_LIKE=""
 ID=""
+# shellcheck source=/dev/null
 [ -f /etc/os-release ] && . /etc/os-release


@@ -15,22 +16,22 @@ main() {
        install_minimal_dependencies
        setup_dns
        # update_fuse_conf has to be called after installing all the packages,
-       # otherwise apt-get fails with conflict
-       update_fuse_conf
+       # otherwise apt-get fails with conflict
+       update_fuse_conf
 }

 determine_distribution() {
-       if command -v apt-get >/dev/null 2>&1 && [ $(expr "$ID_LIKE" : ".*debian") -gt 0 ]; then
+       if command -v apt-get >/dev/null 2>&1 && [ "$(expr "$ID_LIKE" : ".*debian")" -gt 0 ]; then
                DISTRIBUTION="debian_like"
-       elif command -v dnf >/dev/null 2>&1 && [ $(expr "$ID" : ".*fedora") -gt 0 ]; then
+       elif command -v dnf >/dev/null 2>&1 && [ "$(expr "$ID" : ".*fedora")" -gt 0 ]; then
                DISTRIBUTION="redhat_like"
-       elif command -v yum >/dev/null 2>&1 && [ $(expr "$ID_LIKE" : ".*centos") -gt 0 ]; then
+       elif command -v yum >/dev/null 2>&1 && [ "$(expr "$ID_LIKE" : ".*centos")" -gt 0 ]; then
                DISTRIBUTION="centos_like"
-       elif command -v pacman >/dev/null 2>&1 && [ $(expr "$ID_LIKE" : ".*arch") -gt 0 ]; then
+       elif command -v pacman >/dev/null 2>&1 && [ "$(expr "$ID_LIKE" : ".*arch")" -gt 0 ]; then
                DISTRIBUTION="arch_like"
-       elif command -v zypper >/dev/null 2>&1 && [ $(expr "$ID_LIKE" : ".*suse") -gt 0 ]; then
+       elif command -v zypper >/dev/null 2>&1 && [ "$(expr "$ID_LIKE" : ".*suse")" -gt 0 ]; then
                DISTRIBUTION="suse_like"
-       elif command -v apk >/dev/null 2>&1 && [ $(expr "$ID" : ".*alpine") -gt 0 ]; then
+       elif command -v apk >/dev/null 2>&1 && [ "$(expr "$ID" : ".*alpine")" -gt 0 ]; then
                DISTRIBUTION="alpine_like"
        else
                guess_distribution_based_on_package_manager

(the diff above doesn't show the trailing spaces correctly, but you can see the 2 lines that have them).

[jandubois]

I assume this should have tested ID_LIKE instead:

$ nerdctl run --rm -it redhat/ubi8 grep "^ID" /etc/os-release
ID="rhel"
ID_LIKE="fedora"

Since the values for ID and ID_LIKE don't seem to be standardized, I wonder if it wouldn't be best to always check both settings for a whole range of values, something like (untested):

os_like() {
    for candidate in $@; do
        if [ "$ID" = "$candidate" ]; then
            return 0
        fi
        if [ "$(expr "$ID_LIKE" : ".*$candidate")" -gt 0 ]; then
            return 0
        fi
    done
    return 1
}

and then use it like this:

	elif command -v dnf >/dev/null 2>&1 && os_like redhat rhat rhel fedora; then
		DISTRIBUTION="redhat_like"

It will still require special ordering to make sure that centos is checked first, so it is not misidentified as redhat:

$ nerdctl run --rm -it centos grep "^ID" /etc/os-release
ID="centos"
ID_LIKE="rhel fedora"
$ nerdctl run --rm -it rockylinux:9 grep "^ID" /etc/os-release
ID="rocky"
ID_LIKE="rhel centos fedora"
$ nerdctl run --rm -it almalinux:9 grep "^ID" /etc/os-release
ID="almalinux"
ID_LIKE="rhel centos fedora"

It only works right now because the redhat check above is actually broken for anything but fedora itself.

I assume we want to classify almalinux and rockylinux as fedora and not centos, in which case we would have to use [ "$ID" = "centos" ] as a special case at the very top.

There may be more special cases; I don't have time to check all the possible images now. I think it would be great to move the os detection to a separate file (still under cidata), but then add a test script that verifies that is guesses the right setting for a bunch of known images. That way we can add additional images in the future, and adjust the rules, and still be confident that it continues to work for all other images.

[jandubois]

For testing I was thinking of something like this:

nerdctl run --rm -v $PWD/pkg/cidata/cidata.TEMPLATE.d/boot:/ci almalinux:9 sh -c "/ci/30-install-packages.sh && echo $DISTRIBUTION"

It obviously doesn't work right now, but should be easy enough to get to work, and then we just need to call it a bunch of times with known images, and check the resulting DISTRIBUTION value.

[Regrau]

I implemented an external function for distribution detection. The functions in the script are part of the salt-bootstrap.sh. They're much more comprehensive than I could've written them.
As both lima and salt-bootstrap are licensed under the apache license 2 there should be no problem license wise.

As this is my first contribution with the code of others I'm not quite sure if there is anything else that I need to do other than adding the license notice in the file. Any help in that regard is much appreciated.

If the general direction is ok I'd polish up this POC over the next week. If not, let me know and I'll look into what else I can do here.

[jandubois]

I think this is good, but I'll be on vacation and mostly offline for the next 4 weeks, so I hope @AkihiroSuda will provide further guidance. But I think this is all going into the right direction, and being more comprehensive than I originally expected.

[AkihiroSuda]

Sorry, looks too much complicated and also contains a bunch of dead distros like United Linux.

[AkihiroSuda]

Please add a comment to clarify the origin (and the purpose) of this file

[AkihiroSuda]

And I'm feeling this script is too much complicated.. Just grepping /etc/os-release (lsb-release) might be enough

[AkihiroSuda]

Dead distro