Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opensuse SystemPackageTools incorrectly using apt-get when zypper-aptitude #8747

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Mar 31, 2021

Changelog: Bugfix: Fix opensuse SystemPackageTools incorrectly using apt-get when zypper-aptitude.
Docs: Omit

Fix #8737

@SSE4
Copy link
Contributor

SSE4 commented Apr 5, 2021

to be honest, I don't think it should be fixed that way. IMO, functions like have_apt or have_yum should just report if apt/yum is installed, and should be completely distro-agnostic (e.g. it's totally possible to install yum on debian or apt on centos). it's definitely okay if the same system reports both have_apt and have_yum as True. all the detection should be tool-based (checking that apt is available and working), rather distro based.

@memsharded
Copy link
Member Author

all the detection should be tool-based (checking that apt is available and working), rather distro based.

Apt is installed and working in OpenSuse. But it happens it is just a wrapper about zypper, so if you report that apt-get is there, the installation of packages will fail, because those packages do not exist in Suse repos. How do you want to address this issue then?

@SSE4
Copy link
Contributor

SSE4 commented Apr 5, 2021

Apt is installed and working in OpenSuse. But it happens it is just a wrapper about zypper, so if you report that apt-get is there, the installation of packages will fail, because those packages do not exist in Suse repos. How do you want to address this issue then?

not sure I get, why it will fail if Apt is installed and working in OpenSuse? it should be able to install the same packages as zypper. that's only matter of passing correct package names.

@memsharded
Copy link
Member Author

not sure I get, why it will fail if Apt is installed and working in OpenSuse? it should be able to install the same packages as zypper. that's only matter of passing correct package names.

Recipes and SystemPackageTool uses different names for different systems:

if tools.os_info.is_linux and self.settings.os == "Linux":
            if tools.os_info.with_yum:
                if tools.os_info.linux_distro == "fedora" and tools.os_info.os_version >= "32":
                    packages = ["libglvnd-devel"]
                else:
                    packages = ["mesa-libGL-devel"]
            elif tools.os_info.with_apt:
                ubuntu_20_or_later = tools.os_info.linux_distro == "ubuntu" and tools.os_info.os_version >= "20"
                debian_11_or_later = tools.os_info.linux_distro == "debian" and tools.os_info.os_version >= "11"
                pop_os_20_or_later = tools.os_info.linux_distro == "pop" and tools.os_info.os_version >= "20"
                if ubuntu_20_or_later or debian_11_or_later or pop_os_20_or_later:
                    packages = ["libgl-dev"]
                else:
                    packages = ["libgl1-mesa-dev"]
            elif tools.os_info.with_pacman:
                packages = ["libglvnd"]

The problem here is OpenSuse reporting that it has apt, the with_apt is buggy, because it does not have it. It has another different thing that fakes to be apt but it is zypper. So our with_apt reporting True for opensuse is clearly wrong, it shouldn't do that, it is very problematic for users.

@SSE4
Copy link
Contributor

SSE4 commented Apr 5, 2021

not sure I get, why it will fail if Apt is installed and working in OpenSuse? it should be able to install the same packages as zypper. that's only matter of passing correct package names.

Recipes and SystemPackageTool uses different names for different systems:

if tools.os_info.is_linux and self.settings.os == "Linux":
            if tools.os_info.with_yum:
                if tools.os_info.linux_distro == "fedora" and tools.os_info.os_version >= "32":
                    packages = ["libglvnd-devel"]
                else:
                    packages = ["mesa-libGL-devel"]
            elif tools.os_info.with_apt:
                ubuntu_20_or_later = tools.os_info.linux_distro == "ubuntu" and tools.os_info.os_version >= "20"
                debian_11_or_later = tools.os_info.linux_distro == "debian" and tools.os_info.os_version >= "11"
                pop_os_20_or_later = tools.os_info.linux_distro == "pop" and tools.os_info.os_version >= "20"
                if ubuntu_20_or_later or debian_11_or_later or pop_os_20_or_later:
                    packages = ["libgl-dev"]
                else:
                    packages = ["libgl1-mesa-dev"]
            elif tools.os_info.with_pacman:
                packages = ["libglvnd"]

The problem here is OpenSuse reporting that it has apt, the with_apt is buggy, because it does not have it. It has another different thing that fakes to be apt but it is zypper. So our with_apt reporting True for opensuse is clearly wrong, it shouldn't do that, it is very problematic for users.

yes, for me it seems to be a recipe problem. package manager vs distro are a bit orthogonal categories, and conditions should be fixed there.

@memsharded
Copy link
Member Author

yes, for me it seems to be a recipe problem. package manager vs distro are a bit orthogonal categories, and conditions should be fixed there.

I don't think so. How having our own with_apt helper return True for Suse platforms will help the users in anyway? This only causes confusion and issues.

What would be the fix you would propose to the recipe that would make sense?

@czoido
Copy link
Contributor

czoido commented Apr 5, 2021

Right now to check if apt-get is in the system we are just checking that the return code of apt-get moo is not returning an error code and we will return a True for with_apt. We don't check the output of the command because we were concerned about if the output fo the command may change over time or that the different locales would lead to different outputs.
Maybe we could improve the detection of apt-get checking the output of apt-get --version instead of executing moo and just checking if apt string is in the output in the command. That way we avoid checking the distro.

For OpenSuse with zypper-aptitude installed the return string is:

$ apt-get --version
zypper 1.14.43

For example, for Ubuntu focal:

$ apt-get --version
apt 2.0.2ubuntu0.1 (amd64)
Supported modules:
*Ver: Standard .deb
*Pkg:  Debian dpkg interface (Priority 30)
 Pkg:  Debian APT solver interface (Priority -1000)
 Pkg:  Debian APT planner interface (Priority -1000)
 S.L: 'deb' Debian binary tree
 S.L: 'deb-src' Debian source tree
 Idx: Debian Source Index
 Idx: Debian Package Index
 Idx: Debian Translation Index
 Idx: Debian dpkg status file
 Idx: Debian deb file
 Idx: Debian dsc file
 Idx: Debian control file
 Idx: EDSP scenario file
 Idx: EIPP scenario file

@memsharded
Copy link
Member Author

zypper 1.14.43

Ok, that could be a different way, but the key here is to agree if with_apt should return True or False for Suse systems where apt is not apt but a wrapper to zypper. If we keep it returning True, it is problematic for users. If we decide it should be False then we could implement that check too.

@czoido
Copy link
Contributor

czoido commented Apr 5, 2021

zypper 1.14.43

Ok, that could be a different way, but the key here is to agree if with_apt should return True or False for Suse systems where apt is not apt but a wrapper to zypper. If we keep it returning True, it is problematic for users. If we decide it should be False then we could implement that check too.

From my point of view it should return False for Suse because it's not really the expected apt-get program but another one (zypper) which is really called, but I would remove the linux_distro checking for doing that. Just checking the output of the program which is called when running apt-get to decide if with_apt is True or False.

@SSE4
Copy link
Contributor

SSE4 commented Apr 6, 2021

From my point of view it should return False for Suse because it's not really the expected apt-get program but another one (zypper) which is really called, but I would remove the linux_distro checking for doing that. Just checking the output of the program which is called when running apt-get to decide if with_apt is True or False.

totally agree

Copy link
Contributor

@czoido czoido left a comment

Choose a reason for hiding this comment

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

output = check_output_runner([apt_location, 'moo'])
should be changed to:
output = check_output_runner([apt_location, '--version'])
because the output of zypper moo does not contain the string zypper.

@memsharded
Copy link
Member Author

output = check_output_runner([apt_location, '--version'])
because the output of zypper moo does not contain the string zypper.

Oh, right, this is unfortunate, if we are changing this, then there is some risk of breaking. I guess we should look for the apt string in the output for apt --version to consider it valid, but there is some probability that the output of other commands could contain the letter apt, without being apt-get, and I think this is the reason why the apt moo check was implemented in the first place, instead of relying on --version.

@czoido
Copy link
Contributor

czoido commented Apr 6, 2021

I think it should be fine keeping the check that zypper is in the output to return False.
Running apt-get --version with zypper-aptitude installed in OpenSuse returns this string:

$ apt-get --version
zypper 1.14.43

and calling apt-get moo returns this:

$ apt-get moo
   \\\\\
  \\\\\\\__o
__\\\\\\\'/_

Looks like in the original PR there was not special reason to check the existence of apt-get with moo over other arguments so I think it should be ok to change it to --version.

@SSE4
Copy link
Contributor

SSE4 commented Apr 6, 2021

I think it should be fine keeping the check that zypper is in the output to return False.
Running apt-get --version with zypper-aptitude installed in OpenSuse returns this string:

$ apt-get --version
zypper 1.14.43

and calling apt-get moo returns this:

$ apt-get moo
   \\\\\
  \\\\\\\__o
__\\\\\\\'/_

Looks like in the original PR there was not special reason to check the existence of apt-get with moo over other arguments so I think it should be ok to change it to --version.

there was the valid reason - checking apt --version might be too generic, as it could be possible there are another executables called apt could exist (e.g. there is a video game, java tool, programming language and others) and could be available in the path. so it was decided to check for something very specific to the Advanced Packaging Tool, e.g. moo sub-command availability, which is unlikely to be found by another apt executables, but now zypper disproves that.

@czoido
Copy link
Contributor

czoido commented Apr 6, 2021

there was the valid reason - checking apt --version might be too generic, as it could be possible there are another executables called apt could exist (e.g. there is a video game, java tool, programming language and others) and could be available in the path. so it was decided to check for something very specific to the Advanced Packaging Tool, e.g. moo sub-command availability, which is unlikely to be found by another apt executables, but now zypper disproves that.

Oh, I see... Maybe we could check both moo and --version, anyway we are calling apt-get, not apt so I think it should be pretty unlikely that someone had other application called apt-get that is not the packaging tool.

@memsharded
Copy link
Member Author

Too much to check for both moo and --version to see if the output is zypper, because in that case it is not apt-get, with the added risks of running different things. I am adding back the distro based check.

Furthermore, as design rules for Conan codebase:

  • Conan codebase should not check for existing apps via which() and should not check or run apps like apt or similar to check those apps compatibility, interfaces, or support.
  • Conan codebase should implement defaults definitions and mappings, but that are completely dumb. No magic, pure determinism.
  • Conan should provide configuration over those defaults. If we don't want our OpenSuse to use zypper, which will be the default, there could be a [conf] that override that default of the tool to customize it. Without any checks of existence, compatibility or whatsoever. If the user says in OpenSuse it should use Nuget, then it should try to run it, even if not existing or fails.

@memsharded memsharded merged commit ed16166 into conan-io:release/1.35 Apr 7, 2021
@memsharded memsharded deleted the fix/zypper_aptitude_system_package_tool branch April 7, 2021 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants