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

Add missing error logging in WheelBuilder #7484

Merged
merged 5 commits into from
Dec 18, 2019

Conversation

sbidoul
Copy link
Member

@sbidoul sbidoul commented Dec 14, 2019

Further simplify WheelBuilder.build(), and add a missing error report.

Use the same error message as in WheelBuilder.build(),
include the exception in the message.
Since _build_one ensure the output dir is
present and does proper error logging,
we can remove ensure_dir from build(), further
simplifying that method.
@chrahunt chrahunt added the skip news Does not need a NEWS file entry (eg: trivial changes) label Dec 14, 2019
@chrahunt
Copy link
Member

The previous implementation had one important feature: it would fail before the wheel build (which can take some time). Maybe _build_one would be a good place to put this?

@pradyunsg
Copy link
Member

feature: it would fail before the wheel build

That's a good thing IMO and we should add a test for this.

@sbidoul
Copy link
Member Author

sbidoul commented Dec 15, 2019

Note this particular directory is going to be inside the cache directory.

When trying pip wheel with a non-writable cache directory, one gets WARNING: The directory '/tmp/cache/http' or its parent directory is not owned by the current user and the cache has been disabled. Please check the permissions and owner of that directory. If executing pip with sudo, you may want sudo's -H flag..

So it looks like checking that the main conditions for this particular ensure_dir to succeed are checked beforehand and it would fail only in very rare situations. So I'd think this particular optimization and test is maybe not worth the additional code?

@sbidoul
Copy link
Member Author

sbidoul commented Dec 15, 2019

Hm, despite saying the cache is disabled, it attempts to use it nevertheless:

WARNING: The directory '/tmp/cache/http' or its parent directory is not owned by the current user and the cache has been disabled. Please check the permissions and owner of that directory. If executing pip with sudo, you may want sudo's -H flag.
Collecting odoo-autodiscover
  Downloading odoo-autodiscover-2.0.0.tar.gz (16 kB)
Collecting wrapt
  Downloading wrapt-1.11.2.tar.gz (27 kB)
Building wheels for collected packages: odoo-autodiscover, wrapt
  Building wheel for odoo-autodiscover (setup.py) ... done
  WARNING: Building wheel for odoo-autodiscover failed: [Errno 13] Permission denied: 'cache/wheels'
  Running setup.py clean for odoo-autodiscover
  Building wheel for wrapt (setup.py) ... done
  WARNING: Building wheel for wrapt failed: [Errno 13] Permission denied: 'cache/wheels'
  Running setup.py clean for wrapt
Failed to build odoo-autodiscover wrapt
ERROR: Failed to build one or more wheels

@sbidoul
Copy link
Member Author

sbidoul commented Dec 15, 2019

Because the ownership/writeability check is done for the http cache and not the wheel cache.

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Small comment, otherwise this LGTM.

src/pip/_internal/wheel_builder.py Outdated Show resolved Hide resolved
Co-Authored-By: Christopher Hunt <[email protected]>
@chrahunt chrahunt merged commit 2a48286 into pypa:master Dec 18, 2019
@sbidoul sbidoul deleted the wheel-builder-disentangle-3-sbi branch December 18, 2019 08:23
@sbidoul
Copy link
Member Author

sbidoul commented Dec 18, 2019

Thanks!

@pradyunsg
Copy link
Member

LGTM -- thanks @sbidoul and @chrahunt! ^>^

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jan 17, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 17, 2020
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 skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants