Merge lp:~mikemc/ubuntuone-client/use-dirspec-get-cmdline into lp:ubuntuone-client

[Mike McCracken]

- Use dirspec.get_program_path instead of SSO client to find path to syncdaemon executable.

- Use dirspec.get_program_path instead of SSO client to find path to syncdaemon executable.

In detail:

- add utils.py with get_sd_bin_cmd to wrap dirspec's get_program_path with appropriate fallback and app names args, and add 'python' if frozen on mac or win (to use buildout generated python).
- change platform/ipc/perspective_broker to use get_sd_bin_cmd.
- change platform/tools/perspective_broker to use get_sd_bin_cmd, although I believe it is never called.
- removed platform/tools/darwin.py and platform/tools/windows.py and associated tests, they are now unused.

Because of fsevents not quite working yet, with current trunk, not all tests pass on darwin. The following test paths touch the code I've changed and added:

./run-mac-tests tests/platform/tools
./run-mac-tests tests/platform/ipc
./run-mac-tests tests/test_utils.py

To run the tests, you'll need to tweak your PYTHONPATH to include dirspec trunk, and lp:~diegosarmentero/+junk/python-macfsevents (build it and put the build/lib.blahblah path on your PYTHONPATH)

[Brian Curtin (brian.curtin)]

Overall it looks reasonable to me.

515 + if getattr(sys, 'frozen', None) is None \
516 + and sys.platform in ('win32', 'darwin'):

I have one style nit to pick. Rather than breaking the line with '\', I think it's more common to wrap the entire condition in parentheses.

review: Needs Fixing

[lp:~mikemc/ubuntuone-client/use-dirspec-get-cmdline]

1279. By Mike McCracken on 2012-07-12

use saner syntax for multi-line conditions

[Brian Curtin (brian.curtin)]

review: Approve

[dobey (dobey)]

=== added file 'ubuntuone/utils.py'

This might be better as ubuntuone/syncdaemon/utils.py, as it's fairly specific to syncdaemon here, and not a general all-of-ubuntuone thing.

I know that clientdefs.py is there now, and a logger.py, but I actually think everything in ubuntuone-client needs to be moved under ubuntuone/syncdaemon/; just to keep things clear when looking at the code structure. Would also be nice to separate the internal bits from actual library code, and only install actual library code to /usr/lib/pythonwhatever, while installing the internal code bits to a private directory somewhere. But for now, let's keep new files under syncdaemon. A man can dream.

[dobey (dobey)]

review: Needs Fixing

[Diego Sarmentero (diegosarmentero)]

./tests/test_utils.py:
    31: 'os' imported but unused

review: Needs Fixing

[lp:~mikemc/ubuntuone-client/use-dirspec-get-cmdline]

1280. By Mike McCracken on 2012-07-13

remove unused import

1281. By Mike McCracken on 2012-07-13

merge with trunk

1282. By Mike McCracken on 2012-07-13

move utils module into syncdaemon

[dobey (dobey)]

My main concern looks addressed, but I can't test this on osx/win, so abstaining now.

review: Abstain

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

=== removed file 'tests/platform/tools/test_darwin.py'
--- tests/platform/tools/test_darwin.py	2012-05-24 15:09:34 +0000
+++ tests/platform/tools/test_darwin.py	1970-01-01 00:00:00 +0000
@@ -1,52 +0,0 @@
-# -*- coding: utf-8 -*-
-#
-# Copyright 2012 Canonical Ltd.
-#
-# This program is free software: you can redistribute it and/or modify it
-# under the terms of the GNU General Public License version 3, as published
-# by the Free Software Foundation.
-#
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranties of
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-# PURPOSE.  See the GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License along
-# with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-# In addition, as a special exception, the copyright holders give
-# permission to link the code of portions of this program with the
-# OpenSSL library under certain conditions as described in each
-# individual source file, and distribute linked combinations
-# including the two.
-# You must obey the GNU General Public License in all respects
-# for all of the code used other than OpenSSL.  If you modify
-# file(s) with this exception, you may extend this exception to your
-# version of the file(s), but you are not obligated to do so.  If you
-# do not wish to do so, delete this exception statement from your
-# version.  If you delete this exception statement from all source
-# files in the program, then also delete it here.
-"""Tests for the tools windows code."""
-
-from twisted.internet import defer
-from twisted.trial.unittest import TestCase
-
-from ubuntuone.platform.tools import perspective_broker, darwin
-
-
-class TestStartSyncdaemon(TestCase):
-    """Test the process of launching sd."""
-
-    @defer.inlineCallbacks
-    def setUp(self):
-        yield super(TestStartSyncdaemon, self).setUp()
-        self.patch(perspective_broker.UbuntuOneClient, "connect",
-                lambda _: defer.Deferred())
-        self.sdtool = perspective_broker.SyncDaemonToolProxy()
-        self.calls = {}
-
-    def test_start_missing_exe(self):
-       """Test starting the service when the exe is missing."""
-       # file is missing
-       self.patch(darwin.os.path, 'exists', lambda f: False)
-       self.assertFailure(self.sdtool.start(), Exception)

