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
Miscellaneous fixes for try subcommand. #1391
Conversation
Sorry, I had somehow branched off a wrong place initially. Rebased on the latest master now. |
argv = ["ssh", tryhost, | ||
buildbotbin, "tryserver", "--jobdir", trydir] | ||
# allow passing options in ssh command too | ||
argv = ssh_command.split(" ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this be problematic on Windows if SSH is at, say: C:\Program Files (x86)\WinSSH\ssh.exe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is a concern, python has the shlex.split
function. (https://docs.python.org/2/library/shlex.html#shlex.split)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I think it is a concern (personally I never use paths with spaces as it's just a recipe for disaster, so not for me, but I'm pretty sure this would bite others) and shlex
doesn't really help because it treats backslashes as escape characters, meaning that it totally mangles Windows paths.
So either this option would need to be documented to escape backslashes or I'd have to write my own tokenization code. Neither seems really appealing.
Would anybody have any other ideas? TIA!
Looks good. Needs the Travis failures fixed, though, and a release note added. |
Adding release note is not a problem (it should go into
and I don't have time to debug this right now.. Any ideas about what could be causing the Travis failures would be very welcome. |
If the current branch has no tracking branch, use master as the base revision, otherwise the branch itself was used and we were left with an empty diff if there were no uncommitted changes. Signed-off-by: Vadim Zeitlin <vadim@zeitlins.org>
The error message "object of type 'NoneType' has no len()" given in this case before was particularly cryptic, provide something more intelligible if the diff turns out to be empty.
Using just "ssh" doesn't work under Windows where the native CreateProcess() function used by spawnProcess() requires the full path as its argument, so using ssh connection method simply always failed before. Adding a "ssh_command" config option allowing to explicitly specify the command to use might be a good idea as well, but for now just take the first "ssh" in PATH. Signed-off-by: Vadim Zeitlin <vadim@zeitlins.org>
This is especially useful under Windows, where something other than ssh[.exe] is often used (e.g. plink from PuTTY). Signed-off-by: Vadim Zeitlin <vadim@zeitlins.org>
Updated version with:
|
LGTM |
I'm sorry I was away from this PR for a while. I agree that parsing an 'ssh' with spaces in it is hard, and it's only done to allow ssh with arguments. The solution currently in this PR admits it's still imperfect. What if we just didn't support passing arguments to the custom SSH command? |
It would be really unfortunate to not allow this, it would force me to use some intermediate wrapper (batch file? ) as I do need to pass some options to |
Then I think we should stick to some kind of standard unquoting, rather than inventing (and thus thoroughly documenting, testing, and supporting) our own. |
Unfortunately there is no standard quoting mechanism under Windows. I think this one is the simplest possible and so the best. Using |
Right, but this needs to make sense for non-Windows users, too. |
Oops, I guess I forgot about it because I was thinking that specifying a custom ssh command would be much more common under Windows than under Unix. The simplest solution would be to use |
I think |
This method can't be used under Windows as it handles backslashes specially while they are just path separators under Windows, but we should still use it under the other systems to provide familiar semantics there.
Sorry for the typo, I meant the same thing as you, of course, not its exact converse that I actually wrote. Unfortunately the patch implementing this behaviour triggered a new Travis failure and I don't see, again, what is the problem here -- this code should never be executed by the test suite anyhow (as |
That Travis issue is not your trouble. We're having quite an unstable test suite at the moment ... |
That's http://trac.buildbot.net/ticket/3081, I believe. |
(I restarted the failed job in a hope that flaky test wouldn't fail this time) |
Yes, it now passes. Flaky, flaky... |
Miscellaneous fixes for try subcommand.
The first commit is independent of the rest, the last one depends on the second one and could be squashed with it but I preferred to keep them separate just in case.
The commit messages should hopefully be self-explanatory.