crystal-lang / distribution-scripts

40 stars 24 forks source link

Generic dnf support #322

Open gabriel-ss opened 1 month ago

gabriel-ss commented 1 month ago

I was setting up a pipeline today on an amazon linux 2023-based container and got caught by surprise when the crystal installer script didn't work. I found it odd since I used it dozens of times to install the compiler on older amazon linux 2 containers, so I did a little debugging and found what was happening.

The script cleverly tries to infer the correct source from the installed package manager when the information provided by the os-release is not enough. The problem with this is that it assumes that every unknown distro using rpm will manage packages with yum. When an unknown distro uses dnf (like the newer versions of AL), the script just fails. Replacing

    *)
      # If there's no dedicated repository for the distro, try to figure out
      # if the distro is apt or rpm based and use a default repository.
      _discover_distro_type

      case "$DISTRO_TYPE" in
      deb)
        DISTRO_REPO="Debian_Unstable"
        ;;
      rpm)
        DISTRO_REPO="RHEL_7"
        ;;
      *)
        _error "Unable to identify distribution type ($ID). You may specify a repository with the environment variable DISTRO_REPO"
        _error "Please, report to https://forum.crystal-lang.org/c/help-support/11"
        exit 1
        ;;
      esac
  esac
}

_discover_distro_type() {
  DISTRO_TYPE=""
  [[ $(command -v apt-get) ]] && DISTRO_TYPE="deb" && return
  [[ $(command -v yum) ]]     && DISTRO_TYPE="rpm" && return
}

with

    *)
      # If there's no dedicated repository for the distro, try to figure out
      # if the distro is apt, dnf or yum based and use a default repository.
      _discover_distro_type

      case "$DISTRO_TYPE" in
      apt)
        DISTRO_REPO="Debian_Unstable"
        ;;
      dnf)
        DISTRO_REPO="Fedora_Rawhide"
        ;;
      yum)
        DISTRO_REPO="RHEL_7"
        ;;
      *)
        _error "Unable to identify distribution type ($ID). You may specify a repository with the environment variable DISTRO_REPO"
        _error "Please, report to https://forum.crystal-lang.org/c/help-support/11"
        exit 1
        ;;
      esac
  esac
}

_discover_distro_type() {
  DISTRO_TYPE=""
  [[ $(command -v apt-get) ]] && DISTRO_TYPE="apt" && return
  [[ $(command -v dnf) ]]     && DISTRO_TYPE="dnf" && return
  [[ $(command -v yum) ]]     && DISTRO_TYPE="yum" && return
}

should solve the problem with distros using the full version of dnf.

The other problem that I found is that the script is not compatible with microdnf. The script currently tries to install the config-manager dnf command to add the crystal repo, which not only can bring a lot of unneded python depencies but also makes it incompatible with microdnf. Since microdnf will eventually replace the old python based dnf, I think supporting it is a good idea. By modifing

_install_dnf() {
  dnf install -y 'dnf-command(config-manager)'
  dnf config-manager --add-repo https://download.opensuse.org/repositories/${OBS_PROJECT}/$DISTRO_REPO/${OBS_PROJECT}.repo

to use the same process used by _install_yum

_install_dnf() {
  _install_rpm_key

  cat > /etc/yum.repos.d/crystal.repo <<EOF
[crystal]
name=Crystal (${DISTRO_REPO})
type=rpm-md
baseurl=https://download.opensuse.org/repositories/${OBS_PROJECT//:/:\/}/${DISTRO_REPO}/
gpgcheck=1
gpgkey=https://download.opensuse.org/repositories/${OBS_PROJECT//:/:\/}/${DISTRO_REPO}/repodata/repomd.xml.key
enabled=1
EOF

both microdnf and dnf work as expected (tested the modified script on a Fedora 40 and a Fedora Rawhide containers).

Combining both modifications allows the script to successfully install crystal on public.ecr.aws/lambda/provided:al2023, which is based on fedora and uses microdnf. Is there a chance to see those changes being implemented? I could prepare a PR if there is interest on it.

gabriel-ss commented 1 month ago

Hey there, @straight-shoota, sorry to bother, do you have any thoughts on this one?

straight-shoota commented 1 month ago

The installer seems to work just fine in an Amazon Linux 2023 image: docker run --rm -it amazonlinux:2023 /bin/sh -c 'curl https://crystal-lang.org/install.sh | bash'.

Nevertheless, the suggested changes look reasonable. I'm not very familiar with the RPM ecosystem though, so I trust your judgement.

Probably best to have both changes in separate PRs because they're not directly related. In companion with the first change, it probably makes sense to rename $DISTRO_TYPE to something more descriptive like $PACKAGE_MANAGER?

gabriel-ss commented 1 month ago

It does indeed work on amazonlinux:2023 since in that image yum is a symlink to the full dnf. The public.ecr.aws/lambda/provided:al2023 image is a striped down version used by aws lambdas that, among other things, doesn't include the symlink nor the full dnf.

I agree with your points. I'll make two separate PRs and rename the variable as you suggested.

Thank you for your time!

gabriel-ss commented 1 month ago

Done! Got a little confused by the default github behavior of branches on forks, but it's fixed now. Both #323 and #324 should resolve the issue if merged.