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

Get rid of which dependency in system_which #2882

Closed
sedrubal opened this issue Sep 23, 2018 · 11 comments
Closed

Get rid of which dependency in system_which #2882

sedrubal opened this issue Sep 23, 2018 · 11 comments
Assignees
Labels
Contributor Candidate The issue has been identified/triaged and contributions are welcomed/encouraged. hacktoberfest Type: Enhancement 💡 This is a feature or enhancement request. Type: Possible Bug This issue describes a possible bug in pipenv.

Comments

@sedrubal
Copy link

sedrubal commented Sep 23, 2018

Issue description

In many minimal system installations (container distributions, etc., see #2277) the non-standard command which is not available by default. But pipenv currently requires this in order to find python and other tools. One could say, that this is a packager fail, but I think, the requirements for pipenv should be minimal and there are many other ways to find executables apart from which.

Expected result

pipenv should work in most plank standard container images out of the box.

Actual result

Pipenv fails and you have to specify the python executable with --python argument.

Steps to replicate
$ docker run --rm -it fedora:28 bash
# dnf install pipenv
# pipenv --three
Warning: the which -a system utility is required for Pipenv to find Python installations properly.
  Please install it.
Warning: the which -a system utility is required for Pipenv to find Python installations properly.
  Please install it.
Warning: the which -a system utility is required for Pipenv to find Python installations properly.
  Please install it.
Warning: Python 3 was not found on your system…
You can specify specific versions of Python with:
  $ pipenv --python path/to/python
When possible, provide the verbose output (`--verbose`), especially for locking and dependencies resolving issues.
Ideas to solve it
  • I've no idea how what we can do on windows. For Linux, these few python lines do more or less the same as which:
def _which(command: str):
      def is_exe(fpath):
          return os.path.isfile(fpath) and os.access(fpath, os.X_OK)

      fpath, _fname = os.path.split(command)
      if fpath:
          if is_exe(command):
              return command
      else:
          for path in os.environ["PATH"].split(os.pathsep):
              exe_file = os.path.join(path, command)
              if is_exe(exe_file):
                  return exe_file

      return None
  • Apart from which, command -v is more standard and available in most shells
  • When --three is given and sys.version_info.major == 3, you could just use sys.executable. The same applies for --two and so on.

$ pipenv --support This is from a `fedora:29` beta docker image, as pipenv in `fedora:28` has no `--support` argument. The same issue is present on fedora 29 as well.

Pipenv version: '2018.7.1'

Pipenv location: '/usr/lib/python3.7/site-packages/pipenv'

Python location: '/usr/bin/python3'

Other Python installations in PATH:

Warning: the which -a system utility is required for Pipenv to find Python installations properly.
Please install it.
[...]
PEP 508 Information:

{'implementation_name': 'cpython',
 'implementation_version': '3.7.0',
 'os_name': 'posix',
 'platform_machine': 'x86_64',
 'platform_python_implementation': 'CPython',
 'platform_release': '4.18.7-300.fc29.x86_64',
 'platform_system': 'Linux',
 'platform_version': '#1 SMP Mon Sep 10 15:37:17 UTC 2018',
 'python_full_version': '3.7.0',
 'python_version': '3.7',
 'sys_platform': 'linux'}

System environment variables:

  • LANG
  • HOSTNAME
  • PWD
  • HOME
  • FBR
  • DISTTAG
  • FGC
  • TERM
  • SHLVL
  • PATH
  • _
  • PYTHONDONTWRITEBYTECODE
  • PIP_PYTHON_PATH

Pipenv–specific environment variables:

Debug–specific environment variables:

  • PATH: /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
  • LANG: en_US.UTF-8
  • PWD: /

Contents of Pipfile ('/Pipfile'):

@techalchemy
Copy link
Member

I agree that we shouldn't use which, but we actually have a reimplementation of this in pythonfinder which we maintain separately and simply haven't implemented yet (example below).

For the second element:

When --three is given and sys.version_info.major == 3, you could just use sys.executable

It's not quite this simple just because when the user has multiple versions of python 3, we prefer the highest stable release (and I think this is actually implemented using pythonfinder already).

import pythonfinder
finder = pythonfinder.Finder(allow_global=True, system=False)  # allow global searching, use system python i.e. sys.executable
command_path = finder.which("some_command")

You should probably verify that this is actually how it works, I am going to cut a release of pythonfinder soon so it'd be good to double check :)

Would absolutely accept a PR on this though, because yeah, definitely agree that the current approach isn't the best

@techalchemy techalchemy added the Type: Enhancement 💡 This is a feature or enhancement request. label Sep 23, 2018
@uranusjr
Copy link
Member

I didn’t check PythonFinder’s implementation, but the implementation provided in the top post does not work on all platforms. Windows, for example, allows you to omit extensions listed in PATHEXT, so python.exe would show up in where python results. I believe there are other platforms with similar behaviours (e.g. Mac OS 9), but they are likely obscure enough we can ignore them for now.

@techalchemy
Copy link
Member

(That was why I mentioned the pythonfinder implementation, which doesn't use the code from the original recommendation but instead uses an OS and platform agnostic approach)

@immerrr
Copy link
Contributor

immerrr commented Oct 15, 2018

Some inspiration could be borrowed from shutil.which function that's part of Py3 standard library, although some bugs are still open against it.

@uranusjr
Copy link
Member

Also (I learnt this only during last week) there is a backport for shutil.which. https://pypi.org/project/whichcraft/

@techalchemy
Copy link
Member

this isn't the first time someone mentioned shutil.which, but there are plenty of reasons why we don't use it.

@sedrubal
Copy link
Author

Oh I didn't know shutil.which. Can you tell me the reasons not to use it? Is there a related issue?

@uranusjr
Copy link
Member

uranusjr commented Oct 18, 2018

@sedrubal Many of the CPython issues linked by @immerrr are very critical for Pipenv, especially those about Windows.

@Eelviny
Copy link

Eelviny commented Jan 27, 2021

I'm encountering this warning when using the new AWS Lambda distributable for Python.
This is a container runtime so I'm using pipenv install within a Dockerfile.
As I'm already explicitly specifying the version I don't have issues but these new container runtimes don't contain which.

@abbasegbeyemi
Copy link

Encountering the same problem here as well. What's the solution?

@matteius matteius added Type: Possible Bug This issue describes a possible bug in pipenv. hacktoberfest Contributor Candidate The issue has been identified/triaged and contributions are welcomed/encouraged. labels Aug 26, 2022
@oz123
Copy link
Contributor

oz123 commented Jan 23, 2023

This should be solved as we no longer use which from vistir or the pipenv which. shutil.which is now used.
Feel free to re-open if you still encounter this issue.

@oz123 oz123 closed this as completed Jan 23, 2023
@oz123 oz123 self-assigned this Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contributor Candidate The issue has been identified/triaged and contributions are welcomed/encouraged. hacktoberfest Type: Enhancement 💡 This is a feature or enhancement request. Type: Possible Bug This issue describes a possible bug in pipenv.
Projects
None yet
Development

No branches or pull requests

8 participants