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

Enabled build-system isolation via pip. #13994

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

apollo13
Copy link
Member

This is described in https://pip.pypa.io/en/stable/reference/pip/#pep-517-and-518-support and enables build system isolation via PEP-517. In practice this means that pip will use an environment where it downloads setuptools and wheel first and as such will not conflict with whatever is active in the current python env. This is a good thing (tm) :D This now allows for installation in virtualenvs that do not have setuptools/wheel but only pip.

The new behavior (compare against master) of a pip install can be observed due to the following output:

  Building wheel for Django (PEP 517) ... done

if the loglevel is increased further one sees that pip now downloads setuptools & wheel.

@felixxm felixxm self-assigned this Feb 10, 2021
@ngnpope
Copy link
Member

ngnpope commented Feb 10, 2021

For reference, using pyproject.toml was last discussed in #12013.
It's been ~14 months - have your opinions changed @carltongibson? (By the time 4.0 is released it'll be ~2 years.)

We should also update the reusable apps documentation - there was vaguely some discussion in #12006 too.

@apollo13
Copy link
Member Author

Just to clarify: To the best of my knowledge pip always builds a wheel nowadays. I just made the minimal changes (that even keep setup.py) to enable build isolation.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

... have your opinions changed @carltongibson?

Hehe. Not really. 😜 I'm a bit Meh about this at best.

But if folks are keen, it's not something I want to plant a flag in the ground about, more that I don't really see the need (having understood it, and looked at the setuptools alternatives).

When presented like this:

This is a good thing (tm) :D

Then what can I say? Sold! 😀

...the minimal changes...

From the linked docs:

Projects with a pyproject.toml file, but which don’t have a build-system section, will be assumed to have the following backend settings...

So I think an empty pyproject.toml would be minimal. 🤔

This (I think) is the source of my Meh: if we're just using setuptools, why declare it? Who exactly is in this imagined virtualenv that doesn't have a recent enough setuptools? Who's using this?

Last time, we declined the pyproject.toml because of the extra wheel building steps for sdist installations. #12013 (comment) — we hadn't managed to state — beyond "(tm)" — what were the benefits?

But, again, not something I'm stuck to. If you all feel that this is important to add then no problem…

@carltongibson carltongibson changed the title Eanbled build-system isolation via pip. Enabled build-system isolation via pip. Feb 10, 2021
@apollo13
Copy link
Member Author

So I think an empty pyproject.toml would be minimal. 🤔

Yes, but I thought it would not hurt to write it out explicitly, so the intentions are clear. Or we'd at least have to have a comment in it.

This (I think) is the source of my Meh: if we're just using setuptools, why declare it? Who exactly is in this imagined virtualenv that doesn't have a recent enough setuptools? Who's using this?

I do not have setuptools in my venv at all. Why should I install it? Django has no runtime dependency on it after all. The only thing I have is pip. Why should I put setuptools into my environment just to install Django -- after the install I can immediately remove it. FWIW it is not just about us doing python setup.py sdist to generate a tarball for release but every pip install of Django that disabled binaries (for whatever reason). Why do we need to force setuptools on those users when it is not required during runtime?

Last time, we declined the pyproject.toml because of the extra wheel building steps for sdist installations. #12013 (comment) — we hadn't managed to state — beyond "(tm)" — what were the benefits?

FWIW if people install the sdist nowdays (I did have to fetch it via the url, because --no-binary would also disable wheel generation), pip does generate wheels even with the current source dists:

➜  Downloads pip install https://files.pythonhosted.org/packages/3b/84/6ed065944dd7ab438e1d19d7bb1b1911b27629946a5e0f8c007a692cc1a7/Django-3.1.6.tar.gz
Collecting https://files.pythonhosted.org/packages/3b/84/6ed065944dd7ab438e1d19d7bb1b1911b27629946a5e0f8c007a692cc1a7/Django-3.1.6.tar.gz
  Using cached Django-3.1.6.tar.gz (9.6 MB)
Requirement already satisfied: asgiref<4,>=3.2.10 in /home/florian/.local/share/virtualenvs/98465d75e1822c5/lib/python3.9/site-packages (from Django==3.1.6) (3.3.1)
Requirement already satisfied: pytz in /home/florian/.local/share/virtualenvs/98465d75e1822c5/lib/python3.9/site-packages (from Django==3.1.6) (2021.1)
Requirement already satisfied: sqlparse>=0.2.2 in /home/florian/.local/share/virtualenvs/98465d75e1822c5/lib/python3.9/site-packages (from Django==3.1.6) (0.4.1)
Building wheels for collected packages: Django
  Building wheel for Django (setup.py) ... done
  Created wheel for Django: filename=Django-3.1.6-py3-none-any.whl size=7834188 sha256=06d44f0a0c11a28e29d23525779ee06bcc171fa2ddacb23e337214d7a8c1e1dd
  Stored in directory: /home/florian/.cache/pip/wheels/fe/b1/e2/935393601036015d3029e937a98ef9bdab66e054b262475638
Successfully built Django
Installing collected packages: Django
Successfully installed Django-3.1.6

beyond "(tm)" — what were the benefits?

It does not clutter a minimal environment with dependencies that are not needed at runtime. In practice it also has the possibility to prevent conflicts with other packages in the environment (granted those are rather slim because everyone is afraid of setuptools anyways :D) -- I am not 100% sure, but probably also with broken setuptools entrypoints in the venv; I did had problems with those in the past.

But, again, not something I'm stuck to. If you all feel that this is important to add then no problem…

Well it is not the end of the world if we don't include it. But why prevent people with a valid virtualenv from installing the Django .tar.gz? I wouldn't argue at all if it would not make any difference, but here we prevent an allowed/valid setup from working.

@felixxm
Copy link
Member

felixxm commented Feb 11, 2021

We should also update the reusable apps documentation - there was vaguely some discussion in #12006 too.

We can do this in a separate PR.

It's been ~14 months - have your opinions changed @carltongibson? (By the time 4.0 is released it'll be ~2 years.)

I've changed my mind 🤷 Now I can see the benefits (pointed out by Florian). For example, installation of Django 3.1.6 crashed for me:

python -m pip install https://www.djangoproject.com/m/releases/3.1./Django-3.1.6.tar.gz
Collecting https://www.djangoproject.com/m/releases/3.1/Django-3.1.6.tar.gz
  Using cached https://www.djangoproject.com/m/releases/3.1/Django-3.1.6.tar.gz (9.6 MB)
Collecting asgiref<4,>=3.2.10
  Using cached asgiref-3.3.1-py3-none-any.whl (19 kB)
Collecting pytz
  Using cached pytz-2021.1-py2.py3-none-any.whl (510 kB)
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.1-py3-none-any.whl (42 kB)
Building wheels for collected packages: Django
  Building wheel for Django (setup.py) ... error
  ERROR: Command errored out with exit status 1:
   command: /tmp/django-pip/bin/python -u -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/tmp/pip-req-build-8sb0wlav/setup.py'"'"'; __file__='"'"'/tmp/pip-req-build-8sb0wlav/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' bdist_wheel -d /tmp/pip-wheel-tlc1z3n7
       cwd: /tmp/pip-req-build-8sb0wlav/
  Complete output (6 lines):
  usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
     or: setup.py --help [cmd1 cmd2 ...]
     or: setup.py --help-commands
     or: setup.py cmd --help
  
  error: invalid command 'bdist_wheel'
  ----------------------------------------
  ERROR: Failed building wheel for Django
  Running setup.py clean for Django
Failed to build Django
Installing collected packages: asgiref, pytz, sqlparse, Django
    Running setup.py install for Django ... done
Successfully installed Django-3.1.6 asgiref-3.3.1 pytz-2021.1 sqlparse-0.4.1

but it went smoothly with pyproject.toml:

Processing Django-3.1.7.tar.gz
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Collecting pytz
  Using cached pytz-2021.1-py2.py3-none-any.whl (510 kB)
