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

don't install empty dirs during wheel installs #1743

Merged
merged 6 commits into from Apr 25, 2014

Conversation

Projects
None yet
6 participants
@qwcode
Contributor

qwcode commented Apr 22, 2014

a fix for #1632, that solves the problem by not installing empty directories (including the empty __pycache__ dirs in pypy under dist-info). this should really only be an issue an odd cases, as packaging doesn't allow/create empty dirs afaik

note that I'm wanting to start a trend here of commiting the src for test packages (when it's something we've constructed) to tests/data/src, and in general use src for project src, and leave packages just for packages.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 22, 2014

Owner

funny, this teardown hasn't been working since the move to pytest.

Owner

qwcode commented on 5ff772d Apr 22, 2014

funny, this teardown hasn't been working since the move to pytest.

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 23, 2014

Member

So you'd like this for a 1.5.5?

Member

Ivoz commented Apr 23, 2014

So you'd like this for a 1.5.5?

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 23, 2014

Contributor

@dstufft @Ivoz @pfmoore looking for a +1 from someone. the main change is a small, subtle change to move_wheel_files to prevent the pypy __pycache__ problem.

cc @alex

Contributor

qwcode commented Apr 23, 2014

@dstufft @Ivoz @pfmoore looking for a +1 from someone. the main change is a small, subtle change to move_wheel_files to prevent the pypy __pycache__ problem.

cc @alex

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Apr 23, 2014

Member

I'm not sure I understand the changes or how they fix things, but pip.wheel is really hard to follow.

Member

dstufft commented Apr 23, 2014

I'm not sure I understand the changes or how they fix things, but pip.wheel is really hard to follow.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 23, 2014

Contributor

@dstufft in #1632, you say: "So I would say ideally we should only install files (creating any directories we need to)". this change does that, whereas before, we created directories regardless of whether we knew a directory had files.

So you'd like this for a 1.5.5?

#1632 was in the 1.5.5 milestone, so I developed against 1.5.X

Contributor

qwcode commented Apr 23, 2014

@dstufft in #1632, you say: "So I would say ideally we should only install files (creating any directories we need to)". this change does that, whereas before, we created directories regardless of whether we knew a directory had files.

So you'd like this for a 1.5.5?

#1632 was in the 1.5.5 milestone, so I developed against 1.5.X

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Apr 23, 2014

Member

I have no idea what's going on. But thanks so much for working on this <3

Member

alex commented Apr 23, 2014

I have no idea what's going on. But thanks so much for working on this <3

Show outdated Hide outdated pip/wheel.py
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 23, 2014

Member

The core change looks OK to me. Why did you add a scheme argument? Was that for the tests? Sorry I don't have time to do a full review at the moment.

Member

pfmoore commented Apr 23, 2014

The core change looks OK to me. Why did you add a scheme argument? Was that for the tests? Sorry I don't have time to do a full review at the moment.

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 23, 2014

Contributor

Why did you add a scheme argument? Was that for the tests?

yes, for tests, instead of patching. not 100% sure on that, but I try to limit patching when possible.

Contributor

qwcode commented Apr 23, 2014

Why did you add a scheme argument? Was that for the tests?

yes, for tests, instead of patching. not 100% sure on that, but I try to limit patching when possible.

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Apr 24, 2014

Member

I'd also like to write the dist-info stuff out of this function on develop as well so I can refactor it somewhat to add INSTALLER / REQUESTED files. Atm it does more than clobber :/

Member

Ivoz commented Apr 24, 2014

I'd also like to write the dist-info stuff out of this function on develop as well so I can refactor it somewhat to add INSTALLER / REQUESTED files. Atm it does more than clobber :/

@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 24, 2014

Contributor

one thing to note is that if someone (on pypy) happens to manually use compileall to compile the installed files (after pip has installed them), then they'd still have a problem, since this fix works on the install side, not the uninstall side. I'm inclined to let that specific manual case still remain a problem, and hope that pypy fixes the problem on their end eventually.

If that concerns anyone, then maybe a special case in the uninstall logic that looks specifically for empty __pycache__ subdirs only? because I hesitate to do anything more general than that and feel confident about it.

thoughts?

Contributor

qwcode commented Apr 24, 2014

one thing to note is that if someone (on pypy) happens to manually use compileall to compile the installed files (after pip has installed them), then they'd still have a problem, since this fix works on the install side, not the uninstall side. I'm inclined to let that specific manual case still remain a problem, and hope that pypy fixes the problem on their end eventually.

