Skip to content
This repository has been archived by the owner on Mar 15, 2024. It is now read-only.

Fix SLES, openSUSE, Ubuntu #46

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Fix SLES, openSUSE, Ubuntu #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4425976
install_pkg: Fix kernel headers for Ubuntu
pevik Nov 26, 2021
4319466
install_pkg: Fix distro aliasing
pevik Nov 26, 2021
e858293
install_pkg: Fix --cmd for sles, opensuse, ubuntu
pevik Nov 23, 2021
5cad4ee
CI: Remove Ubuntu releases
pevik Nov 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/distro-install.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:
strategy:
fail-fast: false
matrix:
container: ["alpine:latest", "debian:testing", "debian:stable", "debian:oldstable", "fedora:latest", "opensuse/tumbleweed", "opensuse/leap", "ubuntu:groovy", "ubuntu:xenial"]
container: ["alpine:latest", "debian:testing", "debian:stable", "debian:oldstable", "fedora:latest", "opensuse/tumbleweed", "opensuse/leap"]
env: [ARGS: "--m32", ARGS: ""]

exclude:
Expand Down
33 changes: 26 additions & 7 deletions install_pkg.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ sub foo_to_pkg
'kernel-devel-alpine' => 'linux-headers',
'kernel-devel-debian' =>
'linux-headers-`dpkg --print-architecture`',
'kernel-devel-ubuntu' =>
'linux-headers-`uname -r`',
pevik marked this conversation as resolved.
Show resolved Hide resolved

# devel libs
'libacl-devel-alpine' => 'acl-dev',
Expand Down Expand Up @@ -74,6 +76,21 @@ sub foo_to_pkg
'libnuma-devel-opensuse' => 'libnuma-devel',
);

my $pkg;

$pkg = $pkg_map{"$foo-$distro"};
return $pkg if defined $pkg;

$pkg = $pkg_map{"$foo-" . get_distro_alias($distro)};
return $pkg if defined $pkg;

return $pkg_map{"$foo"};
}

sub get_distro_alias
{
my ($distro) = @_;

if ($distro eq 'sles') {
$distro = 'opensuse';
}
Expand All @@ -82,10 +99,7 @@ sub foo_to_pkg
$distro = 'debian';
}

my $pkg = $pkg_map{"$foo-$distro"};
return $pkg if defined $pkg;

return $pkg_map{"$foo"};
return $distro;
}

my @distros = qw(alpine debian fedora opensuse sles ubuntu);
Expand All @@ -107,25 +121,28 @@ sub detect_distro
sub pkg_to_m32
{
my ($distro, $pkg_name) = @_;
$distro = get_distro_alias($distro);

if ($distro eq "debian") {
#TODO: we need architecture detection for now default to i386
return "gcc-multilib" if ($pkg_name eq "gcc");
return "$pkg_name:i386";
}

return "$pkg_name-32bit" if ($distro eq "suse");
return "$pkg_name-32bit" if ($distro eq "opensuse");

return;
}

sub setup_m32
{
my ($distro) = @_;
$distro = get_distro_alias($distro);

if ($distro eq "debian") {
return ("dpkg --add-architecture i386");
}

return;
}

Expand All @@ -145,6 +162,7 @@ sub map_pkgs
sub install_pkg
{
my ($distro, $pkgs) = @_;
$distro = get_distro_alias($distro);

if ($distro eq "alpine") {
return 'apk add ' . join(' ', @$pkgs);
Expand All @@ -158,14 +176,15 @@ sub install_pkg
return 'yum install -y ' . join(' ', @$pkgs);
}

if ($distro eq 'suse') {
if ($distro eq 'opensuse') {
return 'zypper --non-interactive --ignore-unknown in ' . join(' ', @$pkgs);
}
}

sub update_pkg_db
{
my ($distro) = @_;
$distro = get_distro_alias($distro);

if ($distro eq "alpine") {
return "apk update";
Expand All @@ -179,7 +198,7 @@ sub update_pkg_db
return "yum update -y";
}

if ($distro eq "suse") {
if ($distro eq "opensuse") {
return "zypper --non-interactive ref";
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe we should just do the get_distro_alias() once in the distro detection code and return the alias if defined, then we can forget about aliases in the rest of the code right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is, that for packages we don't want to use alias, but for others we do want (installing pkg, updating package database, 32bit).

Also, some methods (at least install_ltp_pkgs()) are used from runltp-ng, but we don't export functions with @EXPORT, so I'm not sure what is the interface outside of the module, thus I wanted any method worked called from outside (not expecting to get correct distro from outside).

But sure, I'm open to rewrite code if we agree what's the clearest way.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have to pass the distro to the package mapping since debian and ubuntu have different packages while the rest of the operations are the same for the families of distributions.

Maybe it would be cleaner to have a function to map the distribution name into the package management so that we would map anything debian-like to apt, SUSE to zypper, anything from redhat to yum and alpine to apk.

That would allow us to choose the action based on the tool we are going to use rather than on some distribution name aliases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll add something like distro_to_pkg_tool() method.

Also I was thinking that having @build_pkgs, @devel_libs and @mkfs as global arrays defined on the top would be more readable than having them hidden in the functions. We also add distro versions in of these arrays in foo_to_pkg(), which is a different method. But this can wait after this fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be cleaner to have a function to map the distribution name into the package management so that we would map anything debian-like to apt, SUSE to zypper, anything from redhat to yum and alpine to apk.

Sounds good, I'll add something like distro_to_pkg_tool() method.

Well, we need that distro alias function in foo_to_pkg() function as well, thus not sure about distro_to_pkg_tool() name. You're right, it's only the case for Ubuntu (currently). I suppose we don't want to specifically check for Ubuntu there.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the only function used from the library at the moment is isntall_ltp_pkgs() that does everything including the call to the backend. I guess that it should be the other way the module should export the functions that generate the list of packages and the commandlines and that should be used. Generally the code should be cleaned up a bit, I guess that the detect_distro() function and install_ltp_pkgs() conceptually belongs somewhere else and that the interface to the module should really be the get_build_pkgs() get_runtime_pkgs() and get_install_cmds() functions. And I would declare the rest of the functions internal. Also we would need to add architecture parameter to the get_install_cmds() to properly support m32 on Debian.

Also I would add distro_to_pkg_too() function that would simply map distribution to the tool. As for the pkg_map we should do it so that we would try the distribution name first and if that fails we would use a distribution alias, so for ubuntu we would check if there is package foo-ubuntu mapping, if not we would look up an distro alias hash that would return debian, so we would try foo-debian and if that fails we would try looking up just foo. Does that sound reasonable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the interface to the module should really be the get_build_pkgs() get_runtime_pkgs() and get_install_cmds() functions.

Sure, agree.

I guess that it should be the other way the module should export the functions that generate the list of packages and the commandlines and that should be used.

FYI although I agree exported functions I meant internal arrays. As the code is IMHO a bit too complicated for just a simple thing (given how simple and thus readable are scripts in https://github.com/linux-test-project/ltp/tree/master/ci, although it cannot print things obviously and I don't mean to use bash).

I also don't like that we apped distro to the package "key" with and distro separator is the same (-) as what is used in package key (i.e. 'libaio-devel' vs. 'libaio-devel-opensuse'), sure it's obvious that it's a distro, but it's not that readable.
Also having default which can be overwritten is just strange (which distro is a default?). I'd prefer hashes of hashes array:
({runtime or build}{distro{ => (array of package)) and specify everything (no "default)

Also we should think about other scenarios when some package is not supported on particular distro, should it be empty? Or there are 2 packages needed. This feature is not needed now, but might be for runtime packages (e.g. for networking).

Expand Down