saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Install Salt from the Salt package repositories here:
https://docs.saltproject.io/salt/install-guide/en/latest/
Apache License 2.0
14.21k stars 5.48k forks source link

[BUG] pkg.installed (aptpkg latest_version) UnboundLocalError on foreign-arch package #66940

Open scy opened 1 month ago

scy commented 1 month ago

Description When installing WINE under Debian on an amd64 machine, you usually need to pull i386 packages, too. However, the pkg.installed state seems to struggle with packages only available in foreign architectures, at least when you specify them without their architecture name.

Setup

Steps to Reproduce the behavior On an amd64 machine, try a state like this:

WINE packages:
  cmd.run:
    - unless:
        # Don't run if already enabled.
        - dpkg --print-foreign-architectures | grep -Fqx i386
    - name: dpkg --add-architecture i386 && apt update

  pkg.installed:
    - require:
        - cmd: WINE packages
    - pkgs:
        - fonts-wine
        - libwine
        - libwine:i386
        - wine
        - wine32
        - wine64

This will fail as follows:

          ID: WINE packages
    Function: pkg.installed
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/state.py", line 2428, in call
                  ret = self.states[cdata["full"]](
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/loader/lazy.py", line 160, in __call__
                  ret = self.loader.run(run_func, *args, **kwargs)
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/loader/lazy.py", line 1269, in run
                  return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/loader/lazy.py", line 1284, in _run_as
                  return _func_or_method(*args, **kwargs)
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/loader/lazy.py", line 1317, in wrapper
                  return f(*args, **kwargs)
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/states/pkg.py", line 1934, in installed
                  pkg_ret = __salt__["pkg.install"](
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/loader/lazy.py", line 160, in __call__
                  ret = self.loader.run(run_func, *args, **kwargs)
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/loader/lazy.py", line 1269, in run
                  return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/loader/lazy.py", line 1284, in _run_as
                  return _func_or_method(*args, **kwargs)
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/modules/aptpkg.py", line 819, in install
                  _latest_version = latest_version(
                File "/opt/saltstack/salt/lib/python3.10/site-packages/salt/modules/aptpkg.py", line 507, in latest_version
                  candidates[this_pkg] = candidate
              UnboundLocalError: local variable 'this_pkg' referenced before assignment
     Started: 16:01:22.614404
    Duration: 50.036 ms
     Changes:

The code in question is in aptpkg.latest_version():

    cmd = ["apt-cache", "-q", "policy"]
    cmd.extend(names)
    if repo is not None:
        cmd.extend(repo)
    out = _call_apt(cmd, scope=False)

    short_names = [nom.split(":", maxsplit=1)[0] for nom in names]

    candidates = {}
    for line in salt.utils.itertools.split(out["stdout"], "\n"):
        if line.endswith(":") and line[:-1] in short_names:
            this_pkg = names[short_names.index(line[:-1])]
        elif "Candidate" in line:
            candidate = ""
            comps = line.split()
            if len(comps) >= 2:
                candidate = comps[-1]
                if candidate.lower() == "(none)":
                    candidate = ""
            candidates[this_pkg] = candidate

As you can see, the this_pkg variable is set in the if branch, but referenced in the elif branch. This is at least brittle, if not outright wrong.

The package causing this issue is wine32, since it doesn't exist in the amd64 architecture and thus apt-cache prints its name with an architecture suffix:

# apt-cache -q policy wine32
wine32:i386:
  Installed: 8.0~repack-4
  Candidate: 8.0~repack-4
  Version table:
 *** 8.0~repack-4 500
        500 http://deb.debian.org/debian bookworm/main i386 Packages
        100 /var/lib/dpkg/status

line.endswith(":") and line[:-1] in short_names doesn't match that.

salt-call pkg.latest_version wine32 is also broken, with the same UnboundLocalError.

Changing the line in the YAML from - wine32 to - wine32:i386 allows the state to apply cleanly.

In contrast, salt-call pkg.latest_version wine32:i386 still doesn't work.

Expected behavior Not quite sure actually. Maybe cut off the architecture suffix when parsing the apt-cache output? Or give a warning like package 'wine32' was not found, are you missing an architecture suffix (e.g. 'wine32:i386')??

In any case, the parsing loop should be modified in order to not try and parse Candidate: lines when this_pkg has not yet been set.

Versions Report

salt --versions-report ``` Salt Version: Salt: 3007.1 Python Version: Python: 3.10.14 (main, Apr 3 2024, 21:30:09) [GCC 11.2.0] Dependency Versions: cffi: 1.16.0 cherrypy: 18.8.0 dateutil: 2.8.2 docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 3.1.4 libgit2: Not Installed looseversion: 1.3.0 M2Crypto: Not Installed Mako: Not Installed msgpack: 1.0.7 msgpack-pure: Not Installed mysql-python: Not Installed packaging: 23.1 pycparser: 2.21 pycrypto: Not Installed pycryptodome: 3.19.1 pygit2: Not Installed python-gnupg: 0.5.2 PyYAML: 6.0.1 PyZMQ: 25.1.2 relenv: 0.16.0 smmap: Not Installed timelib: 0.3.0 Tornado: 6.3.3 ZMQ: 4.3.4 Salt Package Information: Package Type: onedir System Versions: dist: debian 12.7 bookworm locale: utf-8 machine: x86_64 release: 6.1.0-25-amd64 system: Linux version: Debian GNU/Linux 12.7 bookworm ```
dmurphy18 commented 1 month ago

@scy Part of the problem may be that Salt doesn't support 32-bit packages on Linux and hasn't for some time.

scy commented 1 month ago

@scy Part of the problem may be that Salt doesn't support 32-bit packages on Linux and hasn't for some time.

I don't think that should keep it from supporting multi-arch package installation in general though. Even if Salt itself does not support 32-bit anymore, Debian still does, and the issue here basically boils down to simple string matching. Also, multi-arch is not just about x86 vs x64, it also applies to different ARM subarchitectures or even cross-platform emulation with things like QEMU and binfmt.

dmurphy18 commented 1 month ago

@scy FYI dropped support for armfh too. With the recent changes in the Salt Core Team, limited resources to address 32-bit items. Of course when Windows moves to a Linux kernel and runs Wine for Windows support, perhaps we will have to look further :rofl: , joking of course. A PR with the fix, changelog and pytest unit tests exercising the code will get this fixed much quicker, otherwise this will be on the TODO list, sorry but limited resources.

scy commented 1 month ago

@scy FYI dropped support for armfh too. With the recent changes in the Salt Core Team, limited resources to address 32-bit items. […] A PR with the fix, changelog and pytest unit tests exercising the code will get this fixed much quicker, otherwise this will be on the TODO list, sorry but limited resources.

That's alright and understandable, all good. :) I have outlined a simple workaround in the original report above; this is therefore probably a low priority issue.

However, I still think we're somehow miscommunicating here. This issue has nothing to do with running Salt on 32-bit machines. Instead, this is about Debian's APT package manager being able to install 32-bit packages on 64-bit machines, which are completely unrelated to Salt's supported architectures. In fact, I could even install ARM packages on my AMD64 machine and run them transparently with QEMU, Linux supports these kinds of shenanigans. This issue is just about Salt not stumbling over cross-architecture packages that I wish to install.

dmurphy18 commented 1 month ago

@scy Understand, have done all manner of crazy things on top of Linux, and older arches too (even used QEMU in a past life) What you want is a corner case, hence it would be on TODO, with the few of us left, living in fire-fighting mode is taking a lot of the time allotted.