Collecting asgiref<4,>=3.2.10
  Using cached asgiref-3.3.1-py3-none-any.whl (19 kB)
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.1-py3-none-any.whl (42 kB)
Building wheels for collected packages: Django
  Building wheel for Django (PEP 517) ... done
  Created wheel for Django: filename=Django-3.1.7-py3-none-any.whl size=7834206 sha256=a2be4af8af7a13016665b9679dd4751147e01269b1a64bf20b8147d173bf0883
  Stored in directory: /pip/wheels/a0/f1/20/887b5a6e9edea5a797413ab142f67fcddab9d6fa811bc7a508
Successfully built Django
Installing collected packages: pytz, asgiref, sqlparse, Django
Successfully installed Django-3.1.7 asgiref-3.3.1 pytz-2021.1 sqlparse-0.4.1

@timgraham
Copy link
Member

For what it's worth, this broke django-cockroachdb's build at the step pip install -e /path/to/django as described in pypa/pip#7953 (comment). I haven't read up on the details to understand the issue, but perhaps this merits a release note to explain what to do if you encounter this problem.

@carltongibson
Copy link
Member

Thanks for the update Tim. I need to have a play to work out exactly what's going on, since we still have the minimal setup.py (so it should™ work) and I'm able to do an editable install locally now (testing that). The weeds get long quickly here, but yes, some advice to the release notes seems reasonable. 🤔

@timgraham
Copy link
Member

I can reproduce this locally adding the --user option:

$ pip install --user -e ~/code/django
Obtaining file:///home/tim/code/django
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Collecting asgiref>=3.3.2
  Downloading asgiref-3.4.0-py3-none-any.whl (25 kB)
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.1-py3-none-any.whl (42 kB)
Requirement already satisfied: pytz in /usr/lib/python3/dist-packages (from Django==4.0.dev20210628094637) (2019.3)
Installing collected packages: asgiref, sqlparse, Django
  Running setup.py develop for Django
    ERROR: Command errored out with exit status 1:
     command: /usr/bin/python3 -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/home/tim/code/django/setup.py'"'"'; __file__='"'"'/home/tim/code/django/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' develop --no-deps --user --prefix=
         cwd: /home/tim/code/django/
    Complete output (28 lines):
    running develop
    WARNING: The user site-packages directory is disabled.
    error: can't create or remove files in install directory
    
    The following error occurred while trying to add or remove files in the
    installation directory:
    
        [Errno 13] Permission denied: '/usr/local/lib/python3.8/dist-packages/test-easy-install-18076.write-test'
    
    The installation directory you specified (via --install-dir, --prefix, or
    the distutils default setting) was:
    
        /usr/local/lib/python3.8/dist-packages/
    
    Perhaps your account does not have write access to this directory?  If the
    installation directory is a system-owned directory, you may need to sign in
    as the administrator or "root" account.  If you do not have administrative
    access to this machine, you may wish to choose a different installation
    directory, preferably one that is listed in your PYTHONPATH environment
    variable.
    
    For information on other options, you may wish to consult the
    documentation at:
    
      https://setuptools.readthedocs.io/en/latest/easy_install.html
    
    Please make the appropriate changes for your system and try again.
    
    ----------------------------------------
ERROR: Command errored out with exit status 1: /usr/bin/python3 -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/home/tim/code/django/setup.py'"'"'; __file__='"'"'/home/tim/code/django/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' develop --no-deps --user --prefix= Check the logs for full command output.

@carltongibson
Copy link
Member

carltongibson commented Jun 29, 2021

Hmmm. This looks setup dependent.

~ $ pip install --user -e ~/Documents/Django-Stack/django
Obtaining file:///Users/carlton/Documents/Django-Stack/django
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Collecting sqlparse>=0.2.2
  Using cached sqlparse-0.4.1-py3-none-any.whl (42 kB)
Requirement already satisfied: pytz in ./.pyenv/versions/3.8.3/lib/python3.8/site-packages (from Django==4.0.dev20210623125341) (2021.1)
Collecting asgiref>=3.3.2
  Downloading asgiref-3.4.0-py3-none-any.whl (25 kB)
Installing collected packages: sqlparse, asgiref, Django
  Running setup.py develop for Django
Successfully installed Django asgiref-3.4.0 sqlparse-0.4.1
~ $ 

@cjerdonek is more knowledgable here 😀 — are we missing something? Is there some comment we could add to guide people who get stuck? (Thanks!)

@apollo13
Copy link
Member Author

Looking at the output of @timgraham

WARNING: The user site-packages directory is disabled.

This is weird, why was it disabled? The rest is just a follow up. If user site-packages are disabled, then it will try to install globally which will fail due to permissions. But this should similarly fail for every other packages as well.

@ngnpope
Copy link
Member

ngnpope commented Jun 29, 2021

Looking at the output of @timgraham

WARNING: The user site-packages directory is disabled.

This is weird, why was it disabled?

This was what I noticed too. What output do you get from the following?

$ python -m site
sys.path = [
    '/home/nick',
    '/usr/lib/python39.zip',
    '/usr/lib/python3.9',
    '/usr/lib/python3.9/lib-dynload',
    '/home/nick/.local/lib/python3.9/site-packages',
    '/usr/lib/python3.9/site-packages',
]
USER_BASE: '/home/nick/.local' (exists)
USER_SITE: '/home/nick/.local/lib/python3.9/site-packages' (exists)
ENABLE_USER_SITE: True

You don't happen to have the PYTHONNOUSERSITE environment variable defined somewhere? See site.ENABLE_USER_SITE for more details. I also noted the following line:

None means it was disabled for security reasons (mismatch between user or group id and effective id) or by an administrator.

So if you see False, you must have some configuration disabling it. If you see None, then maybe the ownership of your ~/.local path is wonky.

@apollo13 apollo13 deleted the buildsystem_isolation branch June 29, 2021 11:14
@timgraham
Copy link
Member

This is on Ubuntu 20.04.

$ python -m site
sys.path = [
    '/home/tim',
    '/usr/lib/python38.zip',
    '/usr/lib/python3.8',
    '/usr/lib/python3.8/lib-dynload',
    '/home/tim/.local/lib/python3.8/site-packages',
    '/usr/local/lib/python3.8/dist-packages',
    '/usr/lib/python3/dist-packages',
]
USER_BASE: '/home/tim/.local' (exists)
USER_SITE: '/home/tim/.local/lib/python3.8/site-packages' (exists)
ENABLE_USER_SITE: True

PYTHONNOUSERSITE isn't set.

@timgraham
Copy link
Member

In the description of pypa/pip#7953, you can see that pip sets PYTHONNOUSERSITE when it runs.

@ngnpope
Copy link
Member

ngnpope commented Jun 29, 2021

Yes, yuck. And I managed to see the issue for myself with this reproducer.

It seems that nobody is interested in fixing this because PEP 517 doesn't officially make provision for editable installs. It looks like there are two competing solutions in the works though, so hopefully in the next 6~12 months we'll have a blessed solution to this problem.

In the meantime, it looks as though this only affects --editable installs with --user (or where --user is assumed because there is no permission to write to global site-packages). Virtual environments seem to be unaffected.

pypa/pip#9990 seems to imply that build isolation should be automatically disabled for editable installs, but, after a quick scan through the source, there is no logic that makes any decision about this other than the --no-build-isolation flag.

The three options open to us seem to be:

  1. The documentation approach -- using --no-build-isolation with pip install --editable --user will work.
  2. Add the following hack to setup.py in Django:
    import site
    import sys
    site.ENABLE_USER_SITE = '--user' in sys.argv[1:]
  3. Revert the change that added build-system isolation until there is a blessed solution for editable installs with PEP 517.

@timgraham
Copy link
Member

Thanks for those proposals, Nick. This discussion can continue on the ticket I created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants