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

Install build dependencies as specified in PEP 518 #4144

Merged
merged 43 commits into from May 19, 2017

Conversation

@takluyver
Copy link
Member

takluyver commented Nov 29, 2016

Fixes #3691.

PEP 518 describes a way for a source tree (e.g. a VCS checkout or an sdist) to declare build dependencies in a toml file. This is my WIP attempt to support that in pip.

  • Following discussion on #3691, it implements an 'environment lite' system, where packages are installed into a temporary prefix, and Python subprocesses get some minimal isolation.
  • I've vendorised pytoml, which seemed to be the clearest and most actively maintained TOML parser. Switching to another one should be easy if needed.

This change is Reviewable

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Nov 29, 2016

Awesome, I haven't looked at it yet but thanks a lot for taking this on. It will be really great to get this into pip, and that unlocks further future efforts (as you know!).

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Nov 29, 2016

:-)

This is very WIP at the moment - I'm largely opening a PR as a lazy way to see what tests I've broken. But all the conceptual pieces are in place.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Nov 29, 2016

The sysconfig module I'm using is not there on Python 2.6. When are we dropping 2.6? ;-)

Assuming the answer is 'not yet', should I use the undocumented data in distutils.command.install for 2.6, or hardcode a copy of the necessary pieces into pip?

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Nov 29, 2016

pip 9 is the last version of pip to support Python 2.6. That makes pip 10 a version that no longer does. We might end up releasing a 9.1 or so that doesn't drop support for 2.6. So it really depends. You can use 2.7 specific stuff but that means it will take longer before this can be released (since we don't want to release pip 10 so soon after releasing pip 9, normally wait 4-6 months) or you can keep 2.6 compat and this can be released as part of a 9.1.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Nov 29, 2016

Thanks, that's good to know. I think I'll copy in a fallback for 2.6 for now.

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Dec 15, 2016

This is not mandatory to implement this feature, but #4182 has an interesting sub-problem with build time dependencies that may be useful to consider.

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Jan 8, 2017

Bumping to ask the state of this...

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 9, 2017

This is now working, I believe. It's still pretty rough, but I'd like someone more familiar with pip to look over it and tell me what should be done totally differently before I invest too much effort polishing this way of doing it.

I'll de-WIP it to make that clearer.

@takluyver takluyver changed the title WIP: Install build dependencies as specified in PEP 518 Install build dependencies as specified in PEP 518 Jan 9, 2017

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Jan 15, 2017

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Jan 17, 2017

Sorry, I've been sick. I'll take a look at this PR this week.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 17, 2017

No worries; I hope you're recovering nicely!

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Jan 17, 2017

Get well soon! ^.^

@xavfernandez
Copy link
Contributor

xavfernandez left a comment

The source code of tests/data/packages/pep518-3.0.tar.gz should also be provided in https://github.com/pypa/pip/tree/master/tests/data/src

else:
os.environ['PYTHONPATH'] = self.save_pythonpath

rmtree(self.prefix)

This comment has been minimized.

@xavfernandez

xavfernandez Jan 17, 2017

Contributor

Ideally, this could depend on the value of options.no_clean (which could be passed to the context manager's __init__)

This comment has been minimized.

@takluyver

def __exit__(self, exc_type, exc_val, exc_tb):
if self.save_path is None:
os.environ.pop('PATH', None)

This comment has been minimized.

@xavfernandez

xavfernandez Jan 17, 2017

Contributor

del os.environ['PATH'] should work

This comment has been minimized.

@takluyver

takluyver Jan 18, 2017

Member

It should, but if something inside the context removes one of these environment variables, del will fail with a KeyError. For the extra few characters, I prefer the more defensive option. Let me know if you disagree and I'll change it.

This comment has been minimized.

@jaraco

jaraco Jan 25, 2017

Member

I agree with @takluyver here. You wouldn't want this to fail fast if the code in context removed this environment variable (for whatever reason).

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Jan 18, 2017

I added the source of the pep518 sdist for the test.


def _install_build_reqs(self, reqs, prefix):
# Local install to avoid circular import (wheel <-> req_install)
from pip.req.req_install import InstallRequirement

This comment has been minimized.

@mgedmin

mgedmin Jan 26, 2017

s/Local install/Local import/ perhaps?

This comment has been minimized.

@takluyver

takluyver Jan 26, 2017

Member

Well spotted, done. :-)

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Mar 9, 2017

Bump.

Could someone re-run the PyPy build for this PR? It seems to be broken due to something not related to this change.

I guess this is ready to be merged.

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Mar 9, 2017

Looks good to me

@pfmoore

pfmoore approved these changes Mar 9, 2017

@xavfernandez xavfernandez added this to the 10.0 milestone Mar 19, 2017

@brettcannon

This comment has been minimized.

Copy link

brettcannon commented Mar 23, 2017

Is the news-file/pr check the only thing holding this PR up at the moment?

except ImportError:
# This section is for compatibility with Python 2.6, and can be removed
# once Python 2.7 is the minimum supported version.
sysconfig = None

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

2.6 is not supported so you should be fine to just delete this whole bit and depend entirely on sysconfig.

This comment has been minimized.

@takluyver
pp_toml = pytoml.load(f)
return pp_toml.get('build-system', {}).get('requires', [])

return [] # No pyproject.toml

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

If there is no pyproject.toml, can we return ["setuptools", "wheel"] so we can then drop the need to have setuptools/wheel preinstalled for building from sdists? The assumption would then be if someone has a pyproject.toml they are responsible for setting the build-system requires correctly?

It might make sense to gate that on whether or not they have a [build-system] key though rather than just whether the file exists or not, since some projects have already started to use [tool.*].

This comment has been minimized.

@njsmith

njsmith Mar 24, 2017

Member

I think the simplest would be to say that if the requires key is missing, then it defaults to that?

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

Hmm, that is reasonable I suppose. The use case of something that has a contained within itself build system is probably small enough that optimizing for that case where they might have to do requires = [] is probably small enough (I assume that TOML allows an empty list?).

This comment has been minimized.

@njsmith

njsmith Mar 24, 2017

Member

Yeah... Though I'm actually flip flopping a bit mentally. The spec actually says that build-system.requires is mandatory in all pyproject.toml files. It's annoying if people are already creating invalid files; are you sure we can't break them? 😛

The discussion below about isolation reminds us too that we need some trigger for enabling isolated builds. I think the original idea was that either you would have a pyproject.toml and it would have explicit requires and you would get an isolated build, or else you would have none of those. If we have to treat pyproject.toml's that are missing the build-system section as legacy packages, then we need something different.

Of course even if we were able to make build-system.requires mandatory, we'd still have to default on the actual build system key (the one that's not specified yet). This is a late and somewhat wacky idea, but I guess we could say that it's mandatory to have a key build-system = "setuptools", exactly that string, and then later we could redefine it as the package that gets imported to provide the build system API once that's defined...

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

Well, I guess we could pull down the sdists from PyPI and see if anyone is shipping a pyproject.toml to PyPI yet or not. If nobody is actually shipping one to PyPI yet we can possibly just gate on it.

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

One other thing to consider is whether we should automatically add wheel if they list setuptools, since it will almost certainly be an error if they don't.

This comment has been minimized.

@brettcannon

brettcannon Mar 24, 2017

The PEP actually specifies that setuptools and wheel are implied when there's no pyproject.toml: "Because the use of setuptools and wheel are so expansive in the community at the moment, build tools are expected to use the example configuration file above as their default semantics when a pyproject.toml file is not present.".

This comment has been minimized.

@takluyver

takluyver Apr 1, 2017

Member

This will take a bit more working out, because as @njsmith mentioned, the code here currently falls back to a non-isolated build if pyproject.toml is missing. If we just add default packages here without changing anything else, all builds will be isolated by default, which may break a number of packages.

This comment has been minimized.

@dstufft

dstufft Apr 1, 2017

Member

Ah yea that is true, though I think it's also true if a project has a pyproject.toml that has an empty build-system.requires, so we probably need to propagate up whether or not to expect an isolated environment or not.

if sys.version_info >= (2, 7):
mpip = 'pip'
else:
mpip = 'pip.__main__' # Python 2.6 can't execute a package with -m

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

Again, master doesn't support 2.6 anymore, so this can be dropped.

This comment has been minimized.

