Skip to content
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

Merged
merged 5 commits into from Dec 2, 2014
Merged

Conversation

vadz
Copy link
Contributor

@vadz vadz commented Nov 26, 2014

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.

@vadz
Copy link
Contributor Author

vadz commented Nov 26, 2014

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(" ")
Copy link
Member

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?

Copy link
Contributor

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)

Copy link
Contributor Author

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!

@djmitche
Copy link
Member

Looks good. Needs the Travis failures fixed, though, and a release note added.

@vadz
Copy link
Contributor Author

vadz commented Nov 26, 2014

Adding release note is not a problem (it should go into docs/relnotes/index.rst, right?), but I'm stumped by Travis failures. I don't understand why do they happen nor why do they happen in only the two builds using TWISTED=latest. And I have trouble reproducing them locally because I don't seem to be able to run the tests at all:

[ERROR]: master/buildbot/test/integration/test_try_client.py

Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/twisted/trial/runner.py", line 672, in loadByNames
    things.append(self.findByName(name))
  File "/usr/lib/python2.6/dist-packages/twisted/trial/runner.py", line 481, in findByName
    return filenameToModule(name)
  File "/usr/lib/python2.6/dist-packages/twisted/trial/runner.py", line 94, in filenameToModule
    ret = reflect.namedAny(reflect.filenameToModuleName(fn))
  File "/usr/lib/python2.6/dist-packages/twisted/python/reflect.py", line 464, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "/usr/lib/python2.6/dist-packages/twisted/python/reflect.py", line 400, in _importAndCheckStack
    return __import__(importName)
  File ".../buildbot/master/buildbot/test/integration/test_try_client.py", line 20, in <module>
    from buildbot import master
  File ".../buildbot/master/buildbot/master.py", line 41, in <module>
    from buildbot.db import connector as dbconnector
  File ".../buildbot/master/buildbot/db/connector.py", line 31, in <module>
    from buildbot.db import model
  File ".../buildbot/master/buildbot/db/model.py", line 17, in <module>
    import migrate.versioning.repository
  File ".../migrate-0.2.2-py2.6.egg/migrate/versioning/repository.py", line 8, in <module>
    from migrate.versioning import script,exceptions,version
  File ".../migrate-0.2.2-py2.6.egg/migrate/versioning/script/__init__.py", line 1, in <module>
    from py import *
  File ".../migrate-0.2.2-py2.6.egg/migrate/versioning/script/py.py", line 2, in <module>
    from logsql import LogsqlFile
  File ".../migrate-0.2.2-py2.6.egg/migrate/versioning/script/logsql.py", line 2, in <module>
    from migrate.versioning import logengine
  File ".../migrate-0.2.2-py2.6.egg/migrate/versioning/logengine.py", line 260, in <module>
    LogEngineStrategy()
  File ".../migrate-0.2.2-py2.6.egg/migrate/versioning/logengine.py", line 253, in __init__
    super(LogEngineStrategy,self).__init__('logsql')
exceptions.TypeError: __init__() takes exactly 1 argument (2 given)
-------------------------------------------------------------------------------
Ran 1 tests in 0.001s

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>
@vadz
Copy link
Contributor Author

vadz commented Nov 29, 2014

Updated version with:

  1. Fix for Travis build failure (hopefully).
  2. One new commit improving the error message.
  3. A note in the change log.
  4. Manual parsing of the command string to honour the quotes in it.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 30, 2014

LGTM

@djmitche
Copy link
Member

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?

@vadz
Copy link
Contributor Author

vadz commented Nov 30, 2014

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 plink.exe (and I still can't use Cygwin ssh for some mysterious reason).

@djmitche
Copy link
Member

Then I think we should stick to some kind of standard unquoting, rather than inventing (and thus thoroughly documenting, testing, and supporting) our own. shlex could work, it's just that users will need to add more backslashes. If there's a Windows equivalent to shlex, we could use that equivalent on Windows.

@vadz
Copy link
Contributor Author

vadz commented Nov 30, 2014

Unfortunately there is no standard quoting mechanism under Windows. I think this one is the simplest possible and so the best. Using shlex definitely doesn't seem like a good idea, single quotes don't have any special meaning under Windows while they do for it and doubling backslashes is not great neither.

@djmitche
Copy link
Member

Right, but this needs to make sense for non-Windows users, too.

@vadz
Copy link
Contributor Author

vadz commented Nov 30, 2014

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 shlex under Windows while simply splitting on quotes under Windows, what do you think?

@djmitche
Copy link
Member

I think shlex on POSIX and your regular expression on Windows might be a good way to divide things.

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.
@vadz
Copy link
Contributor Author

vadz commented Nov 30, 2014

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 "ssh" option shouldn't be set anywhere), so how is it possible for the change to it to result in failure, and only with Python 2.6, too?

@benallard
Copy link
Contributor

That Travis issue is not your trouble. We're having quite an unstable test suite at the moment ...

@benallard
Copy link
Contributor

That's http://trac.buildbot.net/ticket/3081, I believe.

@sa2ajj
Copy link
Contributor

sa2ajj commented Dec 2, 2014

(I restarted the failed job in a hope that flaky test wouldn't fail this time)

@sa2ajj
Copy link
Contributor

sa2ajj commented Dec 2, 2014

Yes, it now passes. Flaky, flaky...

sa2ajj pushed a commit that referenced this pull request Dec 2, 2014
Miscellaneous fixes for try subcommand.
@sa2ajj sa2ajj merged commit a34f1f8 into buildbot:master Dec 2, 2014
@vadz vadz deleted the try-fixes branch December 3, 2014 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants