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

Implement PEP 660 allowing both "strict" and "lax/loose" approaches #3265

Merged
merged 55 commits into from Jun 15, 2022

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented Apr 13, 2022

This PR is an attempt to implement editable installs (PEP 660) in such a way that it allows for both “strict” and “loose/lax” editable installations. The overall approach, and the rationale behind which “editable mode” is picked by default is discussed in https://discuss.python.org/t/pep-660-and-setuptools/14855/2.

Summary of changes

  • Starting with the existing PoC (include pep660 proof of concept #3082), re-use bdist_wheel and avoid re-implementing wheel logic
  • Add namespace_packages handling (based on the existing develop command)
  • Add an implementation for the “loose/lax” mode based on static .pth files
    • Target: projects where it is relatively safe to use that approach
      • For example, projects using the src-layout.
  • Add an implementation for the “loose/lax” mode for the projects that cannot be safely handled via .pth files.
    • This implementation is based on a MetaPathFinder and offers the following improvements over the existing implementation (develop command):
      • Capacity to handle more complex package_dir mappings
      • Avoid accidental/unrelated folders and Python files in the project root to be added to sys.path
        • As a trade-off (which seems to be reasonable), this implementation will not automatically pick up new “top-level” modules or packages, but will handle any new file contained by existing package folders.
  • Add an implementation for the “strict” editable mode based on a tree of file links (symlinks are used when available, hardlinks are used otherwise).

(As always) this task ended up being tricker than I was expecting, so I also divided this PR in a series of smaller PRs that can be reviewed independently (but only if the reviews think that reviweing separated PRs can help). Let me know if there is anything different I could do to facilitate the review.

Note, however, that this division was an afterthought, so between “sub-PRs” there might be transient code/changes that are not part of the end result.

Side note

For the sake of not increasing even more the size of the change, I am currently not providing any news fragment or docs for the implementation. Depending on how the feedback/review go, I would provide that in a later stage.

Work towards #2816.

Pull Request Checklist

@abravalheri
Copy link
Contributor Author

abravalheri commented Apr 13, 2022

FAQ about the implementation:

Why is the implementation much more complex than the previous PoCs

This implementation tries to handle the implicit requirement that came up in the previous discussions: allow the user to chose which trade-off makes more sense for the use case in hands. Naturally, supporting multiple strategies instead of only one increases code size/complexity
Moreover, the previous PoCs don’t seem to be able to handle arbitrary mappings in package_dir.

Why editables is not used?

The editables library currently seems to focus in 2 use cases: a static .pth for the top-level package or a strict MetaPathFinder that handles a specific list of modules, (I might be wrong) but neither of which seems to be able to handle the use case I identified as required for this implementation: having a replacement for arbitrary directory symlinks (which are not available on Windows).

There are 2 “asides”: 1) I don’t know if the current implementation in editables for the strict scenario can handle what in setuptools we call package_data, 2) I wanted to avoid the whole debate about “acyclic” build dependency graph…

What happens to data_files, headers, binary extensions, etc?

These files should be available in the installation but are not “editable” (in accordance with the existing spec). There is some discussion starting here: https://discuss.python.org/t/pep-660-and-setuptools/14855/6.

Why a link tree is used for the strict mode instead of a MetaPathFinder?

The link tree allows the correct handling of package_data. (Also much easier to work with).

Which is the default editable mode and why?

Please refer to the discussion in https://discuss.python.org/t/pep-660-and-setuptools/14855/2.

@abravalheri
Copy link
Contributor Author

abravalheri commented Apr 13, 2022

Downsides/short-comings of the proposed implementation

  1. It is fairly complex
  2. It uses wheel and bdist_wheel under the hood, which do not officially provide an stable API.
    The alternative would be duplicating the same logic inside of setuptools, which I would like to avoid…
    Since there is some dialog towards vendoring wheel and/or moving bdist_wheel directly to the setuptools repository, so we comply with the acyclic requirement of PEP 517, it might make sense to endure this non-ideal situation for now…
  3. In the case of flat-layout projects + the “lax/loose” editable mode, new top-level packages/modules will not be automatically picked up (but the nested ones will). This seems to be a reasonable trade-off to avoid having things like setup.py, noxfile.py, docs, tests, notebooks, examples, … accidentally added to sys.path.
  4. The editable install may fail for projects overwriting build_py with a custom class that does not derive from setuptools.command.build_py.build_py (or that considerably change the way this command works internally).

@abravalheri abravalheri marked this pull request as ready for review April 14, 2022 07:41
@abravalheri
Copy link
Contributor Author

abravalheri commented Apr 22, 2022

