Fix SLES, openSUSE, Ubuntu

[pevik]

2 fixes for 22db0cd e572aa5:

Previously these 3 commands didn't print anything:

./install_pkg.pm --distro opensuse --run --cmd
./install_pkg.pm --distro sles --run --cmd
./install_pkg.pm --distro ubuntu --run --cmd

[pevik]

Ah, bug, it needs to be fixed.

[metan-ucw]

It says "Remove Ubuntu" but leap is removed as well. If that was intentional it should be at least mentioned in the commit message.

[pevik]

It says "Remove Ubuntu" but leap is removed as well. If that was intentional it should be at least mentioned in the commit message.

Thanks! Unintentional, Leap put back. Added your review-by to that commit.

[metan-ucw]

I do not like the unless part here that much. Maybe we should just do:

my $pkg;

pkg = $pkg_map{"$foo-$distro");

return $pkg if defined;

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

return $pkg if defined $pkg;

return $pkg_map{"$foo");

[metan-ucw]

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?

[pevik]

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.

[metan-ucw]

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.

[pevik]

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.

[pevik]

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.

[metan-ucw]

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?

[pevik]

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).