Introduce pyls.source_roots configuration

flying-foozy · flying-foozy · commit 3cac737ba59e · 2020-01-17T18:00:50.000+09:00
Before this commit, 'setup.py' or 'pyproject.toml' should be created
to specify source root(s) other than the workspace root, even if such
file is not actually needed.

In order to explicitly specify source root(s) in the workspace, this
commit introduces pyls.source_roots configuration, and makes
workspace.source_roots() return its value if configured.

Path in pyls.source_roots is ignored, if it refers outside of the
workspace.

This configuration is also useful in the case that the workspace
consists of multiple (independent) services and a library shared by
them, for example. In such case, N + 1 source roots are listed in
pyls.source_roots configuration.

BTW, this commit puts some utilities into _util.py for reuse in the
future. Especially, os.path.commonprefix() for checking inside-ness in
find_parents() below should be replaced with is_inside_of(), because
the former returns unintentional value in some cases.

    if not os.path.commonprefix((root, path)):
        log.warning("Path %s not in %s", path, root)
        return []

For example:

  - commonprefix(('/foo', '/bar')) returns not false value but '/'
  - commonprefix(('/foo', '/foobar')) returns not false value but '/foo'
diff --git a/pyls/_utils.py b/pyls/_utils.py
@@ -83,6 +83,48 @@ def find_parents(root, path, names):
     return []
 
 
+def is_inside_of(path, root, strictly=True):
+    """Return whether path is inside of root or not
+
+    It is assumed that both path and root are absolute.
+
+    If strictly=False, os.path.normcase and os.path.normpath are not
+    applied on path and root for efficiency. This is useful if
+    ablsolute "path" is made from relative one and "root" (and already
+    normpath-ed), for example.
+    """
+    if strictly:
+        path = os.path.normcase(os.path.normpath(path))
+        root = os.path.normcase(os.path.normpath(root))
+
+    return path == root or path.startswith(root + os.path.sep)
+
+
+def normalize_paths(paths, basedir, inside_only):
+    """Normalize each elements in paths
+
+    Relative elements in paths are treated as relative to basedir.
+
+    This function yields "(path, validity)" tuple as normalization
+    result for each elements in paths. If inside_only is specified and path is
+    not so, validity is False. Otherwise, path is already normalized
+    as absolute path, and validity is True.
+    """
+    for path in paths:
+        full_path = os.path.normpath(os.path.join(basedir, path))
+        if (not inside_only or
+                # If "inside_only" is specified, path must (1) not be
+                # absolute (= be relative to basedir), (2) not have
+                # drive letter (on Windows), and (3) be descendant of
+                # the root (= "inside_only").
+                (not os.path.isabs(path) and
+                 not os.path.splitdrive(path)[0] and
+                 is_inside_of(full_path, inside_only, strictly=False))):
+            yield full_path, True
+        else:
+            yield path, False
+
+
 def match_uri_to_workspace(uri, workspaces):
     if uri is None:
         return None
diff --git a/pyls/config/pyls_conf.py b/pyls/config/pyls_conf.py
@@ -1,14 +1,20 @@
 import logging
 import os
-from pyls._utils import find_parents, merge_dicts
-from .source import ConfigSource
+from pyls._utils import find_parents, get_config_by_path, merge_dicts, normalize_paths
+from .source import ConfigSource, _set_opt
 
 log = logging.getLogger(__name__)
 
 CONFIG_KEY = 'pyls'
 PROJECT_CONFIGS = ['setup.cfg', 'tox.ini']
 
 OPTIONS = [
+    ('source_roots', 'source_roots', list),
+]
+
+# list of (config_path, inside_only) tuples for path normalization
+NORMALIZED_CONFIGS = [
+    ('source_roots', True),
 ]
 
 
@@ -58,6 +64,23 @@ def project_config(self, document_path):
             if not parsed:
                 continue  # no pyls specific configuration
 
+            self.normalize(parsed, os.path.dirname(files[0]))
+
             settings = merge_dicts(settings, parsed)
 
         return settings
+
+    def normalize(self, config, basedir):
+        for config_path, inside_only in NORMALIZED_CONFIGS:
+            paths = get_config_by_path(config, config_path)
+            if not paths:
+                continue  # not specified (or empty)
+
+            normalized = []
+            for path, valid in normalize_paths(paths, basedir, inside_only and self._norm_root_path):
+                if valid:
+                    normalized.append(path)
+                else:
+                    log.warning("Ignoring path '%s' for pyls.%s", path, config_path)
+
+            _set_opt(config, config_path, normalized)
diff --git a/pyls/workspace.py b/pyls/workspace.py
@@ -97,6 +97,11 @@ def show_message(self, message, msg_type=lsp.MessageType.Info):
 
     def source_roots(self, document_path):
         """Return the source roots for the given document."""
+        source_roots = self._config and self._config.settings(document_path=document_path).get('source_roots')
+        if source_roots:
+            # Use specified source_roots without traversing upper directories
+            return source_roots
+
         files = _utils.find_parents(self._root_path, document_path, ['setup.py', 'pyproject.toml']) or []
         return list(set((os.path.dirname(project_file) for project_file in files))) or [self._root_path]
 
diff --git a/test/test_workspace.py b/test/test_workspace.py
@@ -5,13 +5,16 @@
 import pytest
 
 from pyls import uris
+from pyls.python_ls import PythonLanguageServer
 
 PY2 = sys.version_info.major == 2
 
 if PY2:
     import pathlib2 as pathlib
+    from StringIO import StringIO
 else:
     import pathlib
