Add PWD to sys.path as part of the PEP 517 build

[fchorney]

in reference to this: pypa/pip#6163

[pganssle]

I am amenable to this change, but I think if we start messing around with sys.path in the build hooks, I don't see a good way to switch that behavior back - we won't necessarily be able to detect that someone is counting on this, and so we cannot give any sort of deprecation warning about it.

Given that you need to opt in to using PEP 517, we have an opportunity now to decide what we want the semantics to be, rather than trying to maintain the existing semantics. What are the downsides of requiring that setup.py be more explicit about where it's importing from? I suppose adding to the difficulty of migration is one.

[h-vetinari]

Given that you need to opt in to using PEP 517

That's not the case, hence the swift response to the pip-release, see https://github.com/pypa/pip/labels/C%3A%20PEP%20517%20impact
Rather, it is opt-out with --no-use-pep517.

[pganssle]

Maybe I'm wrong, but this should only be affecting projects that have added build-backend into their pyproject.toml, which greatly limits the scope of this problem (though the fact that there's a bunch of stuff in the wild getting burned is concerning). If pip started using setuptools.build_meta as the backend by default then I agree it's not opt-in.

[fchorney]

I’ll note that this affects any project with a pyproject.toml, regardless if there is a build-backend.

For example, I have a project that uses a pyproject.toml as a config for python black, and this affects that project.

[altendky]

https://github.com/altendky/tempexample/tree/82bd123917bedce24b7e19acbfc4a289f681ca28

There's a pretty minimal example showing the success without pyproject.toml and failure with it.

Click to expand log

python3 -m venv venv
+ python3 -m venv venv
venv/bin/python -m pip install --upgrade pip==19.0.1 setuptools==40.6.3
+ venv/bin/python -m pip install --upgrade pip==19.0.1 setuptools==40.6.3
Collecting pip==19.0.1
  Using cached https://files.pythonhosted.org/packages/46/dc/7fd5df840efb3e56c8b4f768793a237ec4ee59891959d6a215d63f727023/pip-19.0.1-py2.py3-none-any.whl
Collecting setuptools==40.6.3
  Using cached https://files.pythonhosted.org/packages/37/06/754589caf971b0d2d48f151c2586f62902d93dc908e2fd9b9b9f6aa3c9dd/setuptools-40.6.3-py2.py3-none-any.whl
Installing collected packages: pip, setuptools
  Found existing installation: pip 18.1
    Uninstalling pip-18.1:
      Successfully uninstalled pip-18.1
  Found existing installation: setuptools 40.6.2
    Uninstalling setuptools-40.6.2:
      Successfully uninstalled setuptools-40.6.2
Successfully installed pip-19.0.1 setuptools-40.6.3
rm pyproject.toml
+ rm pyproject.toml
venv/bin/python -m pip install --ignore-installed .
+ venv/bin/python -m pip install --ignore-installed .
Processing /home/altendky/tempexample
Installing collected packages: pkg
  Running setup.py install for pkg ... done
Successfully installed pkg-0.0.0
touch pyproject.toml
+ touch pyproject.toml
venv/bin/python -m pip install --ignore-installed .
+ venv/bin/python -m pip install --ignore-installed .
Processing /home/altendky/tempexample
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  Complete output from command /home/altendky/tempexample/venv/bin/python /home/altendky/tempexample/venv/lib/python3.7/site-packages/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /tmp/tmptq1h87o9:
  Traceback (most recent call last):
    File "/home/altendky/tempexample/venv/lib/python3.7/site-packages/pip/_vendor/pep517/_in_process.py", line 207, in <module>
      main()
    File "/home/altendky/tempexample/venv/lib/python3.7/site-packages/pip/_vendor/pep517/_in_process.py", line 197, in main
      json_out['return_val'] = hook(**hook_input['kwargs'])
    File "/home/altendky/tempexample/venv/lib/python3.7/site-packages/pip/_vendor/pep517/_in_process.py", line 54, in get_requires_for_build_wheel
      return hook(config_settings)
    File "/tmp/pip-build-env-3pas05w4/overlay/lib/python3.7/site-packages/setuptools/build_meta.py", line 115, in get_requires_for_build_wheel
      return _get_build_requires(config_settings, requirements=['wheel'])
    File "/tmp/pip-build-env-3pas05w4/overlay/lib/python3.7/site-packages/setuptools/build_meta.py", line 101, in _get_build_requires
      _run_setup()
    File "/tmp/pip-build-env-3pas05w4/overlay/lib/python3.7/site-packages/setuptools/build_meta.py", line 85, in _run_setup
      exec(compile(code, __file__, 'exec'), locals())
    File "setup.py", line 3, in <module>
      import pkg
  ModuleNotFoundError: No module named 'pkg'
  
  ----------------------------------------
Command "/home/altendky/tempexample/venv/bin/python /home/altendky/tempexample/venv/lib/python3.7/site-packages/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /tmp/tmptq1h87o9" failed with error code 1 in /tmp/pip-req-build-3ek7dus1

[uranusjr]

For reference—here’s the thread discussing how a pyproject.toml missing build-backend.requires is handled. pyproject.toml is treated as the indicator whether a project wishes to use PEP 517; if a project supplies that file, the maintainers should be responsible to ensure it works with isolated builds.

https://mail.python.org/archives/list/[email protected]/message/4QWJJAPLH7HECEMOPUFVFX35VVR5MJ4L/

[pganssle]

Hm, if pip is defaulting to the setuptools backend, then the opportunity is not as great as I thought, I do think we basically have to fix this. If anyone wants to take a first crack at this I'd be happy to review it. Preferably we want to make the absolute minimum change that preserves something like the existing semantics.

I also do not want to break anyone who has deliberately removed PWD from their sys.path.

[uranusjr]

The old code path (before PEP 517) in pip chdir to the project root before running setup.py, so in most cases people have pwd in path simply because sys.path contains . by default. Maybe it’d be reasonable to do the same chdir in the build hook? This would only break in cases where people run the PEP 517 tool on its own (not via pip), which should be fairly unlikely given the relatively recent support.

I’ll try to find some time to come up with something today.

[uranusjr]

While working on this, I realise I am personally against the practice of importing the module in setup.py. This kind of runtime-dependent setup is fundamentally at odds with the recent move to declarative build systems, and is a very common source of breakages in environment-agnostic tools (e.g. Pipenv). I kind of wish this can be an opportunity to push people away from this.

[mig4]

@uranusjr I agree, I think it's a bad practice to import the module being installed from its setup.py. The PyPA guide on Single Sourcing Package Version has a couple of recommendations on a better way of handling this requirement.

PEP 517's Build Environment section specifies nothing about an environment should be assumed except that the dependencies declared in pyproject.toml are available, so I'm not sure if this is a bug in pip/setuptools but rather a change that exposes bugs in various projects' setup.py files.

[pganssle]

While working on this, I realise I am personally against the practice of importing the module in setup.py. This kind of runtime-dependent setup is fundamentally at odds with the recent move to declarative build systems, and is a very common source of breakages in environment-agnostic tools (e.g. Pipenv). I kind of wish this can be an opportunity to push people away from this.

Yeah, this was my thinking when I thought PEP 517 was opt-in, but at this point I think we are stuck with the existing semantics at least for a while, unless we can switch PEP 517 into being opt-in. If anyone can think of a way to detect this reliance so that we can deprecate it (possibly some sort of import hook?), I'd like that.

That said, this is also a problem for things other than importing the module you're trying to install. People may have build scripts that live alongside setup.py that they import, or in build_scripts/__init__.py.

Maybe it’d be reasonable to do the same chdir in the build hook? This would only break in cases where people run the PEP 517 tool on its own (not via pip), which should be fairly unlikely given the relatively recent support.

If this is the case, I'd prefer to solve it with chdir for sure. That neatly solves the problem of "what if people are trying to take steps to actively remove . from their sys.path". Also, people may be relying on the fact that PWD is the directory setup.py is in in other ways, like opening files with relative paths.

[uranusjr]

I’ve submitted a PR in #1643. A few observations:

First of all, the build backend is already run in the “correct” directory, i.e. where setup.py resides, so code reading files based on relative paths should work as-is. The problem is, the backend is invoked without ''' in sys.path, contrast to a regular, direct Python invocation. Here’s a common sys.path value:

$ python -c "import sys; [print(repr(p)) for p in sys.path]"
''
'/<python-prefix>/lib/python37.zip'
'/<python-prefix>/lib/python3.7'
'/<python-prefix>/lib-dynload'
'/<python-prefix>/lib/python3.7/site-packages'

But here’s what you get in a pip-invoked PEP 517 backend:

'/<python-prefix>/lib/python37.zip'
'/<python-prefix>/lib/python3.7'
'/<python-prefix>/lib-dynload'
'/<python-prefix>/lib/python3.7/site-packages'

This is why the to-be-installed module cannot be imported in setup.py.

I did not read pip’s PEP 517 implementation, but this is likely intentional, and with a good reason (it makes sense intuitively—isolation). For Setuptools specifically, however, I agree that we’re probably stuck with the current semantics.

The proposed change in the submitted PR only patches the PEP 517 interface to mimic the common sys.path configuration. If someone is relying on removing CWD from sys.path, it is likely done in setup.py (is there even another way?). The sys.path patch happens before setup.py is run, so setup.py still has a chance to remove it if the maintainer so wishes. Hopefully that is reasonable.

[pganssle]

@uranusjr Yes, that makes sense.

The discussion is now happening across two tickets, which is kinda unfortunate, but in the pip ticket, I have suggested that we add a setuptools.build_meta_legacy, which would basically be a version of setuptools.build_meta whose contract is that it attempts to do exactly the same thing as invoking setup.py in the local directory would do.

If we can get pip to change the default backend to build_meta_legacy, that should both allow us to retain the isolated semantics for people who have opted in and preserve the existing semantics for people who just happened to have a pyproject.toml. Let's see how that goes over in pip before proceeding.

[uranusjr]

Cross-posting from the pip issue: I also implemented this as a separate package so package maintainers can apply the work around immediately.

https://pypi.org/project/setuptools-localimport/

[jaraco]

Maybe I'm wrong, but this should only be affecting projects that have added build-backend into their pyproject.toml.

It isn't affecting all projects that have added build-backend to their pyproject.toml either. I have a whole suite of packages declaring build-backend and their test suites are running fine.

[techalchemy]

@jaraco it is only affecting projects that also import their own packages in setup.py or in the case of pendulum, have lines like from build import *.

Pendulum has a directory structure that looks like:

pendulum/
|-setup.py
|-build.py
|-pyproject.toml
|-pendulum/

However, at runtime, sys.path has the path to the vendored pep517 library at its head, which itself has a build.py and which therefore gets imported instead of the actual target. I dropped in a print to show what sys.path looks like in _run_setup() in setuptools/build_meta.py (which only works if you use --no-build-isolation) and what it looks like in the line right above the pep517 invocation in pip and the first executed line in the vendored library:

$ pip install pendulum==1.5.1
['/home/hawk/.virtualenvs/tempenv-734a94685113/bin', '/home/hawk/.virtualenvs/tempenv-734a94685113/lib/python37.zip', '/home/hawk/.virtualenvs/tempenv-734a94685113/lib/python3.7', '/home/hawk/.virtualenvs/tempenv-734a94685113/lib/python3.7/lib-dynload', '/home/hawk/.pyenv/versions/3.7.1/lib/python3.7', '/home/hawk/.virtualenvs/tempenv-734a94685113/lib/python3.7/site-packages', '/home/hawk/git/pip/src', '/home/hawk/git/setuptools']
  Getting requirements to build wheel ... error
...
Complete output from command /home/hawk/.virtualenvs/tempenv-734a94685113/bin/python /home/hawk/git/pip/src/pip/_vendor/pep517/_in_process.py get_requires_for_build_wheel /tmp/tmpy5g2z_86:
  ['/home/hawk/git/pip/src/pip/_vendor/pep517', '/tmp/pip-build-env-ufcjvf_c/site', '/home/hawk/.virtualenvs/tempenv-734a94685113/lib/python37.zip', '/home/hawk/.virtualenvs/tempenv-734a94685113/lib/python3.7', '/home/hawk/.virtualenvs/tempenv-734a94685113/lib/python3.7/lib-dynload', '/home/hawk/.pyenv/versions/3.7.1/lib/python3.7', '/tmp/pip-build-env-ufcjvf_c/overlay/lib/python3.7/site-packages', '/tmp/pip-build-env-ufcjvf_c/normal/lib/python3.7/site-packages']

If you check sys.path at this point: https://github.com/pypa/pip/blob/e5f4bbb7ddff87f48f2b5815513e4211ccdde83a/src/pip/_internal/operations/prepare.py#L149

And then again in the called function: https://github.com/pypa/pip/blob/e5f4bbb7ddff87f48f2b5815513e4211ccdde83a/src/pip/_vendor/pep517/_in_process.py#L48

You'll notice that the parent directory of the _in_process.py is at the head of sys.path. This makes sense because this is invoked by the pep517 build process: https://github.com/pypa/pip/blob/e5f4bbb7ddff87f48f2b5815513e4211ccdde83a/src/pip/_vendor/pep517/wrappers.py#L152-L156

All of this is to say, when setuptools is invoked it is invoked from the right directory but is missing a path entry for the cwd. I don't think there is any real value in discussing if it's 'good' for people to import their own library in setup.py because I think we all know it's bad, but people do it anyway. Specifically if this doesn't get addressed it renders old packages that did do this completely broken unless they circumvent pep517 style builds.

The simple fix is to just add "" to the head of sys.path if it's missing.

[pganssle]

I think at this point we should go with the build_meta_legacy concept so we can at least tell the difference between those who haven't added PEP517 and those who have.

But yeah, in build_meta_legacy we'll prepend '' to sys.path.

[techalchemy]

That seems good to me -- just to clarify -- (this may be rehashing what's been discussed) does that mean independently determining if the project has enabled pep517 even if pip has called for a pep517 build?

[pganssle]

No we won't be messing around with the PEP517 build, just providing two backends: the "legacy" backend, which will attempt to maintain the semantics of invoking python setup.py, and the primary backend, which may not. pip can switch the default PEP 517 backend to build_meta_legacy.

[techalchemy]

so for projects where a pyproject.toml is present and pip is making assumptions as a result, pip will need to modify the behavior from assuming it should attempt to use a pep517 build by default in this case?

[uranusjr]

Does this mean that ’’ is intentionally excluded from sys.path, and will continue to be? If so, it is probably worthy the while to update the PEP to explicitly say it. I mention this since it occurred to me that the legacy backend solution does not immediately fix PyInstaller—they do specify a build backend (and so are not affected by the legacy backend change), but still import a local module. Also they seem to not consider it a problem, from what I read their response to the issue reports.

[pradyunsg]

(on mobile) My understanding of @pganssle's proposal is:

When there's a pyproject.toml without a build-backend, it'll result in a build with the "legacy" backend, with '' in sys.path (and any other compatibility quirks needed). The current PEP 517 backend, will stay as is (with '' not in sys.path), but won't be pip's default backend. It'll be the primary backend pushed for in setuptools documentation IIUC. I'm not sure what the plan is for the legacy variant but I guess we'll seem ;)

This needs a pip update and addition of the legacy backend in setuptools. I don't think this needs a PEP update since a different backend could choose different tradeoffs - in this case setuptools itself providing 2 backends.

The current opt-in to the lack of a '' in sys.path is the presence of pyproject.toml. The future opt-in would be explicitly setting the build-backend to setuptools.build_meta. That's a much cleaner UX. (I still prefer to require explicitly set build-system in pyproject.toml but that ship has sailed)

Fun Fact: My phone autosuggests "setuptools.build_meta" on typing "setupt"

[pradyunsg]

FYI - most of the discussion is happening on pypa/pip#6163 and let's keep the discussion on pip's tracker in the interest of keeping discussions in one place.

[pganssle]

Closed in #1652