-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Build local directories in-place with feature flag #9091
Conversation
@davidhewitt thanks a lot for this PR! There are couple things that remain to be done:
|
@sbidoul @pradyunsg thanks for the reviews, I've made the suggested changes. I recycled the documentation mostly from #7882 with a bit of re-wording. R.e. cleanup - #7882 had a lot of removals of things to do with copying the source tree to a temporary location. I think that's most the cleanup that will occur in the deprecation-removal step. Sadly I don't think it's possible to do any of that cleanup beforehand. I've left a note about cleanup in a comment as suggested. |
"your packages with this new behavior before it becomes the " | ||
"default.\n", | ||
replacement=None, | ||
gone_in=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should set a timeline for this -- let's go with pip 21.2 (Q2 2021)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this warning may go unnoticed in CI runs, and the change may be somewhat non-trivial where it actually causes issues, we may want a 3 steps deprecation:
- warn on copytree by default, encourage users to enable the in-tree build feature (this PR)
- error on copytree by default, allow users to use out-of-tree build with
--enable-legacy
(so people who did not notice still have a cycle to adapt) - remove copytree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general idea with the feature flags is to "allow early adoption and late adoption, while not creating busy-work for folks who aren't affected".
Erroring out by default would create such busy-work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pradyunsg one of the show stoppers that led to the revert of #7882 was #8165 (comment) (the change silently producing invalid build artifacts).
So my worry is that people will not notice the warning and wrong builds will happen silently when we flip the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbidoul Am I correctly understanding the progression that you're suggesting - there will be a point (step 2) where running pip without any options will cause an error, and the only way to get pip to run will be to explicitly state whether or not you want in-place or out-of-tree builds?
That seems like an extremely heavy handed way of protecting a relatively small number of (admittedly quite serious) failure cases. I think at some point we have to acknowledge that it's the responsibility of the projects to spot and act on deprecation warnings, or deal with the consequences...
(On the other hand, if I've misunderstood the steps in your proposed process, please could you clarify?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A possibly (reduced) step 2 could be to just error-out when building wheels (i.e. not on pip install local_dir
), as that's the case which is most likely to have wider repercussions as well as more likely to affect project maintainers than users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea: could pip
return a non-zero exit code if any deprecation warnings are emitted (perhaps with a flag to disable that behaviour)?
Might be a way to get more people to notice a deprecation warning in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfmoore I agree it is heavy handed. Maybe I worry too much after the revert and that would indeed create busy-work for a majority that would not be impacted by the change. Hard to tell as always... we at least have the indication that the world did not collapse during the couple of weeks 7882 was released. The alternative is good communication around that particular deprecation, which might have been the main missing part with the previous attempt.
@davidhewitt the "exit code on deprecation" idea is interesting and might be worth investigating for the future. I guess we'd need to do some kind of inventory of past and current deprecations to see how that would play out for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think allowing affected users to opt-out (when the default changes) would be sufficient to prevent this from being a showstopper TBH -- it changes "pip now breaks your workflow and your only option is to pin to an older version" to "pip has a new default that breaks my workflow. You can re-enable the old behavior, but it is now deprecated and due for removal soon.".
Notice that folks who aren't affected get the new behavior (which is good!) and anyone who is affected doesn't break immediately without an escape hatch (which is nice!). Affected users are also nudged to transition away because it's now a deprecated feature (which is also nice!). :)
Thanks, I've addressed the nit and updated docs & deprecation warning to state the behaviour change will occur in pip 21.2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidhewitt looks good. Tested and confirmed it works as expected. Thanks!
Nice! Also nit addressed! 🚀 |
@pypa/pip-committers shall we go ahead and merge this for 21.0 ? |
It would be great if this could be merged. Is there anything holding it up? |
Let’s do this? |
It's missed 21.0 sadly, but let's get this in for 21.1. |
docs/html/reference/pip_install.rst
Outdated
@@ -809,7 +809,15 @@ You can install local projects by specifying the project path to pip: | |||
During regular installation, pip will copy the entire project directory to a | |||
temporary location and install from there. The exception is that pip will | |||
exclude .tox and .nox directories present in the top level of the project from | |||
being copied. | |||
being copied. This approach is the cause of several performance and correctness | |||
issues, so it is planned that pip 21.2 will change to install directly from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we missed 21.0, is it still correct to say 21.2 here?
news/7555.feature.rst
Outdated
@@ -0,0 +1,5 @@ | |||
Add a feature ``--use-feature=in-tree-build`` to build local projects in-place | |||
when installing. This is expected to become the default behavior in pip 21.2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and here
I can rebase and push with messages for 21.3 tonight if desired? |
Sounds good to me. |
Just wondering if access to this feature should be OK for a key in pyproject.toml |
Ooooh, I never thought about that, but this indeed can be a project-level toggle for maintainers to flip. That should need a PEP though. |
since its pip specific i wonder if it wouldnt make sense as option under tool.pip |
This applies not only to pip, but any tools that download sdists and build them. Most of these tools currently are using pip for that, which is not a good thing, and we shouldn’t make their possibility to switch away from pip more difficult by introducing this pip-specific flag. |
I'm also not comfortable with configuring pip via pyproject.toml as it would tie projects to a specific installer. And I'm personally convinced this copy-to-temp-dir-before-building-to-work-around-backend-quirks feature of pip can't be made correct and must disappear in some future, to be handled by other parts of the ecosystem (build backends preferably, or else the user who knows what to copy before building). At some point I thought about enabling that feature as part of PEP 517, but now I think this should be handled entirely in the backend which should be responsible for building correctly whatever happened previously in the source directory.
When downloading sdists pip already builds "in place" after unpacking the sdist in a temporary location. So this applies only to builds from local directories, correct? |
Right, I meant to write “source” not sdist, sorry. (I was thinking about VCS.) |
Rebased and updated deprecation version number to 21.3. |
... I don't understand that lint failure at all? 😕 |
@pradyunsg Something's acting up in vendoring. |
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 |
Not in vendoring:
I feel like it might be an idea to move those to be separate steps in the pipeline? |
Thanks. Is that docs issue something that will fix itself on a rebase, or something that I need to fix? |
news/7555.feature.rst
Outdated
Add a feature ``--use-feature=in-tree-build`` to build local projects in-place | ||
when installing. This is expected to become the default behavior in pip 21.3; | ||
see `Installing from local packages | ||
<https://pip.pypa.io/en/stable/user_guide/#installing-from-local-packages>` for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the cause; you need to add __
(double underscore) after the closing backtick.
Ahh figured it out, it was the line break in the link. New macos failure looks flaky. |
Bumps [pip](https://github.com/pypa/pip) from 21.0.1 to 21.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's changelog</a>.</em></p> <blockquote> <h1>21.1 (2021-04-24)</h1> <h2>Process</h2> <ul> <li>Start installation scheme migration from <code>distutils</code> to <code>sysconfig</code>. A warning is implemented to detect differences between the two implementations to encourage user reports, so we can avoid breakages before they happen.</li> </ul> <h2>Features</h2> <ul> <li>Add the ability for the new resolver to process URL constraints. (<code>[#8253](pypa/pip#8253) <https://github.com/pypa/pip/issues/8253></code>_)</li> <li>Add a feature <code>--use-feature=in-tree-build</code> to build local projects in-place when installing. This is expected to become the default behavior in pip 21.3; see <code>Installing from local packages <https://pip.pypa.io/en/stable/user_guide/#installing-from-local-packages></code>_ for more information. (<code>[#9091](pypa/pip#9091) <https://github.com/pypa/pip/issues/9091></code>_)</li> <li>Bring back the "(from versions: ...)" message, that was shown on resolution failures. (<code>[#9139](pypa/pip#9139) <https://github.com/pypa/pip/issues/9139></code>_)</li> <li>Add support for editable installs for project with only setup.cfg files. (<code>[#9547](pypa/pip#9547) <https://github.com/pypa/pip/issues/9547></code>_)</li> <li>Improve performance when picking the best file from indexes during <code>pip install</code>. (<code>[#9748](pypa/pip#9748) <https://github.com/pypa/pip/issues/9748></code>_)</li> <li>Warn instead of erroring out when doing a PEP 517 build in presence of <code>--build-option</code>. Warn when doing a PEP 517 build in presence of <code>--global-option</code>. (<code>[#9774](pypa/pip#9774) <https://github.com/pypa/pip/issues/9774></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>Fixed <code>--target</code> to work with <code>--editable</code> installs. (<code>[#4390](pypa/pip#4390) <https://github.com/pypa/pip/issues/4390></code>_)</li> <li>Add a warning, discouraging the usage of pip as root, outside a virtual environment. (<code>[#6409](pypa/pip#6409) <https://github.com/pypa/pip/issues/6409></code>_)</li> <li>Ignore <code>.dist-info</code> directories if the stem is not a valid Python distribution name, so they don't show up in e.g. <code>pip freeze</code>. (<code>[#7269](pypa/pip#7269) <https://github.com/pypa/pip/issues/7269></code>_)</li> <li>Only query the keyring for URLs that actually trigger error 401. This prevents an unnecessary keyring unlock prompt on every pip install invocation (even with default index URL which is not password protected). (<code>[#8090](pypa/pip#8090) <https://github.com/pypa/pip/issues/8090></code>_)</li> <li>Prevent packages already-installed alongside with pip to be injected into an isolated build environment during build-time dependency population. (<code>[#8214](pypa/pip#8214) <https://github.com/pypa/pip/issues/8214></code>_)</li> <li>Fix <code>pip freeze</code> permission denied error in order to display an understandable error message and offer solutions. (<code>[#8418](pypa/pip#8418) <https://github.com/pypa/pip/issues/8418></code>_)</li> <li>Correctly uninstall script files (from setuptools' <code>scripts</code> argument), when installed with <code>--user</code>. (<code>[#8733](pypa/pip#8733) <https://github.com/pypa/pip/issues/8733></code>_)</li> <li>New resolver: When a requirement is requested both via a direct URL (<code>req @ URL</code>) and via version specifier with extras (<code>req[extra]</code>), the resolver will now be able to use the URL to correctly resolve the requirement with extras. (<code>[#8785](pypa/pip#8785) <https://github.com/pypa/pip/issues/8785></code>_)</li> <li>New resolver: Show relevant entries from user-supplied constraint files in the error message to improve debuggability. (<code>[#9300](pypa/pip#9300) <https://github.com/pypa/pip/issues/9300></code>_)</li> <li>Avoid parsing version to make the version check more robust against lousily debundled downstream distributions. (<code>[#9348](pypa/pip#9348) <https://github.com/pypa/pip/issues/9348></code>_)</li> <li><code>--user</code> is no longer suggested incorrectly when pip fails with a permission error in a virtual environment. (<code>[#9409](pypa/pip#9409) <https://github.com/pypa/pip/issues/9409></code>_)</li> <li>Fix incorrect reporting on <code>Requires-Python</code> conflicts. (<code>[#9541](pypa/pip#9541) <https://github.com/pypa/pip/issues/9541></code>_)</li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pypa/pip/commit/2b2a268d25963727c2a1c805de8f0246b9cd63f6"><code>2b2a268</code></a> Bump for release</li> <li><a href="https://github.com/pypa/pip/commit/ea761a6575f37b90cf89035ee8be3808cf872184"><code>ea761a6</code></a> Update AUTHORS.txt</li> <li><a href="https://github.com/pypa/pip/commit/2edd3fdf2af2f09dce5085ef0eb54684b4f9bc04"><code>2edd3fd</code></a> Postpone a deprecation to 21.2</li> <li><a href="https://github.com/pypa/pip/commit/3cccfbf169bd35133ee25d2543659b9c1e262f8c"><code>3cccfbf</code></a> Rename mislabeled news fragment</li> <li><a href="https://github.com/pypa/pip/commit/21cd124b5d40b510295c201b9152a65ac3337a37"><code>21cd124</code></a> Fix NEWS.rst placeholder position</li> <li><a href="https://github.com/pypa/pip/commit/e46bdda9711392fec0c45c1175bae6db847cb30b"><code>e46bdda</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9827">#9827</a> from pradyunsg/fix-git-improper-tag-handling</li> <li><a href="https://github.com/pypa/pip/commit/0e4938d269815a5bf1dd8c16e851cb1199fc5317"><code>0e4938d</code></a> 📰</li> <li><a href="https://github.com/pypa/pip/commit/ca832b2836e0bffa7cf95589acdcd71230f5834e"><code>ca832b2</code></a> Don't split git references on unicode separators</li> <li><a href="https://github.com/pypa/pip/commit/1320bac4ff80d76b8fba2c8b4b4614a40fb9c6c3"><code>1320bac</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9814">#9814</a> from pradyunsg/revamp-ci-apr-2021-v2</li> <li><a href="https://github.com/pypa/pip/commit/e9cc23ffd97cb6d66d32dc3ec27cf832524bb33d"><code>e9cc23f</code></a> Skip checks on PRs only</li> <li>Additional commits viewable in <a href="https://github.com/pypa/pip/compare/21.0.1...21.1">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pip&package-manager=pip&previous-version=21.0.1&new-version=21.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details>
Bumps [pip](https://github.com/pypa/pip) from 21.0.1 to 21.1.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's changelog</a>.</em></p> <blockquote> <h1>21.1.1 (2021-04-30)</h1> <h2>Deprecations and Removals</h2> <ul> <li>Temporarily set the new "Value for ... does not match" location warnings level to <em>DEBUG</em>, to hide them from casual users. This prepares pip 21.1 for CPython inclusion, while pip maintainers digest the first intake of location mismatch issues for the <code>distutils</code>-<code>sysconfig</code> transition. (<code>[#9912](pypa/pip#9912) <https://github.com/pypa/pip/issues/9912></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>This change fixes a bug on Python <!-- raw HTML omitted -->`_)</li> <li>Fix compatibility between distutils and sysconfig when the project name is unknown outside of a virtual environment. (<code>[#9838](pypa/pip#9838) <https://github.com/pypa/pip/issues/9838></code>_)</li> <li>Fix Python 3.6 compatibility when a PEP 517 build requirement itself needs to be built in an isolated environment. (<code>[#9878](pypa/pip#9878) <https://github.com/pypa/pip/issues/9878></code>_)</li> </ul> <h1>21.1 (2021-04-24)</h1> <h2>Process</h2> <ul> <li>Start installation scheme migration from <code>distutils</code> to <code>sysconfig</code>. A warning is implemented to detect differences between the two implementations to encourage user reports, so we can avoid breakages before they happen.</li> </ul> <h2>Features</h2> <ul> <li>Add the ability for the new resolver to process URL constraints. (<code>[#8253](pypa/pip#8253) <https://github.com/pypa/pip/issues/8253></code>_)</li> <li>Add a feature <code>--use-feature=in-tree-build</code> to build local projects in-place when installing. This is expected to become the default behavior in pip 21.3; see <code>Installing from local packages <https://pip.pypa.io/en/stable/user_guide/#installing-from-local-packages></code>_ for more information. (<code>[#9091](pypa/pip#9091) <https://github.com/pypa/pip/issues/9091></code>_)</li> <li>Bring back the "(from versions: ...)" message, that was shown on resolution failures. (<code>[#9139](pypa/pip#9139) <https://github.com/pypa/pip/issues/9139></code>_)</li> <li>Add support for editable installs for project with only setup.cfg files. (<code>[#9547](pypa/pip#9547) <https://github.com/pypa/pip/issues/9547></code>_)</li> <li>Improve performance when picking the best file from indexes during <code>pip install</code>. (<code>[#9748](pypa/pip#9748) <https://github.com/pypa/pip/issues/9748></code>_)</li> <li>Warn instead of erroring out when doing a PEP 517 build in presence of <code>--build-option</code>. Warn when doing a PEP 517 build in presence of <code>--global-option</code>. (<code>[#9774](pypa/pip#9774) <https://github.com/pypa/pip/issues/9774></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>Fixed <code>--target</code> to work with <code>--editable</code> installs. (<code>[#4390](pypa/pip#4390) <https://github.com/pypa/pip/issues/4390></code>_)</li> <li>Add a warning, discouraging the usage of pip as root, outside a virtual environment. (<code>[#6409](pypa/pip#6409) <https://github.com/pypa/pip/issues/6409></code>_)</li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pypa/pip/commit/c53d88c4c374523425f4db6bef949090764465c0"><code>c53d88c</code></a> Bump for release</li> <li><a href="https://github.com/pypa/pip/commit/4417e7f4bef2b2c70767c1dbfe72f82cc6b7b83f"><code>4417e7f</code></a> Update AUTHORS.txt</li> <li><a href="https://github.com/pypa/pip/commit/0c29bfe48e650c5a428b77eba4af7f067c019cc8"><code>0c29bfe</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9912">#9912</a> from uranusjr/sysconfig-remove-warning-for-python-re...</li> <li><a href="https://github.com/pypa/pip/commit/f56ec327b92ebe836e63e07cb2449a20e09dec38"><code>f56ec32</code></a> Make location mismatch messages DEBUG level</li> <li><a href="https://github.com/pypa/pip/commit/999b121402302a00b235a0d443f5661b69d6fd60"><code>999b121</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9883">#9883</a> from uranusjr/isolated-pip-py36-compat</li> <li><a href="https://github.com/pypa/pip/commit/f88420319db450aefbed1500f04e31be46874aaf"><code>f884203</code></a> Fallback to self-invoke via directory on 3.6</li> <li><a href="https://github.com/pypa/pip/commit/7a77484a492c8f1e1f5ef24eaf71a43df9ea47eb"><code>7a77484</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9835">#9835</a> from jamescurtin/9831-bugfix</li> <li><a href="https://github.com/pypa/pip/commit/914bcc3dba88179f4e88ce90b63660474ba795cd"><code>914bcc3</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pypa/pip/issues/9838">#9838</a> from uranusjr/sysconfig-header-with-none-project</li> <li><a href="https://github.com/pypa/pip/commit/2a009a0b8a5d8d03117897f0f11f9c1dcf2a4b5a"><code>2a009a0</code></a> Better explanatory comment</li> <li><a href="https://github.com/pypa/pip/commit/e7b1722efeaf4ff403142847ce1b52caedd5efcd"><code>e7b1722</code></a> Set dist_name to UNKNOWN when empty outside venv</li> <li>Additional commits viewable in <a href="https://github.com/pypa/pip/compare/21.0.1...21.1.1">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pip&package-manager=pip&previous-version=21.0.1&new-version=21.1.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details>
I understand that this will become default soon. Will I still be able to use the old functionality? Because I have a use case where using this feature breaks my installation. |
@ashrauma the current intent is to remove the old functionality at some point. The reason is that the current behavior (copying the source tree to a temp directory before building) can not be made fully correct in pip, because pip has no way to know what needs to be copied for the build to succeed. That said we are eager to learn about specific use cases that this deprecation is affecting and I encourage you to report details about your breakage in #7555. |
Adds back a minimal version of #7882 behind a flag
--use-feature=in-tree-build
(as suggested in #7555 (comment)).This is my first contribution to the pip codebase, so this is currently missing docs and tests. I will figure out some tests later. Would appreciate advice on the appropriate way to document this.
cc @sbidoul