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

Workaround: distutils sdist fails on some filesystems (like VirtualBox shared filesystem) #56

Closed
markmevans opened this Issue Feb 14, 2014 · 8 comments

Comments

Projects
None yet
2 participants
@markmevans
Contributor

markmevans commented Feb 14, 2014

This is not a PyBuilder bug, but I thought this work around might be useful to other PyBuilder users. There is a long standing distutils bug that prevents distutils from working in certain environments. Basically, distutils assumes that all mounted filesystems support hardlinks if os.link exists. This is not a valid assumption:

BuildFailedException: Error while executing setup command sdist
------------------------------------------------------------
BUILD FAILED - Error while executing setup command sdist
------------------------------------------------------------

Here is a gist of a workaround that I use: file-sdist_hack-py

If you are encountering "error: Operation not permitted" errors in VirtualBox on the shared filesystem, including the gist's code in build.py should workaround the issue.

It's admittedly a hack as it monkey patches the distutil_plugin using too much knowledge of the distutil_plugin's internals. Then the monkey patched code generates a setup.py file that monkey patches the os module, deleting it's reference to link. But it works...

@mriehl

This comment has been minimized.

Show comment
Hide comment
@mriehl

mriehl Feb 16, 2014

Member

Is there any way to do this conditionally? (e.G. detect in setup.py if hardlinks are supported).
I think the distutils plugin should prevent users from falling into this kind of trap, but always removing the reference to link seems a bit too strong?

Member

mriehl commented Feb 16, 2014

Is there any way to do this conditionally? (e.G. detect in setup.py if hardlinks are supported).
I think the distutils plugin should prevent users from falling into this kind of trap, but always removing the reference to link seems a bit too strong?

@markmevans

This comment has been minimized.

Show comment
Hide comment
@markmevans

markmevans Feb 17, 2014

Contributor

Yeah, I was almost too embarrassed to post this because it's so crude, but I decided it might help someone in a similar position. Since the distutils maintainer is promising to fix this "soon", I didn't try to get too clever. This was meant as a temporary hack that people could put in build.py.

That said, it could be useful as part of the default template in setup.py since the bug will probably exist in various distributions for a couple more years (Ubuntu 12.04 is still on 2.7.3). I'm a bit nervous about trying to automatically detect support of hardlinks. How about a property on the distutils? Something silly like, issue8876_workaround and a brief description a long the lines of "Work around distutils bug (http://bugs.python.org/issue8876) that prevents publish on some filesystems."

Contributor

markmevans commented Feb 17, 2014

Yeah, I was almost too embarrassed to post this because it's so crude, but I decided it might help someone in a similar position. Since the distutils maintainer is promising to fix this "soon", I didn't try to get too clever. This was meant as a temporary hack that people could put in build.py.

That said, it could be useful as part of the default template in setup.py since the bug will probably exist in various distributions for a couple more years (Ubuntu 12.04 is still on 2.7.3). I'm a bit nervous about trying to automatically detect support of hardlinks. How about a property on the distutils? Something silly like, issue8876_workaround and a brief description a long the lines of "Work around distutils bug (http://bugs.python.org/issue8876) that prevents publish on some filesystems."

@mriehl

This comment has been minimized.

Show comment
Hide comment
@mriehl

mriehl Feb 19, 2014

Member

I think it's interesting and useful. I wasn't even aware of this particular issue with distutils.

But it does not only prevent you from publishing locally. You're also unable to install a PyPI package if the aforementioned workaround is not used in setup.py, right? I think PyBuilder should shield developers from this kind of bug. Published packages should just work.

Auric also mentions that

using hard links only serves to save up a little time and disk space; it seems to me that always copying would solve one or two bugs at a small cost

so while I think it's worth considering a property like you proposed, I'm wondering if maybe it should be done by default. Of course I don't like monkey patching os like this either.

Member

mriehl commented Feb 19, 2014

I think it's interesting and useful. I wasn't even aware of this particular issue with distutils.

But it does not only prevent you from publishing locally. You're also unable to install a PyPI package if the aforementioned workaround is not used in setup.py, right? I think PyBuilder should shield developers from this kind of bug. Published packages should just work.

Auric also mentions that

using hard links only serves to save up a little time and disk space; it seems to me that always copying would solve one or two bugs at a small cost

so while I think it's worth considering a property like you proposed, I'm wondering if maybe it should be done by default. Of course I don't like monkey patching os like this either.

@markmevans

This comment has been minimized.

Show comment
Hide comment
@markmevans

markmevans Feb 19, 2014

Contributor

I did not know there were implications on the installation as well, if so I would be more inclined to include it by default. Since distutils explicitly survives a missing os.link (presumably because not all platforms support it), it's probably reasonably safe.

Maybe include it in the template via variable expansion and a property that defaults to true just in case someone wants to disable it?

Edit:

I did some spelunking through distutils implementation in the sources for Python 2.7.6 and Python 3.3.4.

The only use of os.link() is in distutils.file_util.copy_file() and it depends on an optional argument which defaults to None (which means copy, don't link). The only invocation of copy_file() that requests a hard link is in distutils.command.sdist in the method sdist.make_release_tree() and the decision to request a hard link is based on the result of hasattr(os, ‘link’).

All other invocations of copy_file() use the default value for the link argument (meaning copy.)

So while not pretty, I think it is a fairly safe “hack." I did not find a code path in install that would invoke os.link, so it does seem to be build time only, during package (sdist).

Contributor

markmevans commented Feb 19, 2014

I did not know there were implications on the installation as well, if so I would be more inclined to include it by default. Since distutils explicitly survives a missing os.link (presumably because not all platforms support it), it's probably reasonably safe.

Maybe include it in the template via variable expansion and a property that defaults to true just in case someone wants to disable it?

Edit:

I did some spelunking through distutils implementation in the sources for Python 2.7.6 and Python 3.3.4.

The only use of os.link() is in distutils.file_util.copy_file() and it depends on an optional argument which defaults to None (which means copy, don't link). The only invocation of copy_file() that requests a hard link is in distutils.command.sdist in the method sdist.make_release_tree() and the decision to request a hard link is based on the result of hasattr(os, ‘link’).

All other invocations of copy_file() use the default value for the link argument (meaning copy.)

So while not pretty, I think it is a fairly safe “hack." I did not find a code path in install that would invoke os.link, so it does seem to be build time only, during package (sdist).

@mriehl

This comment has been minimized.

Show comment
Hide comment
@mriehl

mriehl Feb 20, 2014

Member

I actually don't know if installation from PyPI is a problem either. It just made sense to me, that if packaging is an issue, then a pip install is bound to have problems too.

I'll find some time to test this out with vagrant today.

Member

mriehl commented Feb 20, 2014

I actually don't know if installation from PyPI is a problem either. It just made sense to me, that if packaging is an issue, then a pip install is bound to have problems too.

I'll find some time to test this out with vagrant today.

@mriehl

This comment has been minimized.

Show comment
Hide comment
@mriehl

mriehl Feb 20, 2014

Member

While I am able to reproduce the issue when running pyb publish it seems to occur only when creating a binary dist, so pip install works fine. In that case I'll add the workaround as disabled by default with a property to switch it on for local development.

Member

mriehl commented Feb 20, 2014

While I am able to reproduce the issue when running pyb publish it seems to occur only when creating a binary dist, so pip install works fine. In that case I'll add the workaround as disabled by default with a property to switch it on for local development.

@mriehl mriehl added the enhancement label Feb 20, 2014

@mriehl mriehl self-assigned this Feb 20, 2014

mriehl added a commit that referenced this issue Feb 20, 2014

allow denying hardlink capabilities from distutils
This resolves issue #56.
The related stdlib bug is http://bugs.python.org/issue8876 , and once it
is closed and shipped to all python versions that PyBuilder supports,
this can be reverted.
As @markmevans found out, it is not possible to build a bdist when the
underlying filesystem does not support hardlinks, because distutils
assumes that it always does.

Setting the property `distutils_issue8876_workaround_enabled` to True
will deny distutils/setuptools the ability to use os.link and result in
copying files instead of hardlinking them.

@mriehl mriehl closed this Feb 20, 2014

@mriehl

This comment has been minimized.

Show comment
Hide comment
@mriehl

mriehl Feb 20, 2014

Member

I have released 0.10.4 where it is possible to set distutils_issue8876_workaround_enabled to True in order to activate your workaround.

There is also a warning in the distutils plugin documentation.

Member

mriehl commented Feb 20, 2014

I have released 0.10.4 where it is possible to set distutils_issue8876_workaround_enabled to True in order to activate your workaround.

There is also a warning in the distutils plugin documentation.

@markmevans

This comment has been minimized.

Show comment
Hide comment
@markmevans

markmevans Feb 20, 2014

Contributor

Thanks! I really appreciate how responsive you are to input.

Contributor

markmevans commented Feb 20, 2014

Thanks! I really appreciate how responsive you are to input.

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