If that concerns anyone, then maybe a special case in the uninstall logic that looks specifically for empty __pycache__ subdirs only? because I hesitate to do anything more general than that and feel confident about it.

thoughts?

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Apr 24, 2014

Member

People manually calling compileall doesn't seem like a huge deal to me.
That said, I want to point out that this isn't a PyPy issue, per-se, it's a
Debian's patched PyPy issue.

Alex

On Thu, Apr 24, 2014 at 12:15 PM, Marcus Smith notifications@github.comwrote:

one thing to note is that if someone (on pypy) happens to manually use
compileall to compile the installed files (after pip has installed them),
then they'd still have a problem, since this fix works on the install side,
not the uninstall side. I'm inclined to let that specific manual case still
remain a problem, and hope that pypy fixes the problem on their end
eventually.

If that concerns anyone, then maybe a special case in the uninstall logic
that looks specifically for empty pycache subdirs only? because I
hesitate to do anything more general than that and feel confident about it.

thoughts?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1743#issuecomment-41320294
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

Member

alex commented Apr 24, 2014

People manually calling compileall doesn't seem like a huge deal to me.
That said, I want to point out that this isn't a PyPy issue, per-se, it's a
Debian's patched PyPy issue.

Alex

On Thu, Apr 24, 2014 at 12:15 PM, Marcus Smith notifications@github.comwrote:

one thing to note is that if someone (on pypy) happens to manually use
compileall to compile the installed files (after pip has installed them),
then they'd still have a problem, since this fix works on the install side,
not the uninstall side. I'm inclined to let that specific manual case still
remain a problem, and hope that pypy fixes the problem on their end
eventually.

If that concerns anyone, then maybe a special case in the uninstall logic
that looks specifically for empty pycache subdirs only? because I
hesitate to do anything more general than that and feel confident about it.

thoughts?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1743#issuecomment-41320294
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 24, 2014

Member

this isn't a PyPy issue, per-se, it's a Debian's patched PyPy issue.

In which case let's leave it to Debian to patch their pip if they want the compileall bit fixed. I think it's enough of a corner case that I'm not convinced we need to add a special case anyway and this pushes me over to "let's not".

Member

pfmoore commented Apr 24, 2014

this isn't a PyPy issue, per-se, it's a Debian's patched PyPy issue.

In which case let's leave it to Debian to patch their pip if they want the compileall bit fixed. I think it's enough of a corner case that I'm not convinced we need to add a special case anyway and this pushes me over to "let's not".

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Apr 24, 2014

Member

Eh, it's a general issue with our install code, there's no reason for us to be installing random empty directories, we shouldn't be installing directories at all, we should be installing files. It just happens that the issue that exposed this is a corner case.

Member

dstufft commented Apr 24, 2014

Eh, it's a general issue with our install code, there's no reason for us to be installing random empty directories, we shouldn't be installing directories at all, we should be installing files. It just happens that the issue that exposed this is a corner case.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Apr 24, 2014

Member

I'm +1 with the patch as a whole. My comment was about the question @qwcode asked about special casing __pycache__ subdirs in uninstall for people who manually run compileall. I'm going on @alex's comment that it's specific to Debian's patched PyPy - if so, then I'm -1 on special casing, if not I'm neutral.

Member

pfmoore commented Apr 24, 2014

I'm +1 with the patch as a whole. My comment was about the question @qwcode asked about special casing __pycache__ subdirs in uninstall for people who manually run compileall. I'm going on @alex's comment that it's specific to Debian's patched PyPy - if so, then I'm -1 on special casing, if not I'm neutral.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Apr 24, 2014

Member

Oh, yea I don't see any reason to special case __pycache__ here.

Member

dstufft commented Apr 24, 2014

Oh, yea I don't see any reason to special case __pycache__ here.

qwcode added a commit that referenced this pull request Apr 25, 2014

Merge pull request #1743 from qwcode/empty_dirs
don't install empty dirs during wheel installs

@qwcode qwcode merged commit 7f355a6 into pypa:1.5.X Apr 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@qwcode

This comment has been minimized.

Show comment
Hide comment
@qwcode

qwcode Apr 25, 2014

Contributor

I'll get this over to develop as well, in a bit.

Contributor

qwcode commented Apr 25, 2014

I'll get this over to develop as well, in a bit.

@qwcode qwcode referenced this pull request Apr 25, 2014

Merged

port #1743 to develop #1760

qwcode added a commit that referenced this pull request Apr 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment