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 build directory assertion for a PEP 517/518 world #6171

Merged
merged 3 commits into from Jan 23, 2019

Conversation

@pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Jan 23, 2019

Closes #6158

pradyunsg added 2 commits Jan 23, 2019
When the ephemeral cache is used, the build can always occur. There is
no need to check for those.
@@ -840,12 +840,6 @@ def build(
newly built wheel, in preparation for installation.
:return: True if all the wheels built correctly.

This comment has been minimized.

@xavfernandez

xavfernandez Jan 23, 2019
Member

(unrelated to this PR but the docstring seems obsolete :) )

have_directory_for_build = self._wheel_dir or (
autobuilding and self.wheel_cache.cache_dir
)
assert have_directory_for_build

This comment has been minimized.

@cjerdonek

cjerdonek Jan 23, 2019
Member

For future reference, it looks like an alternative way to do this would be to calculate a needs_ephem_cache variable (or similarly defined variable) prior to the for loop above. And then check that ephem_cache is true inside the for loop prior to appending to the buildset (depending on whether needs_ephem_cache is true). That would give you more information if the condition ever fails, and let you raise an exception with more specific, contextual info (as well as fail faster).

This comment has been minimized.

@pradyunsg

pradyunsg Jan 23, 2019
Author Member

Yea, this worked and I decided "good is better than perfect" wrt code cleanup here. :)

@pradyunsg pradyunsg merged commit ab79665 into pypa:master Jan 23, 2019
6 checks passed
6 checks passed
@azure-pipelines
Linux Build #20190122.16 succeeded
Details
@azure-pipelines
Windows Build #20190122.16 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@azure-pipelines
macOS Build #20190123.1 succeeded
Details
@pypa-bot
news-file/pr News files updated and/or change is trivial.
Details
@pradyunsg pradyunsg deleted the pradyunsg:fix/pep-517-building-assertion branch Jan 23, 2019
@vmarkovtsev

This comment was marked as spam.

Copy link

@vmarkovtsev vmarkovtsev commented on 7db2666 Jan 23, 2019

I wonder how pip is tested if such a devastating thing was missed before releasing. It broke all our Travis builds.

This comment has been minimized.

Copy link

@ringerc ringerc replied Jan 24, 2019

This should fix build failures like

Traceback (most recent call last):
  File "virtualenv/local/lib/python2.7/site-packages/pip/_internal/cli/base_command.py", line 176, in main
    status = self.run(options, args)
  File "virtualenv/local/lib/python2.7/site-packages/pip/_internal/commands/install.py", line 346, in run
    session=session, autobuilding=True
  File "virtualenv/local/lib/python2.7/site-packages/pip/_internal/wheel.py", line 848, in build
    assert building_is_possible
AssertionError

This comment has been minimized.

Copy link
Member Author

@pradyunsg pradyunsg replied Jan 24, 2019

See #6158.

simingy added a commit to CiscoTestAutomation/pyats-docker that referenced this pull request Jan 24, 2019
@simingy
Copy link

@simingy simingy commented Jan 24, 2019

still seeing this issue in v19.0.1 in alpine:3.8 when pip installing using --no-cache-dir.

adding --no-use-pep517 seems to be a workaround

@lock
Copy link

@lock 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 S: auto-locked 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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants