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

Fix/use 4e6d935eafa commit #3176

Closed
wants to merge 1 commit into from

Conversation

xavfernandez
Copy link
Member

@xavfernandez xavfernandez commented Oct 12, 2015

Missing piece of #2535


This change is Reviewable

erikrose added a commit to erikrose/pip that referenced this pull request Oct 12, 2015
This reverts commit 62ac258.

pypa#3176 is about to add the missing piece that makes this code useful (and not dead), so let's not delete it.
@warner
Copy link

warner commented Apr 13, 2016

I've been working with this patch, since my project (Versioneer) needs "pip install ." to invoke sdist instead of doing a copy (so my build_py hook can get control and compute a version from git metadata, before the .git directory goes away).

I updated it to match current trunk, but I ran into a new fun problem. The generated sdist tarball is missing the setup.py file! It has everything else (setup.cfg, README), but not the main setup.py .

I think the problem is that distutils.command.sdist.add_defaults() uses self.distribution.script_name to add setup.py to the generated MANIFEST, and sys.argv[0] is used to calculate script_name, but this function invokes setup.py sdist with a funky SETUPTOOLS_SHIM line instead of a raw "setup.py" string. As a result, I believe script_name is missing or doesn't point to a real file, so add_defaults() doesn't add it to the manifest, so the tarball doesn't include it.

I was able to work around this during my tests by putting setup.py into my MANIFEST.in file, but most projects won't be doing that, so it doesn't really solve my problem. I don't know of a good solution for this.

One other weird thing I noticed with this patch in place: the tarball appears to be unpacked in the same directory where the tarball was copied, so the unpacked sdist will differ from the original source tree (one addtional .tar.gz file). Hopefully that won't matter, but it might be cleaner to point --dist-dir= at a new tempdir then unpack the generated tarball into the target location.

Here's my updated version of the patch (it also removes the now-obsolete comment about the operation being very slow)

3176-updated-diff.txt

@dstufft
Copy link
Member

dstufft commented Apr 13, 2016

One way around that would be to just manually copy the setup.py into the temporary location after we unpack the newly created sdist.

Sent from my iPhone

On Apr 13, 2016, at 2:33 AM, Brian Warner [email protected] wrote:

I've been working with this patch, since my project (Versioneer) needs "pip install ." to invoke sdist instead of doing a copy (so my build_py hook can get control and compute a version from git metadata, before the .git directory goes away).

I updated it to match current trunk, but I ran into a new fun problem. The generated sdist tarball is missing the setup.py file! It has everything else (setup.cfg, README), but not the main setup.py .

I think the problem is that distutils.command.sdist.add_defaults() uses self.distribution.script_name to add setup.py to the generated MANIFEST, and sys.argv[0] is used to calculate script_name, but this function invokes setup.py sdist with a funky SETUPTOOLS_SHIM line instead of a raw "setup.py" string. As a result, I believe script_name is missing or doesn't point to a real file, so add_defaults() doesn't add it to the manifest, so the tarball doesn't include it.

I was able to work around this during my tests by putting setup.py into my MANIFEST.in file, but most projects won't be doing that, so it doesn't really solve my problem. I don't know of a good solution for this.

One other weird thing I noticed with this patch in place: the tarball appears to be unpacked in the same directory where the tarball was copied, so the unpacked sdist will differ from the original source tree (one addtional .tar.gz file). Hopefully that won't matter, but it might be cleaner to point --dist-dir= at a new tempdir then unpack the generated tarball into the target location.

Here's my updated version of the patch (it also removes the now-obsolete comment about the operation being very slow)

3176-updated-diff.txt


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

@warner
Copy link

warner commented Apr 13, 2016

I've added that to my patch, attached here. My versioneer tests work better with that.

3176-updated-diff-2.txt

One remaining problem: MANIFEST.in is not, itself, usually included in the sdist. And it seems like the generated MANIFEST file is not there either. So an unpacked sdist tarball (for any project uses MANIFEST.in) is not itself capable of correctly producing a new second-generation sdist.

If I add include MANIFEST.in to my MANIFEST.in, this works. But I don't think that's common practice. If I omit it, then the versioneer.py that my setup.py needs is missing (along with a lot of other files).

Maybe this is a fringe use case. An unpacked sdist is what you get if you visit PyPI in a browser, download a tarball, unpack it, then run pip install .. If instead you do pip install NAME then you get a different code path that doesn't need to build a new sdist first.

warner added a commit to warner/pip that referenced this pull request Apr 13, 2016
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
@warner
Copy link

warner commented Apr 13, 2016

I updated this patch and added the "copy setup.py" workaround in #3615. It passes tests (on py27 at least) locally, we'll see what travis says. I don't know if there's a way to specifically test the setup.py problem, or how to deal with the MANIFEST.in thing, but maybe #3615 is a better place to discuss that now.

If #3615 lands, this PR should probably be closed.

@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. Unfortunately, this pull request does not cleanly merge against the current master branch.

If you do nothing, this Pull Request will be automatically closed by @BrownTruck since it cannot be merged.

If this pull request is still valid, please rebase it against master (or merge master into it) and resubmit it against the master branch, closing and referencing the original Pull Request.

If you choose to rebase/merge and resubmit this Pull Request, here is an example message that you can copy and paste:

Missing piece of #2535





This change is

---

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

@BrownTruck BrownTruck added the auto-bitrotten PRs that died because they weren't updated label May 26, 2016
@BrownTruck
Copy link
Contributor

This Pull Request was closed because it cannot be automatically reparented to the master branch and it appears to have bit rotted.

Please feel free to re-open it or re-submit it if it is still valid and you have rebased it onto master or merged master into it.

@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
@xavfernandez xavfernandez deleted the fix_copy_via_sdist branch January 10, 2020 12:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-bitrotten PRs that died because they weren't updated auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants