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

Clean build dir if bdist_wheel fails #3047

Merged
merged 2 commits into from Sep 29, 2015

Conversation

Projects
None yet
3 participants
@tgs
Contributor

tgs commented Aug 21, 2015

When pip tries to build a wheel but fails, setup.py install is run in
the same directory. However, some build files are not compatible
between 'install' and 'bdist_wheel', particularly scripts - the shebang
lines are different. Pip now cleans the build dir after a failed
bdist_wheel so that pip doesn't install broken scripts.

Closes #3006

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Aug 22, 2015

Contributor

On second thoughts, I think I would add a new method 'def _clean' called in _build_one (and not __build_one) if __build_one returns None.

A simple testcase with a package containing scripts and failing on bdist_wheel would also be nice, cf tests/data/packages or tests/data/src for examples.

Contributor

xavfernandez commented Aug 22, 2015

On second thoughts, I think I would add a new method 'def _clean' called in _build_one (and not __build_one) if __build_one returns None.

A simple testcase with a package containing scripts and failing on bdist_wheel would also be nice, cf tests/data/packages or tests/data/src for examples.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Aug 22, 2015

Member

I agree with @xavfernandez - the basic approach is good, but refactoring as suggested would be useful.

For a test, something entirely artificial (e.g., a package that checks argv to see if bdist_wheel is being called, and if so fails) is probably better than supplying an invalid install_requires, in case downstream fixes the issue there...

Member

pfmoore commented Aug 22, 2015

I agree with @xavfernandez - the basic approach is good, but refactoring as suggested would be useful.

For a test, something entirely artificial (e.g., a package that checks argv to see if bdist_wheel is being called, and if so fails) is probably better than supplying an invalid install_requires, in case downstream fixes the issue there...

@tgs

This comment has been minimized.

Show comment
Hide comment
@tgs

tgs Aug 23, 2015

Contributor

Ok, I'll take a stab at that :)
Maybe Monday.

http://resc.smugmug.com/

Contributor

tgs commented Aug 23, 2015

Ok, I'll take a stab at that :)
Maybe Monday.

http://resc.smugmug.com/

@tgs

This comment has been minimized.

Show comment
Hide comment
@tgs

tgs Aug 24, 2015

Contributor

I refactored the changes to wheel.py and got started testing, but it turns out to be slightly tricky - the wheel build has to fail in just the right place to reproduce the bug, maybe? Will look again tomorrow, but then I'm on vacation for a few days, the next chance I get will be next week.

Contributor

tgs commented Aug 24, 2015

I refactored the changes to wheel.py and got started testing, but it turns out to be slightly tricky - the wheel build has to fail in just the right place to reproduce the bug, maybe? Will look again tomorrow, but then I'm on vacation for a few days, the next chance I get will be next week.

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Aug 25, 2015

Contributor

You can take a look at https://github.com/pypa/pip/blob/develop/tests/data/packages/wheelbroken-0.1.tar.gz , its setup.py contains:

if sys.argv[1] == 'bdist_wheel':
    raise FakeError('this package designed to fail on bdist_wheel')
Contributor

xavfernandez commented Aug 25, 2015

You can take a look at https://github.com/pypa/pip/blob/develop/tests/data/packages/wheelbroken-0.1.tar.gz , its setup.py contains:

if sys.argv[1] == 'bdist_wheel':
    raise FakeError('this package designed to fail on bdist_wheel')

tgs added some commits Aug 21, 2015

Clean build dir if bdist_wheel fails
When pip tries to build a wheel but fails, setup.py install is run in
the same directory.  However, some build files are not compatible
between 'install' and 'bdist_wheel', particularly scripts - the shebang
lines are different.  Pip now cleans the build dir after a failed
bdist_wheel so that pip doesn't install broken scripts.

Closes #3006
@tgs

This comment has been minimized.

Show comment
Hide comment
@tgs

tgs Sep 9, 2015

Contributor

I made a version of wheelbroken where the raise FakeError comes after the call to setup(), so that it actually makes some build files and things. That did trigger the bug.

Contributor

tgs commented Sep 9, 2015

I made a version of wheelbroken where the raise FakeError comes after the call to setup(), so that it actually makes some build files and things. That did trigger the bug.

@tgs

This comment has been minimized.

Show comment
Hide comment
@tgs

tgs Sep 17, 2015

Contributor

This is ready for you to take a look again when you get a chance (thanks for the suggestions!)

For ease of web-based review, here's the setup.py of the new test package:

from distutils.core import setup
import sys

class FakeError(Exception):
    pass

setup(name='wheelbrokenafter',
      version='0.1',
      scripts=['script.py'],
      )

if sys.argv[1] == 'bdist_wheel':
    # We fail afterwards so that we leave our build artifacts lying around.
    raise FakeError('this package designed to fail AFTER bdist_wheel')

Let me know what you think!

Contributor

tgs commented Sep 17, 2015

This is ready for you to take a look again when you get a chance (thanks for the suggestions!)

For ease of web-based review, here's the setup.py of the new test package:

from distutils.core import setup
import sys

class FakeError(Exception):
    pass

setup(name='wheelbrokenafter',
      version='0.1',
      scripts=['script.py'],
      )

if sys.argv[1] == 'bdist_wheel':
    # We fail afterwards so that we leave our build artifacts lying around.
    raise FakeError('this package designed to fail AFTER bdist_wheel')

Let me know what you think!

@tgs

This comment has been minimized.

Show comment
Hide comment
@tgs

tgs Sep 28, 2015

Contributor

Poke?

Contributor

tgs commented Sep 28, 2015

Poke?

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Sep 28, 2015

Member

Looks OK to me. @xavfernandez do you have any further comments?

Member

pfmoore commented Sep 28, 2015

Looks OK to me. @xavfernandez do you have any further comments?

@@ -668,19 +668,24 @@ def _build_one(self, req, output_dir):
logger.info('Stored in directory: %s', output_dir)
return wheel_path
except:
return None
pass

This comment has been minimized.

@xavfernandez

xavfernandez Sep 28, 2015

Contributor

Even if there was no logging previously I think a logger.debug of the exception could be helpful.

@xavfernandez

xavfernandez Sep 28, 2015

Contributor

Even if there was no logging previously I think a logger.debug of the exception could be helpful.

This comment has been minimized.

@pfmoore

pfmoore Sep 28, 2015

Member

You may be right - presumably even if the package is installed, the user wants to know that the wheel build failed as the implication is that the expected improved performance on repeated installs will be lost. Maybe there should even be a warning "wheel build failed, falling back to source install" as well as the debug info providing the full exception details.

@pfmoore

pfmoore Sep 28, 2015

Member

You may be right - presumably even if the package is installed, the user wants to know that the wheel build failed as the implication is that the expected improved performance on repeated installs will be lost. Maybe there should even be a warning "wheel build failed, falling back to source install" as well as the debug info providing the full exception details.

This comment has been minimized.

@pfmoore

pfmoore Sep 28, 2015

Member

Correction - if the wheel build fails, that results in __build_one returning False. The except clause here is solely if the wheel build succeeded, but the process of moving the wheel into wheel_path (the cache, presumably?) failed.

In that case, agreed a debug of the exception might be useful. But logging at warning level that caching the wheel failed is also probably useful. Both points are independent of this patch, though, so I think they should be dealt with separately.

@pfmoore

pfmoore Sep 28, 2015

Member

Correction - if the wheel build fails, that results in __build_one returning False. The except clause here is solely if the wheel build succeeded, but the process of moving the wheel into wheel_path (the cache, presumably?) failed.

In that case, agreed a debug of the exception might be useful. But logging at warning level that caching the wheel failed is also probably useful. Both points are independent of this patch, though, so I think they should be dealt with separately.

This comment has been minimized.

@xavfernandez

xavfernandez Sep 29, 2015

Contributor

k, we'll add the missing log in a different PR

@xavfernandez

xavfernandez Sep 29, 2015

Contributor

k, we'll add the missing log in a different PR

call_subprocess(clean_args, cwd=req.source_dir, show_stdout=False)
return True
except:
logger.error('Failed cleaning build dir for %s', req.name)

This comment has been minimized.

@xavfernandez

xavfernandez Sep 28, 2015

Contributor

Wondering whether this should not be downgraded to a warning (even if the cleanup fails, there are good chances the install will be completed).

@xavfernandez

xavfernandez Sep 28, 2015

Contributor

Wondering whether this should not be downgraded to a warning (even if the cleanup fails, there are good chances the install will be completed).

This comment has been minimized.

@pfmoore

pfmoore Sep 28, 2015

Member

But the install could be broken. We can't tell. I couldn't find a good definition of what the different levels actually mean, but my instinct says that warning means "you should know this but everything is basically OK" and error says "you need to know this, something's wrong". There's nothing that covers "something might be wrong" but I'm inclined to think that by the time you get here a wheel build has failed, and now you can't clean up, so things seem to be going badly and an error is probably warranted.

@pfmoore

pfmoore Sep 28, 2015

Member

But the install could be broken. We can't tell. I couldn't find a good definition of what the different levels actually mean, but my instinct says that warning means "you should know this but everything is basically OK" and error says "you need to know this, something's wrong". There's nothing that covers "something might be wrong" but I'm inclined to think that by the time you get here a wheel build has failed, and now you can't clean up, so things seem to be going badly and an error is probably warranted.

This comment has been minimized.

@tgs

tgs Sep 29, 2015

Contributor

Yeah, I was also debating the logging level with myself. I ended up going with "a setup.py command has failed, so call it an error", which seems to be the rule in other places. I'm happy to go with whatever consensus, though.

@tgs

tgs Sep 29, 2015

Contributor

Yeah, I was also debating the logging level with myself. I ended up going with "a setup.py command has failed, so call it an error", which seems to be the rule in other places. I'm happy to go with whatever consensus, though.

This comment has been minimized.

@xavfernandez

xavfernandez Sep 29, 2015

Contributor

Agreed on leaving it at error level :)

@xavfernandez

xavfernandez Sep 29, 2015

Contributor

Agreed on leaving it at error level :)

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Sep 28, 2015

Contributor

Outside of some questioning on the logging, it looks fine to me.

Contributor

xavfernandez commented Sep 28, 2015

Outside of some questioning on the logging, it looks fine to me.

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Sep 29, 2015

Contributor

Looks like everything is fine, could you add a small changelog for this fix ?
Cf https://github.com/pypa/pip/blob/develop/CHANGES.txt

Contributor

xavfernandez commented Sep 29, 2015

Looks like everything is fine, could you add a small changelog for this fix ?
Cf https://github.com/pypa/pip/blob/develop/CHANGES.txt

pfmoore added a commit that referenced this pull request Sep 29, 2015

Merge pull request #3047 from tgs/clean_wheel_build_dir
Clean build dir if bdist_wheel fails

@pfmoore pfmoore merged commit 908d077 into pypa:develop Sep 29, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Sep 29, 2015

Member

I've merged this and will add the changelog. Thanks @tgs for the contribution!

Member

pfmoore commented Sep 29, 2015

I've merged this and will add the changelog. Thanks @tgs for the contribution!

@xavfernandez

This comment has been minimized.

Show comment
Hide comment
@xavfernandez

xavfernandez Sep 29, 2015

Contributor

👍 Thanks :)

Contributor

xavfernandez commented Sep 29, 2015

👍 Thanks :)

@tgs

This comment has been minimized.

Show comment
Hide comment
@tgs

tgs Sep 29, 2015

Contributor

Thanks for your help, I appreciate it!

Contributor

tgs commented Sep 29, 2015

Thanks for your help, I appreciate it!

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