+    from io import StringIO
 
 
 DOC_URI = uris.from_fs_path(__file__)
@@ -119,3 +122,124 @@ def test_multiple_workspaces(tmpdir, pyls):
     pyls.m_workspace__did_change_workspace_folders(
         added=[], removed=[added_workspaces[0]])
     assert workspace1_uri not in pyls.workspaces
+
+
+def _make_paths_dir_relative(paths, dirname):
+    """Make paths relative to dirname
+
+    This method assumes below for simplicity:
+
+    - if a path in paths is relative, it is relative to "root"
+    - dirname must be relative to "root"
+    - empty dirname means "root" itself
+    - neither "." nor ".." is allowed for dirname
+    """
+    to_root = os.path.join(*(['..'] * len(dirname.split(os.path.sep)))) if dirname else ''
+    return list(p if os.path.isabs(p) else os.path.join(to_root, p) for p in paths)
+
+
+@pytest.mark.parametrize('metafile', [
+    'setup.cfg',
+    'tox.ini',
+    'service/foo/setup.cfg',
+    'service/foo/tox.ini',
+])
+def test_source_roots_config(tmpdir, metafile):
+    """Examine that source_roots config is intentionaly read in.
+
+    This test also examines below for entries in source_roots:
+
+    * absolute path is ignored
+    * relative path is:
+      - ignored, if it does not refer inside of the workspace
+      - otherwise, it is treated as relative to config file location, and
+      - normalized into absolute one
+    """
+    root_path = str(tmpdir)
+
+    invalid_roots = ['/invalid/root', '../baz']
+    source_roots = ['service/foo', 'service/bar'] + invalid_roots
+    doc_root = source_roots[0]
+
+    if metafile:
+        dirname = os.path.dirname(metafile)
+        if dirname:
+            os.makedirs(os.path.join(root_path, dirname))
+
+        # configured by metafile at pyls startup
+        with open(os.path.join(root_path, metafile), 'w+') as f:
+            f.write('[pyls]\nsource_roots=\n    %s\n' %
+                    ',\n    '.join(_make_paths_dir_relative(source_roots, dirname)))
+
+    pyls = PythonLanguageServer(StringIO, StringIO)
+    pyls.m_initialize(
+        processId=1,
+        rootUri=uris.from_fs_path(root_path),
+        initializationOptions={}
+    )
+
+    # put new document under ROOT/service/foo
+    test_uri = uris.from_fs_path(os.path.join(root_path, doc_root, 'hello/test.py'))
+    pyls.workspace.put_document(test_uri, 'assert true')
+    test_doc = pyls.workspace.get_document(test_uri)
+
+    # apply os.path.normcase() on paths below, because case-sensitive
+    # comparison on Windows causes unintentional failure for case
+    # instability around drive letter
+
+    sys_path = [os.path.normcase(p) for p in test_doc.sys_path()]
+    for raw_path in source_roots:
+        full_path = os.path.normcase(os.path.join(root_path, raw_path))
+        if raw_path in invalid_roots:
+            assert os.path.normpath(full_path) not in sys_path
+            assert full_path not in sys_path  # check for safety
+        else:
+            assert os.path.normpath(full_path) in sys_path
+
+
+@pytest.mark.parametrize('metafile', ['setup.cfg', 'tox.ini'])
+def test_pyls_config_readin(tmpdir, metafile):
+    """Examine that pyls config in the workspace root is always read in.
+
+    This test creates two config files. One is created in the
+    workspace root, and another is created in ascendant of the target
+    document. Only the former has source_roots config.
+
+    Then, this test examines that the former is always read in,
+    regardless of existence of the latter, by checking source_roots
+    config.
+    """
+    root_path = str(tmpdir)
+
+    source_roots = ['service/foo', 'service/bar']
+
+    with open(os.path.join(root_path, metafile), 'w+') as f:
+        f.write('[pyls]\nsource_roots=\n    %s\n' %
+                ',\n    '.join(source_roots))
+
+    doc_root = source_roots[0]
+
+    os.makedirs(os.path.join(root_path, doc_root))
+    with open(os.path.join(root_path, doc_root, metafile), 'w+') as f:
+        f.write('\n')
+
+    pyls = PythonLanguageServer(StringIO, StringIO)
+    pyls.m_initialize(
+        processId=1,
+        rootUri=uris.from_fs_path(root_path),
+        initializationOptions={}
+    )
+
+    # put new document under root/service/foo
+    test_uri = uris.from_fs_path(os.path.join(root_path, doc_root, 'hello/test.py'))
+    pyls.workspace.put_document(test_uri, 'assert True')
+    test_doc = pyls.workspace.get_document(test_uri)
+
+    # apply os.path.normcase() on paths below, because case-sensitive
+    # comparison on Windows causes unintentional failure for case
+    # instability around drive letter
+
+    sys_path = [os.path.normcase(p) for p in test_doc.sys_path()]
+    for raw_path in source_roots:
+        full_path = os.path.normcase(os.path.join(root_path, raw_path))
+        assert os.path.normpath(full_path) in sys_path
diff --git a/vscode-client/package.json b/vscode-client/package.json
@@ -35,6 +35,15 @@
                     },
                     "uniqueItems": true
                 },
+                "pyls.source_roots": {
+                    "type": "array",
+                    "default": [],
+                    "description": "List of source roots in the working space.",
+                    "items": {
+                        "type": "string"
+                    },
+                    "uniqueItems": true
+                },
                 "pyls.plugins.jedi.extra_paths": {
                     "type": "array",
                     "default": [],
