linux: copy: Recognize any two SubprocessConnector machines as same-host - #131
Open
kvmajo wants to merge 1 commit into
Open
linux: copy: Recognize any two SubprocessConnector machines as same-host#131kvmajo wants to merge 1 commit into
kvmajo wants to merge 1 commit into
Conversation
linux.copy()'s same-host detection only matched when one machine's
class was a subclass of the other's:
if isinstance(p1.host, p2.host.__class__) or isinstance(p2.host, p1.host.__class__):
If a project defines two separate machine classes that both connect
via connector.SubprocessConnector (i.e. both actually run on
localhost) but don't inherit from each other, copy() falls through
every SSH/Paramiko-specific branch and raises NotImplementedError,
even though both machines run on the same filesystem and a plain cp
would work. Fix this by also recognizing any two SubprocessConnector
instances as being on the same host, regardless of their exact class.
While testing this, a second, independent bug in the same code path
surfaced: even when the isinstance check above already matched (e.g.
literally the same class), the fast path still raised WrongHostError
for any two machine instances that aren't clones of one another. This
is because `p2_w1 = linux.Path(p1.host, p2)` implicitly requires
`p2.host == p1.host`, and Machine.__eq__ only considers clones of the
same original equal - not just "same/compatible class". So the
same-host fast path only ever worked when p1.host and p2.host were
already the exact same object to begin with, making the isinstance
check mostly dead code in practice.
Fixed by reinterpreting p2's raw path via p2._local_str() instead of
wrapping the Path object itself, since by the time we reach this
branch same-host compatibility has already been established by the
outer condition.
Fixes: Rahix#129
Signed-off-by: Martin Jocic <martin.jocic@kvaser.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #129.
linux.copy()'s same-host detection only matched when one machine'sclass was a subclass of the other's:
If a project defines two separate machine classes that both connect
via
connector.SubprocessConnector(i.e. both actually run onlocalhost) but don't inherit from each other,
copy()falls throughevery SSH/Paramiko-specific branch and raises
NotImplementedError,even though both machines run on the same filesystem and a plain
cpwould work. Fixed by also recognizing any two
SubprocessConnectorinstances as being on the same host, regardless of their exact class.
A second bug found while testing
While testing this, a second, independent bug in the same code path
surfaced: even when the isinstance check above already matched (e.g.
literally the same class), the fast path still raised
WrongHostErrorfor any two machine instances that aren't clones ofone another. This is because
p2_w1 = linux.Path(p1.host, p2)implicitly requires
p2.host == p1.host, andMachine.__eq__onlyconsiders clones of the same original equal — not just
"same/compatible class". So the same-host fast path only ever worked
when
p1.hostandp2.hostwere already the exact same object tobegin with, making the isinstance check mostly dead code in practice.
Fixed by reinterpreting
p2's raw path viap2._local_str()insteadof wrapping the
Pathobject itself, since by the time we reach thisbranch same-host compatibility has already been established by the
outer condition.
Testing
pre-commit's pinnedblack,flake8, andmypyall pass on thechanged file.
master:SubprocessConnector-based classes:NotImplementedError.WrongHostError.Both succeed on this branch.
selftest/(excluding the subset that needs a realsshd, whichthis sandbox doesn't have) has identical pass/fail counts before and
after this change: 5 failed / 104 passed on both
masterand thisbranch.