PEP 517 implementation

[pfmoore]

Actual implementation of PEP 517 functionality. This is not complete yet. Notes of outstanding actions:

• Write tests for the new functionality (currently working on getting the existing tests working, to ensure no regressions).
• Implement the --(no-)use-pep517 flag.
• Move the patch to the vendored pep517 package back to the package itself.
• Work out what to do about tests that depend on details of setuptools output.
• Review remaining places where hook calls might be needed.
• Documentation.
• Implement --python-tag support.
• Design review.

Probably others...

[pfmoore]

One immediate question - can anyone explain what test_pep518_with_user_pip in tests/functional/test_install.py is trying to check? As far as I can see, it's attempting to prove that if we run a copy of pip that was installed as --user, then we get that copy of pip. It's not actually related to PEP 518 in any way that I can see. The problem I have is that it's failing because when it installs pip to user-site, setuptools is (now) generating warnings that were apparently not being generated before (or were being suppressed).

There are two independent but related problems here:

1. Relying on setuptools' precise output is fragile (and arguably wrong in a PEP 517 world). We're changing our processing so that we call setuptools via its PEP 517 backend, and if as a result setuptools writes different output, then why is that our concern (as long as pip still does its job)?
2. Stuff going on when pip gets installed as --user is incidental to the actual point of the test (whatever that point is).

I'd like to remove this test, as it's not clear that it has a useful purpose. Failing that, I'd like to ignore the behaviour change in the pip install --user pip_src command (specifically, by setting expect_stderr=True for that call).

[benoit-pierre]

The goal of test_pep518_with_user_pip is to ensure that PEP 518 support works if pip is installed in the user' site: previously this would fail because build isolation was used when running the command to install the build requirement themselves.

[pfmoore]

previously this would fail because build isolation was used when running the command to install the build requirement themselves

Ah, OK. But presumably the requirement in the test is not to install pip in user-site, but rather

1. The site-packages pip (which is what the build isolation environment will see by default) is broken.
2. The user's default environment contains a working pip.

So we don't really care about the details of the install of pip to user-site, other than the fact that it worked? In actual fact, we could probably just set PYTHONPATH to pip's source directory and not do an install at all (although I guess that's fragile in its own way!)

Thanks. I'll add some notes to the test explaining what it's doing, and set expect_stderr=True in the install of pip with a note that we don't so much expect stderr output, as not care whether there is any or not...

[benoit-pierre]

Your latest commit does more than fix test_pep518_with_user_pip.

[pfmoore]

Your latest commit does more than fix test_pep518_with_user_pip.

I know. I intend to reorganise the commits at some point - before I made this into a PR, I was using git commit --amend extremely heavily, just to keep the history looking sane 😢 I'm unfortunately having to use CI to run the tests as they are too slow to run locally, so I'm batching up multiple fixes. I guess I could have just made the commit message "Some more test fixes" or done a commit --amend into the previous commit (that's what I've been doing previously). Would either of those been better for you?

[benoit-pierre]

The real issue with test_pep518_with_user_pip is that it highlights the fact that our build isolation is crap: system site packages should be disabled too...

[pfmoore]

lol, that doesn't surprise me. For now, though, I'll just be glad to get the tests to run at all...

[pradyunsg]

master merges cleanly into this PR and there's cleanups related to FormatControl which should make debugging the --no-binary test failure a little easier + #5455 which makes req_install.py slightly less tedious to navigate.

[pradyunsg]

Break this up into 2 separate functions that get called by this one?

[benoit-pierre]

I think this can be simplified to:

    def get_dist(self):
        """Return a pkg_resources.Distribution for this requirement"""
        if self.metadata_directory:
            metadata_path = self.metadata_directory
        else:
            metadata_path = self.egg_info_path
        metadata_path = metadata_path.rstrip(os.path.sep)
        metadata = pkg_resources.PathMetadata(
            os.path.dirname(metadata_path), metadata_path)
        return pkg_resources.Distribution.from_filename(
            metadata_path, metadata=metadata)

[benoit-pierre]

Or even better:

    def get_dist(self):
        """Return a pkg_resources.Distribution for this requirement"""
        if self.metadata_directory:
            metadata_path = self.metadata_directory
        else:
            metadata_path = self.egg_info_path
        metadata_path = metadata_path.rstrip(os.path.sep)
        return next(pkg_resources.distributions_from_metadata(metadata_path))

[pfmoore]

I'm OK with this - but don't really know the pkg_resources functions well enough to judge the impact. I'll wait for @pradyunsg to do his review, and then I'm happy to just replace the existing get_dist with this one.

On the other hand, it would also be just as easy to put in what we have, and then follow up with a PR simplifying get_dist (in the interest of smaller more focused PRs being better). But as long as making the change doesn't trigger a new wave of weird CI failures, it doesn't really make much difference - this PR is already way too complicated, one more commit won't make much difference 😉

[pradyunsg]

I'm on board for making more follow up PRs for cleanups. This PR is already a little unwieldy in size and it's taken me a lot longer than I estimated to even try it out.

I've bumped existing notes just so that I don't have to scroll too high when taking a pass later to see what places we've already identified for improvements/refactors.

[pradyunsg]

Hmm... If --no-cache-dir is passed, wheel_cache.cache_dir would be None (we can't use ~/.cache etc). That doesn't affect ephemeral cache which is in /tmp.

Yeah, this sounds fine to me and was an oversight on my part when I wrote this.

[pradyunsg]

Considering how long this has become, could this build-the-wheels logic be moved into a method on the command?

[pfmoore]

That's a good idea. I'm focusing on getting the tests to pass right now, but refactoring things is something that definitely needs to be done.

[pradyunsg]

Now would be a nice time to approach refactoring this.

I'm okay if you want to tackle the refactoring in a follow up; tbh I won't mind doing it myself.

[pradyunsg]

Having played around a bit, perhaps we should use a different name here: pip-wheel-metadata stands out... Perhaps a . at the start and perhaps even a sub-folder .pip-metadata/wheel would be a good idea?

Idk. It's a little subjective but I expect this to be something that shows up in development constantly so we should try to optimize the ergonomics here.

[pradyunsg]

Perhaps build/pip-wheel-metadata?

[pradyunsg]

I like how build/pip-wheel-metadata would look here.

[pradyunsg]

error: 'egg_base' must be a directory name (got `t:\pytest-1\popen-gw0\test_pip_wheel_with_pep518_bui0\workspace\tmp\pip-wheel-faaktp\pep518\pip-wheel-metadata`)

Should pip ensure_dir before calling prepare_metadata_for_build_wheel?

[pradyunsg]

This also needs to add support for config settings?

[pfmoore]

Should pip ensure_dir before calling prepare_metadata_for_build_wheel?

Possibly, I'll check. Thanks for the suggestion.

This also needs to add support for config settings?

At this point I've not done anything that changes pip's UI. Adding config_settings support will require design of a command line interface to supply the values, which is something I haven't got to yet. Personally, I'd be happy enough to defer support for config_settings until after the initial implementation is released - the setuptools backend appears to have very primitive (and undocumented) support for it (a single --global-options key, which I guess is supposed to point to a list of setup.py command line arguments?).

There's also pip's --python-tag option to consider - the only place to pass it to the backend is in config_settings, but as config_settings has no standard semantics, we can't do that.

I'll open a separate issue for config settings support in PEP 517. That's probably better, as it allows this PR to progress without getting bogged down in those design questions.

[pfmoore]

#5771 raised for config settings

[pfmoore]

Starting to think about flit as an alternative backend. Some vague (at this point) questions I need to think about and want to keep a note of somewhere:

• We can't copy a flit project to build it, as flit needs a VCS checkout. In-place builds? That's not part of this work.
• Flit sdists have a setuptools shim in there, so we will never actually build a flit project from sdist (it's a setuptools project at that point). Also build-via-sdist won't use the flit backend (not that we care as such)
• We can't easily create flit test cases, because of the VCS requirement, so probably need a dummy backend for non-setuptools testing. Probably a good thing to have, but means we never test that we work with flit. I don't really want the test suite to need git installed (outside of VCS support testing). I also don't want weird test dependencies on users' git config, or complicated stuff to isolate git...

Overall, I suspect that unless flit moves away from its current philosophy of sdists being second-class, and VCS data being key metadata (apologies to the flit project if it's my understanding that's out of date here) I don't think we can usefully treat flit as a typical example of a "non-setuptools" backend. No reason it won't work, but it'll be down to the flit project to make sure it does, pip will follow the standards and shouldn't get distracted by any quirks of flit's particular implementation.

[pfmoore]

Should pip ensure_dir before calling prepare_metadata_for_build_wheel?

Possibly, I'll check. Thanks for the suggestion.

It does - see https://github.com/pypa/pip/pull/5743/files#diff-47a4ece629e4698a5eaefea075d3cbdfR509. Previously, this error has been cause by a bug in the setuptools backend, passing Unicode to a distutils API that requires a str. That's what all the fuss was about needing a new version of setuptools, and the bump in the minimum setuptools version in the code.

The problem here is I've no idea why that would be the case only on Appveyor. I'll need to look into it some more. Hopefully it's not another setuptools bug - I don't like the way this development is so tightly tied to setuptools released versions. If it is a setuptools bug, I'll probably just mark the test as xfail for now (maybe making the xfail conditional on setuptools version when a fix gets released...)

[pfmoore]

Quick status update. I've not worked on this for a couple of weeks now, Real life issues have been taking up most of my time, but I do intend to get back to it as soon as I can. It's not going to be ready for the next release, which is completely fine as far as I'm concerned, but the core functionality is looking good, it's just the wealth of details that need tidying up.

[pfmoore]

Does anyone know why, in https://github.com/pypa/pip/blob/master/src/pip/_internal/cli/base_command.py#L215, we don't pass the command options object into install_req_from_line? I'm adding the --[no-]-use-pep517 option, and it seems really annoying to have to change the signature of the function just to pass a flag that's present in the options that we are in theory passing anyway.

@pradyunsg You were the last person to work on this code (although as far as I can see, all you did was move it in a refactoring). Do you know why this might be?

[pradyunsg]

I don't think I'll be able to take a good look until the end of the week at this.

I'm nearing the end of the semester so all of my procrastination is showing in terms of piled up work. :)

[pfmoore]

lol no worries, I'll probably just pass the options in and see if anything breaks - after all, the whole of this exercise so far has been making sure I've not introduced any regressions, so this is just more of the same. If the tests pass, I'll assume it's OK and move on.

If I get this in, I think I'll be in a position to actually start testing that the new functionality works ;-)

[pfmoore]

Just using options broke it - so I bit the bullet and added a new argument.

[pfmoore]

OK, I believe that's all review comments addressed. @pradyunsg @benoit-pierre @cjerdonek are you aware of anything I've missed? Assuming not, I'm going to go ahead and merge this once CI has passed, and we can address any follow-up fixes that might be needed in separate PRs.

[pradyunsg]

I think there might be a minor logging/code flow issue pointed out here: #5743 (comment)

None the less, it's not a blocker so it can be a follow up if you don't have the time to look into that currently.

[pfmoore]

I think there might be a minor logging/code flow issue

No, you're right, I'd missed that. Fixed now, and thanks for the reminder.

[pfmoore]

OK, merged this. We now have PEP 517 support in pip 😄 If anyone hits any issues with this, ping me in an issue and I'll take a look.

✨ 🎉

[pradyunsg]

Hurrah! Thanks for all the work on this important feature!

[cjerdonek]

Excellent work, @pfmoore! Thanks for all of your patience and hard work! 🏆

[pfmoore]

And to you all for the reviews and comments - I would have burned out on this without the help and support.

After my complaints about the monolithic PR for PEP 518, I feel a bit bad that this was also such a big chunk of change to review. It would be good if we could find a better way of partitioning up big feature changes into more reviewable parts, without leaving master in an unreleasable state. I'll think about what I could have done differently, and if I have any inspirations, I'll post them somewhere :-)

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.