Updates:

  1. In Make sure namespace packages obtained via import finder handle later additions to path abravalheri/setuptools#10 I changed the MetaPathFinder approach by adding an auxiliary PathEntryFinder, this way implicit namespaces can be support dynamic path computation as required by PEP 420.
    The following discussions might be relevant:

  2. I removed the explicit dependency on Fix bug that prevents packages.find.exclude to take effect when include_package_data=True #3261.

  3. pip is currently investigating providing a UI for config_settings - Add a UI to set config settings for PEP 517 backends pip#11059.
    The syntax supported is --config-settings KEY=VALUE.
    This can be used in the context of editable installs to choose which mode the user prefers for the editable install, for example as:

    pip install -e . --config-settings mode=strict

    Here mode and strict are merely illustrative. It is open for bike shedding and we can choose the words we find more appropriate (e.g. editable-mode=pre-flight).
    Just let me know if there is a clear preference and I will implement it accordingly.

@abravalheri
Copy link
Contributor Author

@jaraco @pganssle do you have any opinion about the approach taken here to implement PEP 660?

@jaraco
Copy link
Member

jaraco commented Jun 7, 2022

Hi Anderson. Thanks for all the hard work on this effort. I'm really excited to see the progress.

Thanks also for providing an overview of the approach and its downsides.

I haven't reviewed the implementation specifically, but based on the description you've provided, it sounds like you've found a fairly robust solution. If you're even moderately happy with it, I'm happy to move forward with it.

@abravalheri
Copy link
Contributor Author

Thank you very much Jason. I will wait a few days to see if Paul has any objection, otherwise I will go ahead and follow a very similar process that I did for PEP 621 and ask for feedback from the community (maybe using a beta version?).

@abravalheri abravalheri changed the base branch from main to feature/pep660 June 15, 2022 13:42
@abravalheri
Copy link
Contributor Author

abravalheri commented Jun 15, 2022

Ok, my plans moving forward:

  1. I will assume that the general approach described/implemented here is OK
  2. I will merge the PR into the feature/pep660 branch and then use this branch for further improvements
  3. I will update the implementation to consider pip's new capacity to specify config_settings.
    My so plan is to allow user to the specify editable-mode. This name is up for bikeshedding.
  4. I will update the implementation to consider the following edge cases:
    • Users overwriting commands (e.g. build_ext)
    • Users specifying sub-commands
  5. I will invite the community to try it out and provide feedback.
  6. I will provide at least a small documentation about the feature.
  7. I will release the feature somehow in an experimental fashion similarly to what was done for PEP 621...1

Footnotes

  1. I don't think pip can allow us to toggle the PEP 660 hook, so I will have to come up with a "creative" solution for that.

This workaround can be reverted when issue 3261 is fixed.
- Add implementation of editable strategy based on a link tree
  - This is the `strict` implementation.
  - Only files are linked, not directories.
    - This approach makes it possible to use harlinks when softlinks are not
      available (e.g. Windows)
    - It also guarantees files that would not be part of the final wheel are
      not available in the editable install.
- Add non-editable files to the produced wheel wheel (e.g. `headers`,
  `scripts`, `data`)
According to the PEP 420, namespace packages need to gracefully handle
later additions to path.

- Use a `PathEntryFinder` + an arbitrary placeholder entry on `sys.path`
  to force `PathFinder` to create a namespace spec.
  - Since `_NamespacePath` and `_NamespaceLoader` are private classes (or
    just documented for comparison purposes), there is no other way to
    implement this behaviour directly [^1].

[^1]: Reimplementing _NamespacePath + a custom logic to maintain
      namespace portions don't have a corresponding path entry also
      seems to have the same end result.
@theo-brown
Copy link

Any idea when this will make it into a release?

@abravalheri
Copy link
Contributor Author

abravalheri commented Jun 24, 2022

Any idea when this will make it into a release?

It will hopefully soon. This was the initial proof of concept to see if the other maintainers agree with the approach.

I am refining the implementation now to make it more appropriate for a release.

Then the plan is to open for the community to test and then an actual release.

@abravalheri
Copy link
Contributor Author

@theo-brown, please feel free to participate of the preliminary round of tests :)

@@ -46,6 +46,8 @@
'prepare_metadata_for_build_wheel',
'build_wheel',
'build_sdist',
'get_requires_for_build_editable',
Copy link
Contributor

Choose a reason for hiding this comment

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

@abravalheri You forgot to add 'prepare_metadata_for_build_editable' to the __all__ list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much @dalcinl. I am addressing this on #3477.

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

Successfully merging this pull request may close these issues.

[FR] Implement PEP 660 -- Editable Installations
5 participants