PEP 660 (build_editable) support

[sbidoul]

I wanted to experiment a little bit too with the build_wheel_for_editable concept, just to see how well it fits in the pip code base. I'm sharing in case anyone is interested. This is totally draft and not well tested.

• install --editable
  • backend has build_wheel_for_editable
  • backend does not have build_wheel_for_editable - fallback to setup.py develop
  • backend does not have build_wheel_for_editable - error if no setup.py
  • update to latest PEP 660
  • support get_requires_for_build_editable
  • support prepare_metadata_for_editable
  • fallback if hook not present
  • make sure pip wheel -e does not use any editable hook
• uninstall
• update pip freeze (done in Support PEP 610 editables in pip freeze and pip list #10249)
• update pip list (done in Support PEP 610 editables in pip freeze and pip list #10249)
• in log message Building wheel for * (PEP 517) ... done, mention we are in editable mode?

[dholth]

How does it interact with caching and out of tree builds?

[sbidoul]

How does it interact with caching

Caching should be fine when #8145 is merged. I cherry picked it here for clarity and added a few should_build/should_cache unit tests for the editable+pep517 combination.

and out of tree builds?

That should be ok, since the part that does the copytree is never reached for editable requirements.

[dholth]

Do you have an opinion on passing the target directory to the build so that it can use relative paths to find the real editable module?

[sbidoul]

Do you have an opinion on passing the target directory to the build so that it can use relative paths to find the real editable module?

I'm personally not opposed to it as I'm not too concerned with having a pure backend/frontend interface for editables. But I guess this would quickly be abused by backends to fiddle inside the target directory.

OTOH my view of editable installs is that they are for development only, so in that scenario absolute paths are probably less of an issue?

[dholth]

The 'editable' name or a linked install is better than the 'setup.py develop' name. Packaging provides a feature but shouldn't try to restrict what it is used for.

[sbidoul]

This PR is now somewhat usable. I tested it with a custom backend.

[pfmoore]

Personally, I still prefer the approach where the backend builds a wheel that installs just like any other wheel but the installed code links back to the source directory. This gives editable support with essentially no support code in pip.

Basic implementation is here. It needs fleshing out to make it into a library that backends can use, I plan on doing that when I have more free time.

[dholth]

@pfmoore this could hardly be closer to what you prefer, depending entirely on what the backend decides to put in that wheel.

[pfmoore]

@dholth Whoops, you're right. I think I was getting confused with another one of the proposals. I'm way too tired, and I need to stop posting now, I think.

Apologies for the confusion, and thanks @sbidoul for this! I'll have a proper look when I'm not half asleep 😴

[ofek]

@sbidoul I'm writing a PEP 517 backend for Hatch. Can you explain what build_wheel_for_editable should do?

[pfmoore]

@ofek If you look at editables that's the basic idea - the backend can use that to build a wheel which when installed, will act like an editable installation of the project.

[ofek]

@pfmoore I starred that then forgot 🤦‍♂️ I'll use that, thanks!!!

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

[dholth]

Rudimentary support: https://github.com/dholth/enscons/pull/9/files

[sbidoul]

@dholth cool. I can't seem to make it work though. I've tried an editable install of rsalette in a python 3.8 venv and I get (in verbose mode)

[...]
Getting requirements to build wheel ... done
  Created temporary directory: /tmp/pip-modern-metadata-tbktrjvt
  Running command /home/me/.virtualenvs/tempenv-46f6785219bc/bin/python /home/me/pip/src/pip/_vendor/pep517/_in_process.py prepare_metadata_for_build_wheel /tmp/tmplc9bqnce
  scons: Reading SConscript files ...
  scons: done reading SConscript files.
  scons: Building targets ...
  zip(["/tmp/pip-wheel-32z26ikb/rsalette-0.4.2-ed.py2.py3-none-any.whl"], ["build/wheel/rsalette-0.4.2.dist-info"])
  scons: *** [/tmp/pip-wheel-32z26ikb/rsalette-0.4.2-ed.py2.py3-none-any.whl] TypeError : from_file() got an unexpected keyword argument 'strict_timestamps'
  Traceback (most recent call last):
    File "/tmp/pip-build-env-mpnn1ksd/overlay/lib/python3.8/site-packages/SCons/Action.py", line 1280, in execute
      result = self.execfunction(target=target, source=rsources, env=env)
    File "/tmp/pip-build-env-mpnn1ksd/overlay/lib/python3.8/site-packages/SCons/Tool/zip.py", line 56, in zip
      zf.write(path, os.path.relpath(path, str(env.get('ZIPROOT', ''))))
    File "/usr/lib/python3.8/zipfile.py", line 1741, in write
      zinfo = ZipInfo.from_file(filename, arcname,
  TypeError: from_file() got an unexpected keyword argument 'strict_timestamps'
  scons: building terminated because of errors.
  Building wheel for rsalette (PEP 517) ... done
  Created wheel for rsalette: filename=rsalette-0.4.2-ed.py2.py3-none-any.whl size=22 sha256=8739c76e681f900923b900c9df0ef75cf421d39cabb54650c4b9ad19b6a76d85
  Stored in directory: /tmp/pip-ephem-wheel-cache-24q5wun0/wheels/75/f8/bd/4791c7abdb70831f4b2ce742a4795ad7240c8914367e3a49c1
Successfully built rsalette
[...]

I don't quite understand the unexpected keyword argument 'strict_timestamps' error... it is as if it was running on another python version, although the log says it starts from the python in my venv. And the other issue is that this error is masked, resulting in an invalid wheel (it might be pip not detecting it, not sure yet).

[pradyunsg]

But first I'd like to make sure we are clear with the BuildPurpose thing.
BuildPurpose could be a flag instead of an enum, no problem with that.

Yup yup. I'm largely ambivalent on whether we make it a enum vs bool (although... a bool is cleaner if we don't expect to ever need an additional "purpose").

However I tried to make it a property of InstallRequirement, and it did not feel right (see first commit in this PR).
Basically I had to do this, after parsing the requirements:

53419b6#diff-0e8a4985186a14b2deceeb18b81dc7aa90accb68480ac485e49a8f1803dd0339L124-L128

Yep yup, I think that loop for setting those attributes is fine. I reviewed this commit-by-commit, so I noticed all the changes between the two commits as well. :)

[pradyunsg]

I don't understand your worries:

Phrasing that in terms of supports_pep660 was a mistake on my part. I'm thinking of a project that uses setuptools and has a pyproject.toml:

• 21.2: pip will call setup.py develop directly.
• 21.3: pip will try to create a build environment w/ build-system.requires, check for the hook on setuptools.build_meta, fail and fallback to the setup.py call (due to how the try-except falls through to the legacy case).

This is the "jumping" the boundaries of a pyproject.toml based build system vs setup.py based build system I was referring to. It's also a bunch of additional work that brings absolutely no behavior change for the user except for an additional slowdown.

[sbidoul]

Yup yup. The trouble is that we'll create an entire build environment, make a call to setuptools, note that it's missing a hook, and then fallback to just calling setup.py develop -- basically doing an additional bunch of work, for absolutely no benefit whatsoever.

It's also a bunch of additional work that brings absolutely no behavior change for the user except for an additional slowdown.

Understood. I don't see any alternative though. We can't even do something if we detect a setup.py because we recently started supporting editables with setup.cfg only. And there is no fast mechanism to query build backend capabilities.

I guess at some point we'll have to work on build environment creation performance (with caching or something, I don't know).

[dholth]

If we make PEP 660 faster than the old way, maybe the problem will eventually solve itself.

[sbidoul]

I added a spinner message around legacy metadata preparation

• so the fallback to setup.py egg_info when the backend does not support PEP 660 shows something meaningful (before it was silent and it was confusing to have a log saying we did prepare editable metadata using pyproject.toml)
• for symmetry with pyproject.toml metadata preparation, and it will also help users understanding what happens when metadata preparation is slow

[sbidoul]

@pradyunsg I handled (most of) your review comments :)

[pradyunsg]

I'm a bit sad at how things are gonna work with existing setup.py-based projects, but not sad enough to block this; since they're still going to work. All of my concerns are non-blocking now and we can tackle them separately.

I do think we're taking on a bunch of technical debt along with this improvement (notably, around the tests and around the build logic), but it's definitely more than worthwhile. We do have to figure out how to pay that down though! :)

[pradyunsg]

This is gonna need a rebase and a squashing of the fixup commits.

[pradyunsg]

Wait, where are my manners!? I failed to touch on all the important bits in my get-the-code-stuff-done mode. 😅

I'm genuinely excited that after, uhm... quite a while... we're finally able to fill the hole that we had left in PEP 517 (for good reason, as the length of the discussion for PEP 660 proved!). This PR is the culmination of a lot of work and I'm really glad that we're here now.

@sbidoul I really appreciate you being receptive and responsive to all of my feedback here; even if a bunch of it has seemed a bit odd to you. :)

[sbidoul]

I do think we're taking on a bunch of technical debt along with this improvement

Well, I did not have the impression to make the code a lot harder to read or maintain than it is.
But I remain open to improvements suggestions to make in follow-ups - short of rewriting the full build logic. :)

Rebased and squashed. Thanks again for the review. Feel free to merge when green.

[pfmoore]

I'd also like to say I'm really pleased to see this getting in. Sorry I haven't had time to review the PR, but it's great to see it land.

I have skimmed the PR, and I don't feel the technical debt is that bad - it's mainly around the whole need to support legacy projects, and we're going to have to live with that until setuptools implements PEP 660. Unfortunately, I'm not sure there's a lot of motivation in the setuptools project to implement PEP 660, but that's out of our control.

Thanks @sbidoul for all your work on this!