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

Build local directories in place #7882

Merged
merged 5 commits into from
Apr 13, 2020

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Mar 22, 2020

This is a rough experiment with in-place builds, to see the effect on the code base and tests, and advance the discussion in #7555.

@sbidoul sbidoul force-pushed the build-in-place-7555-sbi branch 3 times, most recently from 61602fe to 6b6dee2 Compare March 28, 2020 18:26
@sbidoul
Copy link
Member Author

sbidoul commented Mar 28, 2020

This is getting in good shape.
We can see that quite a lot of code relating to copying the source directory can be removed.
Tests pass except on travis. I still need to check why (might be a transient).

As expected some tests had to be changed to cope with the .egg-info directory that gets created by setuptools in the source directory.

The only thing that stands out is this pip-egg-info directory that pip creates in the project directory to generate (temporary) egg-info metadata. I think it can be converted to a temporary directory to have that out of the way.

@pypa/pip-committers what do you think? Should I continue down this path?

@uranusjr
Copy link
Member

The Travis failures are all the same, the worker has problems reaching PyPI for setup (it didn’t even reach the test phase).

@pradyunsg
Copy link
Member

@sbidoul Sounds like a good idea! Still need to look at this PR, and hopefully by the time this gets updated, I'd have cleaned up my notifications situation significantly. :)

@sbidoul
Copy link
Member Author

sbidoul commented Mar 31, 2020

Note: maybe we should disable the copy while keeping the code around to facilitate an emergency revert in case this change raises problem for valid uses cases we'd have not anticipated. And then remove the dead code after 3 months.

@sbidoul sbidoul force-pushed the build-in-place-7555-sbi branch 2 times, most recently from 1969caa to 1cbbf58 Compare April 10, 2020 12:17
@pradyunsg
Copy link
Member

pradyunsg commented Apr 10, 2020

while keeping the code around

git revert is cheap to do, so removing is fine.

@pradyunsg
Copy link
Member

@sbidoul Do you want this to be on the pip 20.1 release? I personally think it would reasonable to try to be on this train, since bundling this in the same release as the new resolver... might not be the best thing to do. OTOH, it might feel hurried to some but, hey, who knows. :)

Plus, this isn't a big "code change" but is a whole bunch of communication + community management discussion. We will have a beta period for 20.1, and it'd make a lot of sense to try to get feedback on this change as part of that IMO.

news/7555.removal Outdated Show resolved Hide resolved
@sbidoul
Copy link
Member Author

sbidoul commented Apr 11, 2020

@pradyunsg ok, to get this out in 20.1. TIL there will be a beta period, so yes it's a good candidate to test the beta feedback process.

@pradyunsg
Copy link
Member

TIL

It got announced today. :P

#7951 (comment)

@pradyunsg pradyunsg added the type: enhancement Improvements to functionality label Apr 11, 2020
@pradyunsg pradyunsg added this to the 20.1 milestone Apr 11, 2020
@pradyunsg pradyunsg added the !release blocker Hold a release until this is resolved label Apr 11, 2020
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Implementation -- LGTM.
Documentation -- would be important to have for this IMO; NEWS fragment feels like it's compensating for the lack of docs. :/

Comment on lines +1 to +9
Building of local directories is now done in place. Previously pip did copy the
local directory tree to a temporary location before building. That approach had
a number of drawbacks, among which performance issues, as well as various
issues arising when the python project directory depends on its parent
directory (such as the presence of a VCS directory). The user visible effect of
this change is that secondary build artifacts, if any, may therefore be created
in the local directory, whereas before they were created in a temporary copy of
the directory and then deleted. This notably includes the ``build`` and
``.egg-info`` directories in the case of the setuptools build backend.
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit excessive. I think we should only summarize the change here, add a section to the documentation and link to that from here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pradyunsg would you have a suggestion about a suitable place for this in the documentation?

Copy link
Member

Choose a reason for hiding this comment

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

https://pip.pypa.io/en/latest/reference/pip_install/#local-project-installs needs updating, so that would likely be the best place.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pradyunsg thanks for the documentation pointer, I updated that section. Regarding the changelog I thought the breaking change warranted a bit more verbosity than usual.

src/pip/_internal/operations/prepare.py Show resolved Hide resolved
Since we now build in place, pip install
calls setup.py in place which in turn creates
fake_pkg.egg-info. Since in this test the package we are installing is in script.temp_path,
we must tell script to expect temporary
files to be created.
This particular test checks which files are
created. Since we now build in place, expect the .egg-info directory to be created.
@pradyunsg
Copy link
Member

LGTM! I think I'll try to do another pass on the documentation prior to the release, but no promises.

Let's open the flood gates, AKA merge this PR! \o/

@pradyunsg pradyunsg merged commit f458573 into pypa:master Apr 13, 2020
@pradyunsg
Copy link
Member

Hurrah! Thanks for actually figuring out a small-patch-that's-actually-reasonable for doing this! ^.^

Thanks @NoahGorny for taking a look and for the reviews + inputs @xavfernandez @uranusjr! ^>^

@sbidoul
Copy link
Member Author

sbidoul commented Apr 13, 2020

Thanks @pradyunsg. I went ahead and closed #7555 and linked issues.

@pradyunsg
Copy link
Member

Oh... I was waiting until after the beta release, to use those issues to get users to test the beta and report feedback on this change, instead of closing them now, which sorta indicates that "there's nothing more to do here".

I guess we can still post the "hey test this beta" messages posted there, but it might not have the same effect anymore. This makes things a bit more... tricky... in terms of communicating around this change. :/

@sbidoul
Copy link
Member Author

sbidoul commented Apr 14, 2020

Oh, sorry. I was under the impression that we were always closing issues as soon as the fixing PR was merged. I did not know you wanted to apply another process here.

Wrt the beta process, I personally have little doubt the above issues are resolved with this PR, so the beta process is probably not so much about verifying that. We probably need to focus on surfacing possible new issues that this change may create to users and use cases we don't know yet. And communicate clearly about potential impacts.

@pradyunsg
Copy link
Member

Yea, it's not like you did anything inappropriate or anything along those lines.

I'm still figuring out how exactly to phrase the communication around this, which is basically why I hadn't actually commented that I'm thinking about using this approach. We learn more as we do more stuff, I guess. :P

poppyschmo added a commit to poppyschmo/pytest-pdb-break that referenced this pull request May 10, 2020
Starting with pip 20.1b1+, the *-info subdir appears as a build artifact
under the "local" directory (here, /project). See
- pypa/pip#7555
- pypa/pip#7882
- pypa/pip@ace0c16
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation !release blocker Hold a release until this is resolved type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants