make the test suite pass on Windows. (#408)

* These changes make it so that the test suite passes on Windows.

Some areas for further work have been marked with notes.

.gitignore

@@ -5,3 +5,4 @@ __pycache__
 /ops.egg-info
 .idea
 /docs/_build
+*~

.travis.yml

@@ -16,9 +16,19 @@ matrix:
     include:
         - os: osx
           language: generic
+        - os: windows
+          language: shell
+          before_install:
+            - choco install python --version 3.8.5
+            - python -m pip install --upgrade pip
+          env:
+            - PATH=/c/Python38:/c/Python38/Scripts:$PATH
 
 install:
     - pip3 install -r requirements-dev.txt
 
+before_script:
+    - printenv | sort
+
 script:
-    - ./run_tests
+    - ./run_tests -v

ops/main.py

@@ -15,8 +15,10 @@
 import inspect
 import logging
 import os
+import shutil
 import subprocess
 import sys
+import typing
 import warnings
 from pathlib import Path
 
@@ -36,6 +38,17 @@
 logger = logging.getLogger()
 
 
+def _exe_path(path: Path) -> typing.Optional[Path]:
+    """Find and return the full path to the given binary.
+
+    Here path is the absolute path to a binary, but might be missing an extension.
+    """
+    p = shutil.which(path.name, mode=os.F_OK, path=str(path.parent))
+    if p is None:
+        return None
+    return Path(p)
+
+
 def _get_charm_dir():
     charm_dir = os.environ.get("JUJU_CHARM_DIR")
     if charm_dir is None:
@@ -69,9 +82,6 @@ def _create_event_link(charm, bound_event, link_to):
 
     event_dir.mkdir(exist_ok=True)
     if not event_path.exists():
-        # CPython has different implementations for populating sys.argv[0] for Linux and Windows.
-        # For Windows it is always an absolute path (any symlinks are resolved)
-        # while for Linux it can be a relative path.
         target_path = os.path.relpath(link_to, str(event_dir))
 
         # Ignore the non-symlink files or directories
@@ -96,6 +106,11 @@ def _setup_event_links(charm_dir, charm):
     charm -- An instance of the Charm class.
 
     """
+    # XXX: on windows this function does not accomplish what it wants to:
+    #      it creates symlinks with no extension pointing to a .py
+    #      and juju only knows how to handle .exe, .bat, .cmd, and .ps1
+    #      so it does its job, but does not accomplish anything as the
+    #      hooks aren't 'callable'.
     link_to = os.path.realpath(os.environ.get("JUJU_DISPATCH_PATH", sys.argv[0]))
     for bound_event in charm.on.events().values():
         # Only events that originate from Juju need symlinks.
@@ -170,7 +185,8 @@ def __init__(self, charm_dir: Path):
         self._charm_dir = charm_dir
         self._exec_path = Path(os.environ.get('JUJU_DISPATCH_PATH', sys.argv[0]))
 
-        if JujuVersion.from_environ().is_dispatch_aware() and (charm_dir / 'dispatch').exists():
+        dispatch = charm_dir / 'dispatch'
+        if JujuVersion.from_environ().is_dispatch_aware() and _exe_path(dispatch) is not None:
             self._init_dispatch()
         else:
             self._init_legacy()
@@ -204,8 +220,8 @@ def run_any_legacy_hook(self):
             # we *are* the legacy hook
             return
 
-        dispatch_path = self._charm_dir / self._dispatch_path
-        if not dispatch_path.exists():
+        dispatch_path = _exe_path(self._charm_dir / self._dispatch_path)
+        if dispatch_path is None:
             logger.debug("Legacy %s does not exist.", self._dispatch_path)
             return
 
@@ -224,10 +240,11 @@ def run_any_legacy_hook(self):
         try:
             subprocess.run(argv, check=True)
         except subprocess.CalledProcessError as e:
-            logger.warning(
-                "Legacy %s exited with status %d.",
-                self._dispatch_path, e.returncode)
+            logger.warning("Legacy %s exited with status %d.", self._dispatch_path, e.returncode)
             sys.exit(e.returncode)
+        except OSError as e:
+            logger.warning("Unable to run legacy %s: %s", self._dispatch_path, e)
+            sys.exit(1)
         else:
             logger.debug("Legacy %s exited with status 0.", self._dispatch_path)
 

ops/model.py

@@ -1013,11 +1013,12 @@ def __init__(self, unit_name=None, model_name=None):
         self._leader_check_time = None
 
     def _run(self, *args, return_output=False, use_json=False):
-        kwargs = dict(stdout=PIPE, stderr=PIPE)
+        kwargs = dict(stdout=PIPE, stderr=PIPE, check=True)
+        args = (shutil.which(args[0]),) + args[1:]
         if use_json:
             args += ('--format=json',)
         try:
-            result = run(args, check=True, **kwargs)
+            result = run(args, **kwargs)
         except CalledProcessError as e:
             raise ModelError(e.stderr)
         if return_output:

ops/storage.py

@@ -22,6 +22,13 @@
 import yaml
 
 
+def _run(args, **kw):
+    cmd = shutil.which(args[0])
+    if cmd is None:
+        raise FileNotFoundError(args[0])
+    return subprocess.run([cmd, *args[1:]], **kw)
+
+
 class SQLiteStorage:
 
     DB_LOCK_TIMEOUT = timedelta(hours=1)
@@ -272,10 +279,10 @@ def set(self, key: str, value: typing.Any) -> None:
         # have the same default style.
         encoded_value = yaml.dump(value, Dumper=_SimpleDumper, default_flow_style=None)
         content = yaml.dump(
-            {key: encoded_value}, encoding='utf-8', default_style='|',
+            {key: encoded_value}, encoding='utf8', default_style='|',
             default_flow_style=False,
             Dumper=_SimpleDumper)
-        subprocess.run(["state-set", "--file", "-"], input=content, check=True)
+        _run(["state-set", "--file", "-"], input=content, check=True)
 
     def get(self, key: str) -> typing.Any:
         """Get the bytes value associated with a given key.
@@ -286,12 +293,8 @@ def get(self, key: str) -> typing.Any:
             CalledProcessError: if 'state-get' returns an error code.
         """
         # We don't capture stderr here so it can end up in debug logs.
-        p = subprocess.run(
-            ["state-get", key],
-            stdout=subprocess.PIPE,
-            check=True,
-        )
-        if p.stdout == b'' or p.stdout == b'\n':
+        p = _run(["state-get", key], stdout=subprocess.PIPE, check=True, universal_newlines=True)
+        if p.stdout == '' or p.stdout == '\n':
             raise KeyError(key)
         return yaml.load(p.stdout, Loader=_SimpleLoader)
 
@@ -303,7 +306,7 @@ def delete(self, key: str) -> None:
         Raises:
             CalledProcessError: if 'state-delete' returns an error code.
         """
-        subprocess.run(["state-delete", key], check=True)
+        _run(["state-delete", key], check=True)
 
 
 class NoSnapshotError(Exception):

run_tests

@@ -1,3 +1,13 @@
 #!/bin/bash
 
-python3 -m unittest "$@"
+if command -v python3 >/dev/null; then
+    # linux or macos
+    _python=python3
+else
+    # windows
+    _python=python
+fi
+
+"$_python" --version
+
+"$_python" -m unittest "$@"

test/bin/relation-ids.bat

@@ -0,0 +1,22 @@
+@ECHO OFF
+
+CALL :CASE_%1 2>NUL
+IF ERRORLEVEL 1 ECHO []
+
+EXIT /B 0
+
+:CASE_db
+    ECHO ["db:1"]
+    EXIT /B
+
+:CASE_mon
+    ECHO ["mon:2"]
+    EXIT /B
+
+:CASE_db1
+    ECHO ["db1:4"]
+    EXIT /B
+
+:CASE_db2
+    ECHO ["db2:5", "db2:6"]
+    EXIT /B

test/bin/relation-list.bat

@@ -0,0 +1,23 @@
+@ECHO OFF
+
+CALL :CASE_%2 %* 2>NUL
+IF ERRORLEVEL 1 CALL :CASE_3 %*
+EXIT /B %ERRORLEVEL%
+
+:CASE_1
+:CASE_2
+    ECHO ["remote/0"]
+    EXIT /B 0
+
+:CASE_3
+    ECHO ERROR invalid value "%2" for option -r: relation not found >&2
+    EXIT /B 2
+
+:CASE_4
+:CASE_5
+    ECHO ["remoteapp1/0"]
+    EXIT /B 0
+
+:CASE_6
+    ECHO ["remoteapp2/0"]
+    EXIT /B 0

test/test_charm.py

@@ -39,7 +39,9 @@ def restore_env(env):
             os.environ.update(env)
         self.addCleanup(restore_env, os.environ.copy())
 
-        os.environ['PATH'] = "{}:{}".format(Path(__file__).parent / 'bin', os.environ['PATH'])
+        os.environ['PATH'] = os.pathsep.join([
+            str(Path(__file__).parent / 'bin'),
+            os.environ['PATH']])
         os.environ['JUJU_UNIT_NAME'] = 'local/0'
 
         self.tmpdir = Path(tempfile.mkdtemp())

test/test_helpers.py

@@ -28,47 +28,58 @@
 def fake_script(test_case, name, content):
     if not hasattr(test_case, 'fake_script_path'):
         fake_script_path = tempfile.mkdtemp('-fake_script')
-        os.environ['PATH'] = '{}:{}'.format(fake_script_path, os.environ["PATH"])
+        old_path = os.environ["PATH"]
+        os.environ['PATH'] = os.pathsep.join([fake_script_path, os.environ["PATH"]])
 
         def cleanup():
             shutil.rmtree(fake_script_path)
-            os.environ['PATH'] = os.environ['PATH'].replace(fake_script_path + ':', '')
+            os.environ['PATH'] = old_path
 
         test_case.addCleanup(cleanup)
         test_case.fake_script_path = pathlib.Path(fake_script_path)
 
     template_args = {
         'name': name,
-        'path': test_case.fake_script_path,
+        'path': test_case.fake_script_path.as_posix(),
         'content': content,
     }
 
-    with (test_case.fake_script_path / name).open('wt') as f:
+    path = test_case.fake_script_path / name
+    with path.open('wt') as f:
         # Before executing the provided script, dump the provided arguments in calls.txt.
         # ASCII 1E is RS 'record separator', and 1C is FS 'file separator', which seem appropriate.
         f.write('''#!/bin/sh
 {{ printf {name}; printf "\\036%s" "$@"; printf "\\034"; }} >> {path}/calls.txt
 {content}'''.format_map(template_args))
-    os.chmod(str(test_case.fake_script_path / name), 0o755)
+    os.chmod(str(path), 0o755)
+    # TODO: this hardcodes the path to bash.exe, which works for now but might
+    #       need to be set via environ or something like that.
+    path.with_suffix(".bat").write_text(
+        '@"\\Program Files\\git\\bin\\bash.exe" {} %*\n'.format(path))
 
 
 def fake_script_calls(test_case, clear=False):
-    try:
-        with (test_case.fake_script_path / 'calls.txt').open('r+t') as f:
-            calls = [line.split('\x1e') for line in f.read().split('\x1c')[:-1]]
-            if clear:
-                f.truncate(0)
-            return calls
-    except FileNotFoundError:
+    calls_file = test_case.fake_script_path / 'calls.txt'
+    if not calls_file.exists():
         return []
 
+    # newline and encoding forced to linuxy defaults because on
+    # windows they're written from git-bash
+    with calls_file.open('r+t', newline='\n', encoding='utf8') as f:
+        calls = [line.split('\x1e') for line in f.read().split('\x1c')[:-1]]
+        if clear:
+            f.truncate(0)
+    return calls
+
 
 class FakeScriptTest(unittest.TestCase):
 
     def test_fake_script_works(self):
         fake_script(self, 'foo', 'echo foo runs')
         fake_script(self, 'bar', 'echo bar runs')
-        output = subprocess.getoutput('foo a "b c "; bar "d e" f')
+        # subprocess.getoutput goes via the shell, so it needs to be
+        # something both sh and CMD understand
+        output = subprocess.getoutput('foo a "b c " && bar "d e" f')
         self.assertEqual(output, 'foo runs\nbar runs')
         self.assertEqual(fake_script_calls(self), [
             ['foo', 'a', 'b c '],