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.

Sign up for GitHub

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

Jump to bottom

Address #6197: use the ephemeral cache if we need to when autobuilding #6219

Merged
merged 4 commits into from
Feb 2, 2019
Merged

Address #6197: use the ephemeral cache if we need to when autobuilding #6219

merged 4 commits into from
Feb 2, 2019

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Jan 29, 2019 •
edited
Loading

Failing test followed by proposed fix for #6197.

@cjerdonek cjerdonek changed the title Add failing test for the have_directory_for_build AssertionError (#6197) [WIP] Add failing test for the have_directory_for_build AssertionError (#6197) Jan 29, 2019
@cjerdonek cjerdonek mentioned this pull request Jan 29, 2019
Closed
pradyunsg
pradyunsg reviewed Jan 29, 2019
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if this is the right approach. This seems like another of pip's cause is far from effect regressions. :(

I've really not had enough time to take a good look at this situation yet to be sure though.

src/pip/_internal/wheel.py Outdated Show resolved Hide resolved
@cjerdonek
Copy link
Member Author

@pradyunsg I made the change. Could it just be a matter of passing a boolean flag to should_use_ephemeral_cache() telling it whether pip is in the "no cache dir" case? (and then using that flag appropriately in the function)

@cjerdonek
Copy link
Member Author

@uranusjr Can you take a look at this issue and PR since you had taken a look at this part when refactoring out _contains_egg_info() and so may have a better understanding? The assertion error actually goes through that code path.

@cjerdonek cjerdonek added C: cache Dealing with cache and files in it type: bugfix labels Jan 30, 2019
@cjerdonek cjerdonek changed the title [WIP] Add failing test for the have_directory_for_build AssertionError (#6197) Address #6197: use the ephemeral cache if we need to when autobuilding Jan 31, 2019
@cjerdonek cjerdonek mentioned this pull request Jan 31, 2019
Closed
@cjerdonek
Copy link
Member Author

@pradyunsg I added my idea for the fix that I said in my last comment to you. You can see what the change was by looking at the second commit in the sequence. I also verified that this change works against the Dockerfile reproducer that was posted in the original issue.

pradyunsg
pradyunsg reviewed Jan 31, 2019
View reviewed changes
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.

LGTM as a quick-fix.

I'm still concerned if there's some issue with the "preparation" of the requirements but I genuinely don't have bandwidth for the next few days.

news/6197.bugfix Outdated
@@ -0,0 +1,2 @@
Fix the ``have_directory_for_build`` ``AssertionError`` when using
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rephrase this to:

Fix a crash where PEP 517-based builds would fail due to not finalizing
a build directory internally, resulting in an ``AssertionError``.

Copy link
Member Author

Choose a reason for hiding this comment

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

I adjusted the news entry along these lines and added some more test cases (by parametrizing the original test).

@danking danking mentioned this pull request Jan 31, 2019
Merged
@uranusjr
Copy link
Member

uranusjr commented Feb 2, 2019

The logic looks good to me.

pradyunsg
pradyunsg approved these changes Feb 2, 2019
View reviewed changes
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.

Let's go.

@pradyunsg pradyunsg merged commit d95b5f2 into pypa:master Feb 2, 2019
@cjerdonek cjerdonek deleted the wheel-build-assertion-error branch February 3, 2019 10:01
MarkKoz added a commit to python-discord/sir-lancebot that referenced this pull request Mar 25, 2019
@MarkKoz
Docker: Update base image to Python 3.7.2
63d1eac 
pip has to be at least version 19.0.2, which fixes a
have_directory_for_build AssertionError when installing with
PIP_NO_CACHE_DIR set. The updated image comes with pip 19.0.3.

See pypa/pip#6219 for more info.
@lock
Copy link

lock bot commented May 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: cache Dealing with cache and files in it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cjerdonek @uranusjr @pradyunsg