=== removed file 'tests/platform/tools/test_windows.py'
--- tests/platform/tools/test_windows.py	2012-05-24 15:09:34 +0000
+++ tests/platform/tools/test_windows.py	1970-01-01 00:00:00 +0000
@@ -1,64 +0,0 @@
-# -*- coding: utf-8 -*-
-#
-# Copyright 2012 Canonical Ltd.
-#
-# This program is free software: you can redistribute it and/or modify it
-# under the terms of the GNU General Public License version 3, as published
-# by the Free Software Foundation.
-#
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranties of
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-# PURPOSE.  See the GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License along
-# with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-# In addition, as a special exception, the copyright holders give
-# permission to link the code of portions of this program with the
-# OpenSSL library under certain conditions as described in each
-# individual source file, and distribute linked combinations
-# including the two.
-# You must obey the GNU General Public License in all respects
-# for all of the code used other than OpenSSL.  If you modify
-# file(s) with this exception, you may extend this exception to your
-# version of the file(s), but you are not obligated to do so.  If you
-# do not wish to do so, delete this exception statement from your
-# version.  If you delete this exception statement from all source
-# files in the program, then also delete it here.
-"""Tests for the tools windows code."""
-
-import sys
-
-from twisted.internet import defer
-from twisted.trial.unittest import TestCase
-
-from ubuntuone.platform.tools import perspective_broker, windows
-
-# ugly trick to stop pylint for complaining about
-# WindowsError on Linux
-if sys.platform != 'win32':
-    WindowsError = None
-
-
-class TestStartSyncdaemon(TestCase):
-    """Test the process of launching sd."""
-
-    @defer.inlineCallbacks
-    def setUp(self):
-        yield super(TestStartSyncdaemon, self).setUp()
-        self.patch(perspective_broker.UbuntuOneClient, "connect",
-                lambda _: defer.Deferred())
-        self.sdtool = perspective_broker.SyncDaemonToolProxy()
-        self.calls = {}
-
-    def test_start_missing_exe(self):
-       """Test starting the service when the exe is missing."""
-       # file is missing
-       self.patch(windows.os.path, 'exists', lambda f: False)
-       key = 'key'
-       path = 'path/to/exe'
-       self.patch(windows, 'OpenKey', lambda k,p: key)
-       self.patch(windows, 'QueryValueEx', lambda k,p: path)
-
-       self.assertFailure(self.sdtool.start(), WindowsError)

=== added file 'tests/syncdaemon/test_utils.py'
--- tests/syncdaemon/test_utils.py	1970-01-01 00:00:00 +0000
+++ tests/syncdaemon/test_utils.py	2012-07-13 17:12:18 +0000
@@ -0,0 +1,79 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# In addition, as a special exception, the copyright holders give
+# permission to link the code of portions of this program with the
+# OpenSSL library under certain conditions as described in each
+# individual source file, and distribute linked combinations
+# including the two.
+# You must obey the GNU General Public License in all respects
+# for all of the code used other than OpenSSL.  If you modify
+# file(s) with this exception, you may extend this exception to your
+# version of the file(s), but you are not obligated to do so.  If you
+# do not wish to do so, delete this exception statement from your
+# version.  If you delete this exception statement from all source
+# files in the program, then also delete it here.
+"""Test utility functions."""
+
+import sys
+
+from twisted.trial.unittest import TestCase
+
+import ubuntuone.syncdaemon.utils
+
+
+class UtilsTestCase(TestCase):
+    """Test utils."""
+
+    def test_get_sd_bin_cmd_src_nonlinux(self):
+        """Test that we use the buildout python running from source."""
+        self.patch(sys, 'platform', 'darwin')
+        self.patch(ubuntuone.syncdaemon.utils, 'get_program_path',
+                   lambda _, *args, **kwargs: 'test-path')
+        args = ubuntuone.syncdaemon.utils.get_sd_bin_cmd()
+        self.assertEqual(len(args), 2)
+        self.assertEqual(args[0], 'python')
+
+    def test_get_sd_bin_cmd_src_linux(self):
+        """Test that we DO NOT use the buildout python running from source."""
+        self.patch(sys, 'platform', 'linux2')
+        self.patch(ubuntuone.syncdaemon.utils, 'get_program_path',
+                   lambda _, *args, **kwargs: 'test-path')
+        args = ubuntuone.syncdaemon.utils.get_sd_bin_cmd()
+        self.assertEqual(len(args), 1)
+        self.assertEqual(args[0], 'test-path')
+
+    def test_get_sd_bin_cmd_installed_nonlinux(self):
+        """Test that we DO NOT use the buildout python when installed."""
+        sys.frozen = True
+        self.addCleanup(delattr, sys, 'frozen')
+        self.patch(sys, 'platform', 'darwin')
+        self.patch(ubuntuone.syncdaemon.utils, 'get_program_path',
+                   lambda _, *args, **kwargs: 'test-path')
+        args = ubuntuone.syncdaemon.utils.get_sd_bin_cmd()
+        self.assertEqual(len(args), 1)
+        self.assertEqual(args[0], 'test-path')
+
+    def test_get_sd_bin_cmd_installed_linux(self):
+        """Test that we DO NOT use the buildout python when installed."""
+        sys.frozen = True
+        self.addCleanup(delattr, sys, 'frozen')
+        self.patch(sys, 'platform', 'linux2')
+        self.patch(ubuntuone.syncdaemon.utils, 'get_program_path',
+                   lambda _, *args, **kwargs: 'test-path')
+        args = ubuntuone.syncdaemon.utils.get_sd_bin_cmd()
+        self.assertEqual(len(args), 1)
+        self.assertEqual(args[0], 'test-path')

=== modified file 'ubuntuone/platform/ipc/__init__.py'
--- ubuntuone/platform/ipc/__init__.py	2012-05-22 15:03:52 +0000
+++ ubuntuone/platform/ipc/__init__.py	2012-07-13 17:12:18 +0000
@@ -34,9 +34,6 @@
 if sys.platform in ('win32', 'darwin'):
     from ubuntuone.platform.ipc import perspective_broker as source
     ExternalInterface = source.IPCInterface
-elif sys.platform == "darwin":
-    from ubuntuone.platform.ipc import darwin as source
-    ExternalInterface = source.IPCInterface
 else:
     from ubuntuone.platform.ipc import linux as source
     ExternalInterface = source.DBusInterface

=== removed file 'ubuntuone/platform/ipc/darwin.py'
--- ubuntuone/platform/ipc/darwin.py	2012-05-15 14:56:44 +0000
+++ ubuntuone/platform/ipc/darwin.py	1970-01-01 00:00:00 +0000
@@ -1,44 +0,0 @@
-# -*- coding: utf-8 -*-
-#
-# Copyright 2012 Canonical Ltd.
-#
-# This program is free software: you can redistribute it and/or modify it
-# under the terms of the GNU General Public License version 3, as published
-# by the Free Software Foundation.
-#
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranties of
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-# PURPOSE.  See the GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License along
-# with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-# In addition, as a special exception, the copyright holders give
-# permission to link the code of portions of this program with the
-# OpenSSL library under certain conditions as described in each
-# individual source file, and distribute linked combinations
-# including the two.
-# You must obey the GNU General Public License in all respects
-# for all of the code used other than OpenSSL.  If you modify
-# file(s) with this exception, you may extend this exception to your
-# version of the file(s), but you are not obligated to do so.  If you
-# do not wish to do so, delete this exception statement from your
-# version.  If you delete this exception statement from all source
-# files in the program, then also delete it here.
-"""IPC implementation for MAC OS."""
-
-import logging
-
-# Disable the "Invalid Name" check here, as we have lots of DBus style names
-# pylint: disable-msg=C0103
-
-logger = logging.getLogger("ubuntuone.SyncDaemon.DBus")
-
-
-def is_already_running(bus=None):
-    """Return if the sd is running by trying to get the port."""
-
-
-class IPCInterface(object):
-    """Holder of all exposed objects."""

=== modified file 'ubuntuone/platform/ipc/perspective_broker.py'
--- ubuntuone/platform/ipc/perspective_broker.py	2012-05-22 14:07:55 +0000
+++ ubuntuone/platform/ipc/perspective_broker.py	2012-07-13 17:12:18 +0000
@@ -43,7 +43,8 @@
     Referenceable,
     Root,
 )
-from ubuntu_sso.main import get_activation_cmdline
+
+from ubuntuone.syncdaemon.utils import get_sd_bin_cmd
 from ubuntu_sso.utils.tcpactivation import (
     ActivationClient,
     ActivationConfig,
@@ -71,7 +72,9 @@
     """Get the configuration to activate the sso service."""
     description = DescriptionFactory()
     service_name = SD_SERVICE_NAME
-    cmdline = get_activation_cmdline(service_name)
+
+    cmdline = get_sd_bin_cmd()
+
     return ActivationConfig(service_name, cmdline, description)
 
 

=== removed file 'ubuntuone/platform/tools/darwin.py'
--- ubuntuone/platform/tools/darwin.py	2012-05-24 09:43:19 +0000
+++ ubuntuone/platform/tools/darwin.py	1970-01-01 00:00:00 +0000
@@ -1,49 +0,0 @@
-# -*- coding: utf-8 -*-
-#
-# Copyright 2012 Canonical Ltd.
-#
-# This program is free software: you can redistribute it and/or modify it
-# under the terms of the GNU General Public License version 3, as published
-# by the Free Software Foundation.
-#
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranties of
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-# PURPOSE.  See the GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License along
-# with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-# In addition, as a special exception, the copyright holders give
-# permission to link the code of portions of this program with the
-# OpenSSL library under certain conditions as described in each
-# individual source file, and distribute linked combinations
-# including the two.
-# You must obey the GNU General Public License in all respects
-# for all of the code used other than OpenSSL.  If you modify
-# file(s) with this exception, you may extend this exception to your
-# version of the file(s), but you are not obligated to do so.  If you
-# do not wish to do so, delete this exception statement from your
-# version.  If you delete this exception statement from all source
-# files in the program, then also delete it here.
-"""SyncDaemon Tools darwin implementation."""
-
-import errno
-import os
-
-import ubuntuone
-
-
-def get_sd_install_path():
-    """Return the path where the sd script was installed."""
-    # TODO: We need to find a nicer way to find paths on darwin when since this
-    # method will probably not work once the application has been packaged.
-    ubuntuone_pkg_dir = os.path.dirname(os.path.dirname(ubuntuone.__file__))
-    ubuntuone_bin_dir = os.path.join(ubuntuone_pkg_dir, 'bin')
-    ubuntuone_bin = os.path.join(ubuntuone_bin_dir,
-                    'ubuntuone-syncdaemon')
-    if not os.path.exists(ubuntuone_bin):
-        raise Exception(errno.ENOENT,
-            'Could not start syncdaemon: File not found %s' % ubuntuone_bin)
-    return ubuntuone_bin
-

=== modified file 'ubuntuone/platform/tools/perspective_broker.py'
--- ubuntuone/platform/tools/perspective_broker.py	2012-05-24 09:43:19 +0000
+++ ubuntuone/platform/tools/perspective_broker.py	2012-07-13 17:12:18 +0000
@@ -29,28 +29,17 @@
 """SyncDaemon Tools."""
 
 import subprocess
-import sys
 
 from twisted.internet import defer
 
 from ubuntuone.platform.ipc.perspective_broker import is_already_running
 from ubuntuone.platform.ipc.ipc_client import UbuntuOneClient
+from ubuntuone.syncdaemon.utils import get_sd_bin_cmd
 
 
 # make pyflakes happy since we can't disable the warning
 is_already_running = is_already_running
 
-if sys.platform == 'win32':
-    from ubuntuone.platform.tools import windows
-    get_sd_install_path = windows.get_sd_install_path
-elif sys.platform == 'darwin':
-    from ubuntuone.platform.tools import darwin
-    get_sd_install_path = darwin.get_sd_install_path
-else:
-    # add an implementation that will result in os.path.exists always to be
-    # false
-    get_sd_install_path = lambda: None
-
 
 class IPCError(Exception):
     """An IPC specific error signal."""
@@ -175,8 +164,8 @@
     def start(self):
         """Start syncdaemon, should *not* be running."""
         try:
-            path = get_sd_install_path()
+            cmd = get_sd_bin_cmd()
         except Exception, e:
             defer.fail(e)
-        p = subprocess.Popen([path])
+        p = subprocess.Popen(cmd)
         return defer.succeed(p)

=== removed file 'ubuntuone/platform/tools/windows.py'
--- ubuntuone/platform/tools/windows.py	2012-05-24 09:43:19 +0000
+++ ubuntuone/platform/tools/windows.py	1970-01-01 00:00:00 +0000
@@ -1,56 +0,0 @@
-# -*- coding: utf-8 -*-
-#
-# Copyright 2012 Canonical Ltd.
-#
-# This program is free software: you can redistribute it and/or modify it
-# under the terms of the GNU General Public License version 3, as published
-# by the Free Software Foundation.
-#
-# This program is distributed in the hope that it will be useful, but
-# WITHOUT ANY WARRANTY; without even the implied warranties of
-# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
-# PURPOSE.  See the GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License along
-# with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-# In addition, as a special exception, the copyright holders give
-# permission to link the code of portions of this program with the
-# OpenSSL library under certain conditions as described in each
-# individual source file, and distribute linked combinations
-# including the two.
-# You must obey the GNU General Public License in all respects
-# for all of the code used other than OpenSSL.  If you modify
-# file(s) with this exception, you may extend this exception to your
-# version of the file(s), but you are not obligated to do so.  If you
-# do not wish to do so, delete this exception statement from your
-# version.  If you delete this exception statement from all source
-# files in the program, then also delete it here.
-"""SyncDaemon Tools windows implementation."""
-
-import errno
-import os
-import sys
-
-from _winreg import OpenKey, HKEY_LOCAL_MACHINE, QueryValueEx
-
-
-U1_REG_PATH = r'Software\\Ubuntu One'
-SD_INSTALL_PATH = 'SyncDaemonInstallPath'
-
-# ugly trick to stop pylint for complaining about
-# WindowsError on Linux
-if sys.platform != 'win32':
-    WindowsError = None
-
-
-def get_sd_install_path():
-    """Return the path where the sd script was installed."""
-    # look in the reg to find the path of the .exe to be executed
-    # to launch the sd on windows
-    key = OpenKey(HKEY_LOCAL_MACHINE, U1_REG_PATH)
-    path = QueryValueEx(key, SD_INSTALL_PATH)[0]
-    if not os.path.exists(path):
-        raise WindowsError(errno.ENOENT,
-            'Could not start syncdaemon: File not found %s' % path)
-    return path

=== added file 'ubuntuone/syncdaemon/utils.py'
--- ubuntuone/syncdaemon/utils.py	1970-01-01 00:00:00 +0000
+++ ubuntuone/syncdaemon/utils.py	2012-07-13 17:12:18 +0000
@@ -0,0 +1,55 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# In addition, as a special exception, the copyright holders give
+# permission to link the code of portions of this program with the
+# OpenSSL library under certain conditions as described in each
+# individual source file, and distribute linked combinations
+# including the two.
+# You must obey the GNU General Public License in all respects
+# for all of the code used other than OpenSSL.  If you modify
+# file(s) with this exception, you may extend this exception to your
+# version of the file(s), but you are not obligated to do so.  If you
+# do not wish to do so, delete this exception statement from your
+# version.  If you delete this exception statement from all source
+# files in the program, then also delete it here.
+"""Utility functions for ubuntuone client."""
+
+import os
+import sys
+
+from dirspec.utils import get_program_path
+
+DARWIN_APP_NAMES = {'ubuntuone-syncdaemon': 'UbuntuOne Syncdaemon.app'}
+
+
+def get_sd_bin_cmd():
+    """Get cmd + args to launch syncdaemon executable."""
+    syncdaemon_dir = os.path.dirname(__file__)
+    ubuntuone_dir = os.path.dirname(syncdaemon_dir)
+    tree_dir = os.path.dirname(ubuntuone_dir)
+    fallback_dirs = [os.path.join(tree_dir, 'bin')]
+    path = get_program_path('ubuntuone-syncdaemon',
+                            fallback_dirs=fallback_dirs,
+                            app_names=DARWIN_APP_NAMES)
+    cmd_args = [path]
+
+    # adjust cmd for platforms using buildout-generated python
+    # wrappers
+    if (getattr(sys, 'frozen', None) is None
+        and sys.platform in ('win32', 'darwin')):
+        cmd_args.insert(0, 'python')
+    return cmd_args