@takluyver
python_tag=python_tag,
isolate=True)
else:
# Old style build, in the current environment

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

I'd probably not call it old style, since it'd also be legtimately the style that would be used if someone had a completely self contained build system yea?

This comment has been minimized.

@njsmith

njsmith Mar 24, 2017

Member

Ideally a self contained build system would mean building inside an empty clean environment, not the user's current environment.

This comment has been minimized.

@dstufft

dstufft Mar 24, 2017

Member

Well this isn't that either :) If someone explicitly has no build deps it will be inside the current environment.

This comment has been minimized.

@takluyver

takluyver Apr 1, 2017

Member

Updated the comment

@ofek

This comment has been minimized.

Copy link
Contributor

ofek commented Sep 25, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Sep 25, 2017

As I understand it, that is not valid. Only the build-system table has been specified; any other information should be under the tool namespace, arranged by the name of the tool that is specifying it.

Here's the relevant bit of PEP 518:

All other top-level keys and tables are reserved for future use by other PEPs except for the [tool] table. Within that table, tools can have users specify configuration data as long as they use a sub-table within [tool], e.g. the flit tool would store its configuration in [tool.flit].

We need some mechanism to allocate names within the tool.* namespace, to make sure that different projects don't attempt to use the same sub-table and collide. Our rule is that a project can use the subtable tool.$NAME if, and only if, they own the entry for $NAME in the Cheeseshop/PyPI.

@ofek ofek referenced this pull request Sep 25, 2017

Merged

Projectfile #39

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Sep 25, 2017

@Carreau

This comment has been minimized.

Copy link
Contributor

Carreau commented Sep 25, 2017

It might make sense to have PyPI validate a pyproject.toml file (if one exists) on upload to ensure the schema is correct.

That might be an issue is someone make an sdist and a wheel, the wheel get uploaded first, then the sdist get refused because of invalid pyproject.toml, and end up with a sourceless package.

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Sep 26, 2017

That might be an issue is someone make an sdist and a wheel, the wheel get uploaded first, then the sdist get refused because of invalid pyproject.toml, and end up with a sourceless package.

This isn't a new problem, and is essentially an unsolvable problem unless we either mandate that sources are uploaded first (can't / won't) or we remove all validation of a sdist upon upload (can't / won't).

@Carreau

This comment has been minimized.

Copy link
Contributor

Carreau commented Sep 26, 2017

This isn't a new problem, and is essentially an unsolvable problem unless we either mandate that sources are uploaded first (can't / won't) or we remove all validation of a sdist upon upload (can't / won't).

I know you want to also have a "staging area" when things could get uploaded, but not made public first. This could be a place where this is handled. Only allow moving from "staging" to published is all checks are passing.

@ofek

This comment has been minimized.

Copy link
Contributor

ofek commented Sep 26, 2017

So, this might require a separate discussion but... are there any plans to have just 1 file to control Python projects? Like Pipfile and pyproject.toml combined.

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Sep 26, 2017

Pipfile and pyproject.toml are different files because the target audience/purpose is different. The fact they are different is a positive thing.

@ofek

This comment has been minimized.

Copy link
Contributor

ofek commented Sep 26, 2017

Yes I understand they serve different purposes but is there a disadvantage to using just 1 file for all the functionality?

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Sep 26, 2017

Yes, not everything you'd use Pipfile for is a python package, for example Warehouse does not have a setup.py or a pyproject.toml.

@ofek

This comment has been minimized.

Copy link
Contributor

ofek commented Sep 26, 2017

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Sep 26, 2017

Nope, it was a sketch of some ideas not an example of a valid pyproject.toml.

@ofek

This comment has been minimized.

Copy link
Contributor

ofek commented Sep 26, 2017

Is pyproject.toml planned to replace setup.py eventually? Or is it just another file we must now use whose sole purpose is [build-system]?

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Sep 26, 2017

From an author's POV it is meant to replace setup.py yes, but we're not going to dictate how a build tool wants its users to specify information like name or version. A tool like setuptools could put it in pyproject.toml inside of a [tool.setuptools] table, or it could put it somewhere else too. It's up to that specific build tool.

@ofek

This comment has been minimized.

Copy link
Contributor

ofek commented Sep 26, 2017

Thanks for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment