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

Fallback to system modules if vendorized one do not exist #1745

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
None yet
3 participants
@athmane
Contributor

athmane commented May 12, 2018

PS. this will help with fabric2 packaging.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet May 14, 2018

Member

Thanks! I believe we do this in Invoke but I didn't get around to applying it to Fabric.

Member

bitprophet commented May 14, 2018

Thanks! I believe we do this in Invoke but I didn't get around to applying it to Fabric.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet May 14, 2018

Member

Hm. Master or 2.0...master or 2.0...feels like arguably a bug so gonna backport.

Member

bitprophet commented May 14, 2018

Hm. Master or 2.0...master or 2.0...feels like arguably a bug so gonna backport.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet May 14, 2018

Member

Also I'd love to test this but it seems more trouble than it's worth; any test would have to expect one setup or the other (existence of standalone six, or lack, for example) or be parameterized at runtime. And we've got bigger fish to fry right now.

Member

bitprophet commented May 14, 2018

Also I'd love to test this but it seems more trouble than it's worth; any test would have to expect one setup or the other (existence of standalone six, or lack, for example) or be parameterized at runtime. And we've got bigger fish to fry right now.

@rpkilby

This comment has been minimized.

Show comment
Hide comment
@rpkilby

rpkilby May 14, 2018

Contributor

Out of curiosity, what's the reason to do this? In what cases would the vendored package imports not be present?

Contributor

rpkilby commented May 14, 2018

Out of curiosity, what's the reason to do this? In what cases would the vendored package imports not be present?

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet May 14, 2018

Member

@rpkilby There was a ticket for this for Invoke at one point, the tl;dr is that some OS level packages don't agree with vendoring and prefer to distribute a copy of the code that expects a "regularly" installed copy of those deps.

So e.g. Debian might yank out vendor/ and add metadata depending on say python-six (which shows up in the Python import system as just regular ol six) and at that point we'd break without this change.

"Normal" installs from pip will always end up using the vendored copy, but with this change in play, the code can be used by those OS downstream packages as-is without them having to apply additional patches.

Note that this is subtly different from the inverse, preferring the public copy of a module and only falling back to the vendored one, which has its own issues and is not desirable.

Member

bitprophet commented May 14, 2018

@rpkilby There was a ticket for this for Invoke at one point, the tl;dr is that some OS level packages don't agree with vendoring and prefer to distribute a copy of the code that expects a "regularly" installed copy of those deps.

So e.g. Debian might yank out vendor/ and add metadata depending on say python-six (which shows up in the Python import system as just regular ol six) and at that point we'd break without this change.

"Normal" installs from pip will always end up using the vendored copy, but with this change in play, the code can be used by those OS downstream packages as-is without them having to apply additional patches.

Note that this is subtly different from the inverse, preferring the public copy of a module and only falling back to the vendored one, which has its own issues and is not desirable.

@rpkilby

This comment has been minimized.

Show comment
Hide comment
@rpkilby

rpkilby May 14, 2018

Contributor

So e.g. Debian might yank out vendor/ and add metadata depending on say python-six (which shows up in the Python import system as just regular ol six) and at that point we'd break without this change.

Interesting, I didn't realize that this occurred. Thanks for the info.

Contributor

rpkilby commented May 14, 2018

So e.g. Debian might yank out vendor/ and add metadata depending on say python-six (which shows up in the Python import system as just regular ol six) and at that point we'd break without this change.

Interesting, I didn't realize that this occurred. Thanks for the info.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet May 14, 2018

Member

If you're curious for more, go git blame in Invoke and you'll presumably end up at the old ticket about this, which might have more info. But that's my recollection.

Member

bitprophet commented May 14, 2018

If you're curious for more, go git blame in Invoke and you'll presumably end up at the old ticket about this, which might have more info. But that's my recollection.

@bitprophet bitprophet merged commit 220baa9 into fabric:master May 14, 2018

1 check passed

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

bitprophet added a commit that referenced this pull request May 14, 2018

@athmane

This comment has been minimized.

Show comment
Hide comment
@athmane

athmane May 14, 2018

Contributor

@rpkilby reasoning behind this is in:

pyinvoke/invoke#204
pyinvoke/invoke#412

Contributor

athmane commented May 14, 2018

@rpkilby reasoning behind this is in:

pyinvoke/invoke#204
pyinvoke/invoke#412

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