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

[BUG] salt.modules.pip:is_installed doesn't handle locally installed packages #58202

Closed
jholloway7 opened this issue Aug 14, 2020 · 2 comments · Fixed by #63354
Closed

[BUG] salt.modules.pip:is_installed doesn't handle locally installed packages #58202

jholloway7 opened this issue Aug 14, 2020 · 2 comments · Fixed by #63354
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@jholloway7
Copy link

jholloway7 commented Aug 14, 2020

Description

I'm setting up a fairly complex virtualenv with a few packages (e.g. cdecimal-2.3, internal packages, etc) that aren't published on pypi for whatever reason. I copy the package into the file system, check the hash and pip install it from a local file.

The normal pip syntax for this is something like this:

 $ /var/venv/base/bin/pip install /var/venv/deps/cdecimal-2.3.tar.gz

But the same effect can be achieved through the pip.installed state:

/var/venv/deps/cdecimal-2.3.tar.gz
  pip.installed:
    - pip_bin: /var/venv/base/bin/pip
    - require:
      - virtualenv: /var/venv/base
      - file: /var/venv/deps/cdecimal-2.3.tar.gz

When you run pip freeze --all on this virtualenv, the output syntax of such packages is slightly different than the normal 'frozen' version number:

$ /var/venv/base/bin/pip freeze --all
    ...
    cdecimal @ file:///var/venv/deps/cdecimal-2.3.tar.gz
    celery==3.1.26.post2
    ...

This syntax isn't handled by the pip.is_installed module:

    $ salt-call pip.is_installed cdecimal /var/venv/base
    [ERROR   ] Can't parse line 'cdecimal @ file:///var/venv/deps/cdecimal-2.3.tar.gz'
    local:
        False

The pip.is_installed module is used by other pip states to determine if a given library is installed. Hence, while it doesn't fail any states, it never sees that the locally installed package has been installed and installs it every time.

Setup
Please note that this setup is contrived for simplicity with a readily accessible example. There are other ways to install this particular package using --find-links that works around the problem. For self-maintained dist packages, I haven't really found a good solution that's as straightforward as simply pip installing the dist tarball directly from the file system.

/var/venv/base:
  virtualenv.managed: []
/var/venv/deps/cdecimal-2.3.tar.gz:
  file.managed:
    - source: https://www.bytereef.org/software/mpdecimal/releases/cdecimal-2.3.tar.gz
    - source_hash: d737cbe43ed1f6ad9874fb86c3db1e9bbe20c0c750868fde5be3f379ade83d8b
    - makedirs: True
    - creates: /var/venv/deps/cdecimal-2.3.tar.gz
  pip.installed:
    - pip_bin: /var/venv/base/bin/pip
    - require:
      - virtualenv: /var/venv/base
      - file: /var/venv/deps/cdecimal-2.3.tar.gz

Steps to Reproduce the behavior

    $ salt-call pip.is_installed cdecimal /var/venv/base
    [ERROR   ] Can't parse line 'cdecimal @ file:///var/venv/deps/cdecimal-2.3.tar.gz'
    local:
        False

Expected behavior
For completeness, pip.is_installed could/should parse the pip freeze syntax of "[pkgname] @ file:///..." to determine that a locally installed package is present.

I think it would be as simple as splitting on the first '@', trimming spaces and comparing the first element to the specified package name.

Screenshots
N/A

Versions Report

salt --versions-report
# salt --versions-report
Salt Version:
           Salt: 3001.1

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: 4.3.0
          gitdb: 2.0.3
      gitpython: 2.1.8
         Jinja2: 2.10
        libgit2: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: 3.4.7
         pygit2: Not Installed
         Python: 3.6.9 (default, Jul 17 2020, 12:50:27)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 17.1.2
          smmap: 2.0.3
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: ubuntu 18.04 Bionic Beaver
         locale: UTF-8
        machine: x86_64
        release: 5.3.0-1032-aws
         system: Linux
        version: Ubuntu 18.04 Bionic Beaver

Additional context
N/A

@jholloway7 jholloway7 added the Bug broken, incorrect, or confusing behavior label Aug 14, 2020
@sagetherage sagetherage assigned Ch3LL and unassigned dwoz Sep 23, 2020
@Ch3LL Ch3LL removed their assignment Oct 7, 2020
@Ch3LL Ch3LL added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around and removed needs-triage labels Oct 7, 2020
@Ch3LL Ch3LL added this to the Approved milestone Oct 7, 2020
@elgalu
Copy link

elgalu commented Jun 5, 2021

@jholloway7 how did you proceed? I'm currently a bit fed up with thousands of ERROR ] Can't parse line ... :)

@jholloway7
Copy link
Author

jholloway7 commented Sep 15, 2021

@jholloway7 how did you proceed? I'm currently a bit fed up with thousands of ERROR ] Can't parse line ... :)

Sorry, this got lost in my github notifications or I would have replied sooner. I ended up using an 'unless' in my pip.installed state:

maybe-install-cdecimal:
  pip.installed:
    - name: /var/venv/deps/cdecimal-2.3.tar.gz
    - pip_bin: /var/venv/base/bin/pip
    - user: <user>
    - unless: /var/venv/base/bin/pip freeze | grep cdecimal-2.3
    - require:
      - virtualenv: /var/venv/base
      - file: /var/venv/deps/cdecimal-2.3.tar.gz

It uses the stdout of pip freeze to determine if cdecimal-2.3 has already been installed from a local tarball and skips the state. This probably isn't very robust in other scenarios, but it was a viable workaround in my case for this particular package.

peter-volkov added a commit to peter-volkov/salt that referenced this issue Sep 16, 2021
Conda support 
When pip is a part of Conda envirionment, pip freeze will return something like
```
argon2-cffi @ file:///tmp/build/80754af9/argon2-cffi_1613037097816/work
async-generator @ file:///home/ktietz/src/ci/async_generator_1611927993394/work
attrs @ file:///tmp/build/80754af9/attrs_1620827162558/work
backcall @ file:///home/ktietz/src/ci/backcall_1611930011877/work
bleach @ file:///tmp/build/80754af9/bleach_1628110601003/work
boto3 @ file:///tmp/build/80754af9/boto3_1603939295338/work
botocore @ file:///tmp/build/80754af9/botocore_1603924881626/work
...
```
And then salt will show parsing warnings like
```
[ERROR   ] Can't parse line '_libgcc_mutex=0.1=main'
[ERROR   ] Can't parse line '_openmp_mutex=4.5=1_gnu'
[ERROR   ] Can't parse line 'abseil-cpp=20200923.3=h2531618_0'
[ERROR   ] Can't parse line 'appdirs=1.4.4=py_0'
[ERROR   ] Can't parse line 'argon2-cffi=20.1.0=py38h27cfd23_1'
[ERROR   ] Can't parse line 'arrow-cpp=1.0.1=py38h14434a2_32_cpu'
[ERROR   ] Can't parse line 'async_generator=1.10=pyhd3eb1b0_0'
```
What can be seen in saltstack#58202

The correct way to get package versions in that case is to use command like
```
/opt/conda/bin/conda list --export
```
which return something like
```
 /opt/conda/bin/conda list --export
# This file may be used to create an environment using:
# $ conda create --name <env> --file <this file>
# platform: linux-64
_libgcc_mutex=0.1=main
_openmp_mutex=4.5=1_gnu
abseil-cpp=20200923.3=h2531618_0
appdirs=1.4.4=py_0
argon2-cffi=20.1.0=py38h27cfd23_1
arrow-cpp=1.0.1=py38h14434a2_32_cpu
```

So I can use state like
```
pip-environment:
  pip.installed:
    - bin_env: /opt/conda/bin/pip3
    - freeze_command: "/opt/conda/bin/conda list --export"
    - parallel: True
    - pkgs:
       ...
    - require:
      - conda: conda-environment
```
@peter-volkov peter-volkov mentioned this issue Sep 16, 2021
3 tasks
nicholasmhughes added a commit to nicholasmhughes/salt that referenced this issue Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants