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

switch 'pip install .' to use sdists, not copytree #3615

Closed
wants to merge 2 commits into from

Conversation

warner
Copy link

@warner warner commented Apr 13, 2016

This has an updated (mergeable) version of the original #2535 missing patch (to actually use the new copy function), and a workaround for the missing-setup.py problem that I found during testing.

refs #2195, #2535, #3176


This change is Reviewable

xavfernandez and others added 2 commits April 13, 2016 13:33
This works around a problem with the new sdist-based "pip install .":

* when creating the sdist, we don't run a literal "setup.py sdist"
* instead, sys.argv[0] is a complicated shim that injects
  setuptools even into distutils-based projects
* as a result, distutils.command.sdist.add_defaults() doesn't realize
  that "setup.py" is the name of its setup script (it gets confused
  because sys.argv[0] is not a real file).
* so add_defaults() doesn't include setup.py in the generated
  tarball. (projects could add "include setup.py" to their MANIFEST.in,
  but this is not common practice because usually it's automatic)
* so the unpacked sdist (from which pip will make a wheel) lacks the
  critical setup.py

This copies the setup.py from source tree to unpacked target tree.

The patch also removes a performance comment that was obsoleted by
switching to _copy_dist_from_dir().

refs pypa#2195, pypa#2535, pypa#3176
@rbtcollins
Copy link

Can we not just install from the directory itself? I really don't buy that its pips job to test the sdistability of every package every time.

@rbtcollins
Copy link

I should mention - doing an sdist for some projects is much potentially more expensive than an install, because you may well compile documentation, man pages etc. I don't have any stats on how common that is - but my preferred thing is to really just get the focus on make-a-wheel tightened, and let the source path be a source path.

@dstufft
Copy link
Member

dstufft commented Apr 14, 2016

I don't think we should just install from the directory itself. It is my belief that, by default, pip should do an isolated build by default, and explicitly opt-in to anything else.

@warner
Copy link
Author

warner commented Apr 15, 2016

For context, my interest in this change comes from Versioneer, where I've hooked the distutils/setuptools build_py command to run git describe to automatically figure out a version string.

When the package's setup.py is located in a subdirectory of the original git checkout (python-versioneer/python-versioneer#38), the current "copy os.path.dirname(setup_py)" behavior fails to copy the .git directory, and my hook fails. The sdist-first approach works because my hook gets run during the creation of the sdist.

I'd be happy with anything that lets me get the right version string. I'm neutral about whether pip install . should have side-effects similar to pip wheel . (which I guess is what would happen if we don't do an isolated build). For my purposes, the most reliable approach would be to run just setup.py egg_info in the original directory, then somehow use that version string in the final build (wherever it happens), maybe by copying it on top of the copied source tree. Or maybe the post-copy build could be told to pay attention to the .egg_info from the original source tree instead of re-building one in the new copy.

@scudette
Copy link

I think the whole point of an isolated install is that there is a clear and constant path from a source tree to an installed setup. Trying to argue that building an sdist first might be too expensive and therefore we can probably just skip this step is basically sacrificing stability and repeatable builds for the dubious gains of some CPU cycles at install time.

Basically right now there are many different but similar paths of getting a package installed:

1 - python setup.py sdist + pip install sdist
2 - pip wheel directory + pip install wheel
3 - pip install directory

Because they go through different paths what you get installed can be totally different. In the example rbtcollins commented on, the project builds documentation when they run sdist. So clearly not running sdist first (i.e. path 2 above) will install a broken package (missing documentation). While running sdist first and then installing the sdist will produce a different install with documentation.

This non-deterministic and confusing state of affairs occurs because pip tries to take too many shortcuts. If the rules are clear and the installation process is well defined things would be way better.

In the versioneer example, the setup.py file is specifically designed to embed the version into the sdist so it will work exactly as expected - sdist will be built with a frozen version, and pip will install that.

@dstufft dstufft closed this May 18, 2016
@dstufft
Copy link
Member

dstufft commented May 18, 2016

Accidentally closed this, reopening. Sorry!

@dstufft dstufft reopened this May 18, 2016
@BrownTruck
Copy link
Contributor

Hello!

As part of an effort to ease the contribution process and adopt a more standard workflow pip has switched to doing development on the master branch. However, this Pull Request was made against the develop branch so it will need to be resubmitted against master.

If you do nothing, this Pull Request will be automatically migrated by @BrownTruck for you.

If you would like to retain control over this pull request then you should resubmit it against the master branch, closing and referencing the original Pull Request.

If you choose to migrate this Pull Request yourself, here is an example message that you can copy and paste:

This has an updated (mergeable) version of the original #2535 missing patch (to actually use the new copy function), and a workaround for the missing-setup.py problem that I found during testing.

refs #2195, #2535, #3176





This change is

---

*This was migrated from pypa/pip#3615 to reparent it to the ``master`` branch. Please see original pull request for any previous discussion.*

If this pull request is no longer needed, please feel free to close it.

@BrownTruck
Copy link
Contributor

This Pull Request has been automatically migrated to #3722 to reparent it to the master branch. Please see the new pull request for any new discussion.

@BrownTruck BrownTruck closed this May 26, 2016
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants