Merge lp:~mikemc/dirspec/add-exefind into lp:dirspec

[Mike McCracken]

- Add function for getting executable command lines across projects and platforms and packagings (LP: #1021833)

- Add function for getting executable command lines across projects and platforms and packagings (LP: #1021833)

[dobey (dobey)]

First thing I notice this will need is an __all__ to only export the public method.

Also, I don't like the naming, and I think we should probably have a better fallback for Linux/BSD/etc… We'll also need to deal with varying python executable name, such as python3 instead of python.

review: Needs Fixing

[lp:~mikemc/dirspec/add-exefind]

9. By Mike McCracken on 2012-07-09

Add support for installed linux using generated-at-install module.constants to find BIN_DIR
Avoid using 'python ' + exe on non-mac/win
Refactor tests.

[Mike McCracken (mikemc)]

rev 9 addresses the __all__ attr, and the issue of a linux fallback.

On linux, the difference between installed and not installed is not the sys.frozen attribute.
I've adapted the approach from ubuntu_sso.utils.get_bin_dir:
On linux, if we are running from source, we find the path relative to the module, eg. srctree/bin next to srctree/ubuntu_sso. If there's something there, we use it, otherwise we look at the generated constants.py that is generated when setup.py is run.

Clients of this code other than ubuntu-sso-client would need to add a constants.py with a BIN_DIR attribute to work the same way. However, only sso-client currently launches any processes using Popen on linux - the other projects use DBus.

I am still open to suggestions about what to do wrt. the python exe name - I don't completely understand when & why that'll change.

[Mike McCracken (mikemc)]

Also, please elaborate on your naming concerns. I'm open to suggestions!

[lp:~mikemc/dirspec/add-exefind]

10. By Mike McCracken on 2012-07-09

Remove an unused line

[dobey (dobey)]

I don't like the name, because it's platform-specific, as there are no .exes on OSX or Linux generally (C# and things under WINE, notwithstanding). Also, dirspec already has a utils.py module, and this is but a utility function, albeit with a few additional private helper functions.

Also, I'm really not liking the API, now that I've thought about it more. The idea of having a special module in one's code that gets passed in, and must have specific platform-dependent constants in it, is rather nasty. I think all these things the code is poking at a module for, would be better passed in as kwargs on the entry point method.

As for the python prefix issue, we can't just prepend it anyway. It won't work very well for things which aren't written in Python. If someone else wanted to use the library in their code to launch a non-python application, for whatever reason, prefixing python here would break that. So I'd rather avoid us having that here if possible. It would be nice if we could find some other way to solve this problem for running programs from tree, on Windows/OS X. Maybe I don't understand that problem fully enough yet, and need to have it explained better to me, but it seems like a separate problem from the need to avoid duplicating the code to find the files on disk to run.

[Mike McCracken (mikemc)]

naming - no prob moving it to utils. call it utils.get_cmdline

originally wanted an API as smart as possible, otherwise we're not removing much complexity from the callsites.
But I don't disagree with the comments about the module pokery. It's
not a very intuitive API.

So, here's what it'd look like if we avoided module pokery:

We need to tell it the name of the executable we're looking for, and
three extra things:

- what directory to look in , or near (best we can do is pass in
       __file__ from something)
-- Unless we give it more info, like I was by passing the module, all
   it can do is look for a bin/ in cwd, then cwd/.. , etc...

- only for darwin: what the name of the sub-app will be if packaged.

- only for linux: we need to give it a way to get the compiled-in
  BIN_DIR constant for SSO, which is at ubuntu_sso.constants.BIN_DIR

Giving all this explicitly (and we always need to give all the args,
because the same code should work on all platforms) makes a kind of
messy call site:

cmdline = get_cmdline('ubuntuone-syncdaemon',
  ubuntuone.syncdaemon.__file__,
  "Ubuntu One Syncdaemon.app") # no constants module

cmdline = get_cmdline('ubuntu-sso-login-qt,
  ubuntu_sso.__file__,
  "Ubuntu SSO.app", # or, probably DARWIN_APP_NAMES['ubuntu-sso-login-qt']
  dir_constants=ubuntu_sso.constants)

# here's what the function's signature would look like, with some overly descriptive names that I didn't think about too hard:

def get_cmdline(executable_name, some_path_in_same_project_tree, darwin_subapp_name, linux_installed_constants_module=None)

[Mike McCracken (mikemc)]

Regarding prepending 'python ', the issue is that the scripts now have
an absolute path to /usr/bin/python, but on windows and darwin we have
a buildout-generated python wrapper that sets up PYTHONPATH to point
to the eggs in the buildout directory, etc, so we need to use that.

One option would be to avoid using the buildout python, and either
just set PYTHONPATH to include all the eggs while developing, or use
virtualenv, or something. However, that's a rather big change.

If we want to stick with the buildout python, we need to just prepend
'python' to the cmdline and use Popen(shell=True).

All that said, I think it'd be fine to move that out of this general
library function. There'd just be a bit of extra code after we call it
each time:

cmdline = get_cmdline(blah)
if getattr(sys, "frozen", None) is None and
   sys.platform in ('win32','darwin'):
   # then we are in a source tree, using the buildout python wrapper,
   # and we need to make sure we run it using that, so that its path
   # makes sense.
   cmdline = 'python ' + cmdline

for ease of writing tests, we might also want to wrap that in a
function on the client end (ie, not in dirspec)

[lp:~mikemc/dirspec/add-exefind]

11. By Mike McCracken on 2012-07-09

- Move get_cmdline into utils.py

- Don't prepend 'python ' to cmdline, this is too specific to buildout
  python and should be done by clients of this code.

[dobey (dobey)]

So, for the case where all the arguments are passed in, the API doesn't have to be overly complex to allow for it. We can simply require them to be passed in **kwargs, and document what they are called. This can keep the basic API nice and simple, and we can have additional things for the cases where we need them.

[dobey (dobey)]

This is more what I was thinking, in terms of having a 'smarter' API, that we could just use via kwargs, and simply state in the docstring what kwargs are available for use, though they aren't required: http://bazaar.launchpad.net/~dobey/dirspec/add-exefind/revision/12

I haven't updated the docstring in that revision, but there are the fallback_dirs= and app_names= kwargs, to specify a list of fallback directories, and a dict of app names for darwin.

=== modified file 'dirspec/tests/test_utils.py'
--- dirspec/tests/test_utils.py	2012-06-26 15:25:21 +0000
+++ dirspec/tests/test_utils.py	2012-07-09 22:27:20 +0000
@@ -14,11 +14,15 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 """Tests for utilities for the base directory implementation."""
+import imp
 import os
 import sys
 
+from twisted.trial.unittest import TestCase
+
 from dirspec import basedir
-from dirspec.utils import get_env_path, get_special_folders, user_home
+from dirspec.utils import (get_env_path, get_special_folders,
+                           user_home, get_cmdline)
 from dirspec.tests import BaseTestCase
 
 
@@ -150,3 +154,139 @@
         self.assertEqual(get_env_path(fake_env_var, default), default)
     test_get_env_path_var.skip = 'UnicodeEncodeError: bug #907053'
     test_get_env_path_no_var.skip = test_get_env_path_var.skip
+
+
+class ExeFindBaseTestCase(TestCase):
+    """Base class for testing the executable finder."""
+
+    def setUp(self):
+        """Set up fake modules."""
+        super(ExeFindBaseTestCase, self).setUp()
+        self.fakemod = imp.new_module("fun.bar.baz")
+        self.fakemod.__file__ = "/path/to/fun/bar/baz"
+        self.faketoplevelmod = imp.new_module("toplevel")
+        self.faketoplevelmod.__file__ = "/path/to/toplevel"
+        self.patch(os.path, "exists", lambda x: True)
+
+
+class UnfrozenSrcTestCase(ExeFindBaseTestCase):
+    """Test nonlinux cmdline using a wrapped python."""
+
+    def setUp(self):
+        super(UnfrozenSrcTestCase, self).setUp()
+        self.patch(sys, "platform", "darwin")
+
+    def test_unfrozen_dev_submodule(self):
+        """Not frozen, return path to bin dir."""
+        path = get_cmdline("foo", self.fakemod)
+        self.assertEquals(path, "/path/to/bin/foo")
+
+    def test_unfrozen_dev_toplevel(self):
+        """Not frozen, return python + path to bin dir."""
+        path = get_cmdline("foo", self.faketoplevelmod)
+        self.assertEquals(path, "/path/to/bin/foo")
+
+    def test_unfrozen_dev_toplevel_raises_nopath(self):
+        """Not frozen, raise OSError when the path doesn't exist."""
+        self.patch(os.path, "exists", lambda x: False)
+        self.assertRaises(OSError, get_cmdline, "foo",
+                          self.faketoplevelmod)
+
+
+class DarwinPkgdTestCase(ExeFindBaseTestCase):
+    """Test cmdline for running packaged on darwin."""
+
+    def setUp(self):
+        """SetUp to mimic frozen darwin."""
+        super(DarwinPkgdTestCase, self).setUp()
+        self.patch(sys, "platform", "darwin")
+        sys.frozen = True
+
+        self.fakeappmod = imp.new_module("modinapp")
+        self.fakeappmod.__file__ = "/path/to/Main.app/subpath/to/code"
+        self.fakeappmod.DARWIN_APP_NAMES = {"foo": "Foo.app"}
+
+    def tearDown(self):
+        """tearDown, Remove frozen attr"""
+        del sys.frozen
+
+    def test_darwin_pkgd(self):
+        """Return sub-app path on darwin when frozen."""
+        path = get_cmdline("foo", self.fakeappmod)
+        expectedpath = os.path.join("/path/to/Main.app/Contents/Resources/",
+                                    "Foo.app/Contents/MacOS/foo")
+        self.assertEquals(path, expectedpath)
+
+    def test_darwin_pkgd_raises_on_no_appnames(self):
+        """Raise AttributeError when no APP_NAMES dict is in the module."""
+        self.assertRaises(AttributeError, get_cmdline,
+                          "foo", self.fakemod)
+
+    def test_darwin_pkgd_raises_nopath(self):
+        """Frozen, raise OSError when the path doesn't exist."""
+        self.patch(os.path, "exists", lambda x: False)
+        self.assertRaises(OSError, get_cmdline, "foo",
+                          self.fakeappmod)
+
+
+class Win32PkgdTestCase(ExeFindBaseTestCase):
+    """Test cmdline for running packaged on windows."""
+
+    def setUp(self):
+        """SetUp to mimic frozen windows."""
+        super(Win32PkgdTestCase, self).setUp()
+        self.patch(sys, "platform", "win32")
+        sys.frozen = True
+
+    def tearDown(self):
+        """tearDown, Remove frozen attr"""
+        del sys.frozen
+
+    def test_windows_pkgd(self):
+        """Return sub-app path on windows when frozen."""
+
+        self.patch(sys, "executable", os.path.join("C:\\path", "to",
+                                                   "current.exe"))
+        # patch abspath to let us run this tests on non-windows:
+        self.patch(os.path, "abspath", lambda x: x)
+        path = get_cmdline("foo", None)
+        expectedpath = os.path.join("C:\\path", "to", "foo.exe")
+        self.assertEquals(path, expectedpath)
+
+    def test_windows_pkgd_raises_nopath(self):
+        """Frozen, raise OSError when the path doesn't exist."""
+        self.patch(os.path, "exists", lambda x: False)
+        self.assertRaises(OSError, get_cmdline, "foo",
+                          self.faketoplevelmod)
+
+
+class LinuxTestCase(ExeFindBaseTestCase):
+    """Test cmdline for running on linux."""
+
+    def setUp(self):
+        """SetUp to mimic linux2."""
+        super(LinuxTestCase, self).setUp()
+        self.patch(sys, "platform", "linux2")
+
+    def test_linux_src_relative_path_exists(self):
+        """linux unfrozen, return source relative path if it exists."""
+        path = get_cmdline("foo", self.fakemod)
+        expectedpath = "/path/to/bin/foo"
+        self.assertEquals(path, expectedpath)
+
+    def test_linux_no_src_relative_path_yes_constants(self):
+        """linux unfrozen, return value from constants.py."""
+        self.patch(os.path, "exists", lambda x: False)
+        consts = imp.new_module("constants")
+        consts.BIN_DIR = "/path/to/bin"
+        self.fakemod.constants = consts
+        self.addCleanup(delattr, self.fakemod, "constants")
+        path = get_cmdline("foo", self.fakemod)
+        expectedpath = "/path/to/bin/foo"
+        self.assertEquals(path, expectedpath)
+
+    def test_linux_no_src_relative_path_no_constants(self):
+        """raise if no src rel path and no constants module."""
+        self.patch(os.path, "exists", lambda x: False)
+        self.assertRaises(OSError, get_cmdline,
+                          "foo", self.faketoplevelmod)

=== modified file 'dirspec/utils.py'
--- dirspec/utils.py	2011-12-17 00:46:26 +0000
+++ dirspec/utils.py	2012-07-09 22:27:20 +0000
@@ -15,6 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 """Utilities for multiplatform support of XDG directory handling."""
 
+import errno
 import os
 import sys
 
@@ -24,11 +25,84 @@
            'default_config_path',
            'default_data_home',
            'default_data_path',
+           'get_cmdline',
            'get_env_path',
            'unicode_path',
            ]
 
 
+def _get_exe_path_frozen_win32(exe_name):
+    """Get path to the helper .exe on packaged windows."""
+    # all the .exes are in the same place on windows:
+    cur_exec_path = os.path.abspath(sys.executable)
+    exe_dir = os.path.dirname(cur_exec_path)
+    return os.path.join(exe_dir, exe_name + ".exe")
+
+
+def _get_exe_path_frozen_darwin(exe_name, module):
+    """Get path to the sub-app executable on packaged darwin."""
+
+    sub_app_name = module.DARWIN_APP_NAMES[exe_name]
+
+    main_app_dir = "".join(module.__file__.partition(".app")[:-1])
+    main_app_resources_dir = os.path.join(main_app_dir,
+                                          "Contents",
+                                          "Resources")
+    exe_bin = os.path.join(main_app_resources_dir,
+                           sub_app_name,
+                          "Contents", "MacOS",
+                           exe_name)
+    return exe_bin
+
+
+def _get_exe_path_from_constants(exe_name, module):
+    """Get path to executable from module's constants submodule."""
+    try:
+        bin_dir = getattr(module.constants, 'BIN_DIR')
+        return os.path.join(bin_dir, exe_name)
+    except (ImportError, AttributeError):
+        msg = '_get_exe_path_from_constants: cannot build a valid path. ' \
+            'constants module not available.'
+        raise OSError(errno.ENOENT, msg)
+
+
+def get_cmdline(exe_name, module):
+    """Get the command line to execute helper executable exe_name.
+
+    Given an exe name and a module that is in the same project as the
+    exe, returns the appropriate command line to run that exe.
+
+    Raises OSError if the exe does not exist. Some clients may wish to
+    trap and ignore this error.
+    """
+    if getattr(sys, "frozen", None) is not None:
+
+        if sys.platform == 'win32':
+            exe_path = _get_exe_path_frozen_win32(exe_name)
+        elif sys.platform == 'darwin':
+            exe_path = _get_exe_path_frozen_darwin(exe_name, module)
+        else:
+            raise Exception("Unsupported platform for frozen execution: %r" %
+                            sys.platform)
+    else:
+        module_depth = module.__name__.count('.')
+        parent_dir_index = -1 * (module_depth + 1)
+        parent_dir_l = module.__file__.split(os.path.sep)[:parent_dir_index]
+        parent_dir = os.path.sep.join(parent_dir_l)
+        exe_path = os.path.join(parent_dir, 'bin', exe_name)
+
+    if not os.path.exists(exe_path):
+        if sys.platform in ('win32', 'darwin'):
+            # these platforms don't have meaningful values in
+            # constants.py, so just raise:
+            raise OSError(errno.ENOENT,
+                          "Could not find executable at path %r" % exe_path)
+
+        exe_path = _get_exe_path_from_constants(exe_name, module)
+
+    return exe_path
+
+
 def get_env_path(key, default):
     """Get a UTF-8 encoded path from an environment variable."""
     if key in os.environ: