refactor workspace initialization and fix bug in tsrc apply-manifest

Workspace instances are now built with a WorkspaceConfig and LocalManifest instances.

LocalManifest is abstract and is either implemented by:

* ClonedManifest, which represents a manifest cloned in
<workspace_path>/.tsrc/manifest,
* or ManifestCopy, which represents a manifest located in the file system

This allows for a more robust implementation of the `apply-manifest` command

Fix #268

docs/changelog.md

@@ -1,3 +1,7 @@
+# 2.2.2 (2020-01-24)
+
+* Fix #268: `tsrc apply-manifest` no longer skips file system operations
+
 # 2.2.1 (2020-01-24)
 
 * Add CI jobs to check this project also works with Python 3.9

tsrc/cli/__init__.py

@@ -9,6 +9,7 @@
 from argh import arg
 
 import tsrc
+import tsrc.workspace
 from tsrc.manifest import Manifest
 from tsrc.workspace import Workspace
 from tsrc.workspace.config import WorkspaceConfig
@@ -65,7 +66,7 @@ def workspace_action(f: Callable) -> Callable:
     def res(*args: Any, workspace_path: Optional[Path] = None, **kwargs: Any) -> Any:
         if not workspace_path:
             workspace_path = find_workspace_path()
-        workspace = tsrc.Workspace(workspace_path)
+        workspace = tsrc.workspace.from_path(workspace_path)
         return f(workspace, *args, **kwargs)
 
     return res
@@ -90,7 +91,7 @@ def res(
     ) -> Any:
         if not workspace_path:
             workspace_path = find_workspace_path()
-        workspace = tsrc.Workspace(workspace_path)
+        workspace = tsrc.workspace.from_path(workspace_path)
         workspace.repos = resolve_repos(workspace, groups, all_cloned)
         return f(workspace, *args, **kwargs)
 
@@ -123,7 +124,7 @@ def get_workspace(workspace_path: Optional[Path]) -> tsrc.Workspace:
     """
     if not workspace_path:
         workspace_path = find_workspace_path()
-    return tsrc.Workspace(workspace_path)
+    return tsrc.workspace.from_path(workspace_path)
 
 
 def get_workspace_with_repos(
@@ -136,7 +137,7 @@ def get_workspace_with_repos(
     Uses the value of the `-w, --workspace` option first, then the values
     of the  `--groups` and `--all-cloned` options.
     """
-    workspace = get_workspace(workspace_path)
+    workspace = tsrc.workspace.from_path(workspace_path)
     workspace.repos = resolve_repos(workspace, groups, all_cloned)
     return workspace
 

tsrc/cli/apply_manifest.py

@@ -8,6 +8,7 @@
 
 import tsrc.manifest
 from tsrc.cli import repos_from_config, workspace_action, workspace_arg
+from tsrc.workspace.manifest_copy import ManifestCopy
 
 
 @workspace_arg  # type: ignore
@@ -19,8 +20,9 @@ def apply_manifest(
     """ apply a local manifest file """
     ui.info_1("Applying manifest from", manifest_path)
 
-    manifest = tsrc.manifest.load(manifest_path)
-    workspace.repos = repos_from_config(manifest, workspace.config)
+    manifest_copy = ManifestCopy(manifest_path)
+    workspace.local_manifest = manifest_copy
+    workspace.repos = repos_from_config(manifest_copy.get_manifest(), workspace.config)
     workspace.clone_missing()
     workspace.set_remotes()
     workspace.perform_filesystem_operations()

tsrc/cli/init.py

@@ -7,8 +7,8 @@
 from argh import arg
 
 import tsrc
+import tsrc.workspace
 from tsrc.cli import groups_arg, repos_from_config, workspace_arg
-from tsrc.workspace import Workspace
 from tsrc.workspace.config import WorkspaceConfig
 
 remote_help = "only use this remote when cloning repositories"
@@ -47,12 +47,13 @@ def init(
 
     workspace_config.save_to_file(cfg_path)
 
-    workspace = Workspace(workspace_path)
+    workspace = tsrc.workspace.from_path(workspace_path)
+
     workspace.update_manifest()
     manifest = workspace.get_manifest()
     workspace.repos = repos_from_config(manifest, workspace_config)
     workspace.clone_missing()
     workspace.set_remotes()
     workspace.perform_filesystem_operations()
     ui.info_2("Workspace initialized")
-    ui.info_2("Configuration written in", ui.bold, workspace.cfg_path)
+    ui.info_2("Configuration written in", ui.bold, cfg_path)

tsrc/test/cli/test_apply_manifest.py

@@ -36,6 +36,36 @@ def test_apply_manifest_adds_new_repo(
     assert (workspace_path / "bar").exists(), "bar repo should have been cloned"
 
 
+def test_apply_manifest_performs_filesystem_operation(
+    tsrc_cli: CLI, git_server: GitServer, workspace_path: Path
+) -> None:
+    """Scenario:
+
+    * Create a manifest with one repo
+    * Create a workspace using `tsrc init`
+    * Copy the manifest file somewhere in the workspace
+    * Edit the copied manifest to contain a new symlink
+    * Run `tsrc apply-manifest /path/to/copied_manifest`
+    * Check that the new symlink gets created
+
+    """
+    git_server.add_repo("foo")
+    tsrc_cli.run("init", git_server.manifest_url)
+
+    cloned_manifest_path = workspace_path / ".tsrc/manifest/manifest.yml"
+    copied_manifest_path = workspace_path / "manifest.yml"
+    shutil.copy(cloned_manifest_path, copied_manifest_path)
+
+    add_symlink_to_manifest(
+        copied_manifest_path, "foo", source="some_source", target="foo/README"
+    )
+    tsrc_cli.run("apply-manifest", str(copied_manifest_path))
+
+    assert (
+        workspace_path / "some_source"
+    ).exists(), "some_source symlink should have been created"
+
+
 def add_repo_to_manifest(manifest_path: Path, dest: str, url: str) -> None:
     yaml = ruamel.yaml.YAML()
     data = yaml.load(manifest_path.read_text())
@@ -44,3 +74,15 @@ def add_repo_to_manifest(manifest_path: Path, dest: str, url: str) -> None:
     repos.append(to_add)
     with manifest_path.open("w") as fileobj:
         yaml.dump(data, fileobj)
+
+
+def add_symlink_to_manifest(
+    manifest_path: Path, dest: str, *, source: str, target: str
+) -> None:
+    yaml = ruamel.yaml.YAML()
+    data = yaml.load(manifest_path.read_text())
+    repos = data["repos"]
+    (repo_config,) = [x for x in repos if x["dest"] == dest]
+    repo_config["symlink"] = [{"source": source, "target": target}]
+    with manifest_path.open("w") as fileobj:
+        yaml.dump(data, fileobj)

tsrc/test/conftest.py

@@ -7,6 +7,7 @@
 from cli_ui.tests import MessageRecorder
 
 import tsrc
+import tsrc.workspace
 
 from .helpers.cli import tsrc_cli  # noqa
 from .helpers.git_server import git_server  # noqa
@@ -27,7 +28,7 @@ def workspace_path(tmp_path: Path) -> Path:
 
 @pytest.fixture
 def workspace(workspace_path: Path) -> tsrc.Workspace:
-    return tsrc.Workspace(workspace_path)
+    return tsrc.workspace.from_path(workspace_path)
 
 
 @pytest.fixture()

tsrc/test/test_resolve_repos.py

@@ -3,6 +3,7 @@
 
 import ruamel.yaml
 
+import tsrc.workspace
 from tsrc.cli import resolve_repos
 from tsrc.repo import Repo
 from tsrc.workspace import Workspace
@@ -39,7 +40,7 @@ def create_workspace(
         repo_groups=repo_groups or [],
     )
     config.save_to_file(tmp_path / ".tsrc" / "config.yml")
-    return Workspace(tmp_path)
+    return tsrc.workspace.from_path(tmp_path)
 
 
 def repo_names(repos: List[Repo]) -> List[str]:

tsrc/workspace/__init__.py

@@ -11,6 +11,7 @@
 import tsrc.executor
 import tsrc.git
 
+from .cloned_manifest import ClonedManifest
 from .cloner import Cloner
 from .config import WorkspaceConfig
 from .file_system_operator import FileSystemOperator
@@ -37,16 +38,12 @@ def copy_cfg_path_if_needed(root_path: Path) -> None:
 
 
 class Workspace:
-    def __init__(self, root_path: Path) -> None:
-        local_manifest_path = root_path / ".tsrc" / "manifest"
-        self.cfg_path = root_path / ".tsrc" / "config.yml"
+    def __init__(
+        self, root_path: Path, *, local_manifest: LocalManifest, config: WorkspaceConfig
+    ) -> None:
         self.root_path = root_path
-        self.local_manifest = LocalManifest(local_manifest_path)
-        copy_cfg_path_if_needed(root_path)
-        if not self.cfg_path.exists():
-            raise WorkspaceNotConfigured(root_path)
-
-        self.config = WorkspaceConfig.from_file(self.cfg_path)
+        self.config = config
+        self.local_manifest = local_manifest
 
         # Note: at this point the repositories on which the user wishes to
         # execute an action is unknown. This list will be set after processing
@@ -65,9 +62,7 @@ def get_manifest(self) -> tsrc.Manifest:
         return self.local_manifest.get_manifest()
 
     def update_manifest(self) -> None:
-        manifest_url = self.config.manifest_url
-        manifest_branch = self.config.manifest_branch
-        self.local_manifest.update(url=manifest_url, branch=manifest_branch)
+        self.local_manifest.update()
 
     def clone_missing(self) -> None:
         to_clone = []
@@ -118,3 +113,21 @@ def __init__(self, root_path: Path):
         super().__init__(
             f"Workspace in '{root_path}' is not configured. Please run `tsrc init`"
         )
+
+
+def from_path(root_path: Path) -> Workspace:
+    cfg_path = root_path / ".tsrc" / "config.yml"
+    copy_cfg_path_if_needed(root_path)
+    if not cfg_path.exists():
+        raise WorkspaceNotConfigured(root_path)
+
+    config = WorkspaceConfig.from_file(cfg_path)
+
+    local_manifest_path = root_path / ".tsrc" / "manifest"
+    cloned_manifest = ClonedManifest(
+        local_manifest_path,
+        url=config.manifest_url,
+        branch=config.manifest_branch,
+    )
+
+    return Workspace(root_path, local_manifest=cloned_manifest, config=config)

tsrc/workspace/cloned_manifest.py

@@ -0,0 +1,60 @@
+from pathlib import Path
+
+import tsrc.manifest
+from tsrc.workspace.local_manifest import LocalManifest
+
+
+class ClonedManifest(LocalManifest):
+    """Represent a manifest repository that has been cloned locally
+    inside `<workspace>/.tsrc/manifest`.
+
+    Usage:
+
+    >>> cloned_manifest = ClonedManifest(workspace / ".tsrc/manifest"),
+            url="git@acme.com/manifest.git", branch="devel"
+         )
+
+    # First, update the cloned repository using the remote git URL and the
+    # branch passed in the constructor
+    >> cloned_manifest.update()
+
+    # Then, read the `manifest.yml` file from the clone repository:
+    >>> manifest = cloned_manifest.get_manifest()
+
+    """
+
+    def __init__(self, clone_path: Path, *, url: str, branch: str) -> None:
+        self.clone_path = clone_path
+        self.url = url
+        self.branch = branch
+
+    def update(self) -> None:
+        if self.clone_path.exists():
+            self._reset_manifest_clone()
+        else:
+            self._clone_manifest()
+
+    def get_manifest(self) -> tsrc.manifest.Manifest:
+        return tsrc.manifest.load(self.clone_path / "manifest.yml")
+
+    def _reset_manifest_clone(self) -> None:
+        tsrc.git.run(self.clone_path, "remote", "set-url", "origin", self.url)
+
+        tsrc.git.run(self.clone_path, "fetch")
+        tsrc.git.run(self.clone_path, "checkout", "-B", self.branch)
+        # fmt: off
+        tsrc.git.run(
+            self.clone_path, "branch", self.branch,
+            "--set-upstream-to", f"origin/{self.branch}"
+        )
+        # fmt: on
+        ref = f"origin/{self.branch}"
+        tsrc.git.run(self.clone_path, "reset", "--hard", ref)
+
+    def _clone_manifest(self) -> None:
+        parent = self.clone_path.parent
+        name = self.clone_path.name
+        parent.mkdir(parents=True, exist_ok=True)
+        tsrc.git.run(
+            self.clone_path.parent, "clone", self.url, "--branch", self.branch, name
+        )

tsrc/workspace/local_manifest.py

@@ -1,55 +1,14 @@
-from pathlib import Path
-from typing import List, Optional, Tuple, cast  # noqa
+import abc
 
 import tsrc
 import tsrc.manifest
 
 
-class LocalManifest:
-    """Represent a manifest repository that has been cloned locally
-    inside `<workspace>/.tsrc/manifest`.
-
-    Usage:
-
-    >>> local_manifest = LocalManifest(Path(workspace / ".tsrc/manifest")
-
-    # First, update the cloned repository using a remote git URL and a
-    # branch:
-    >>> manifest.update("git@acme.com/manifest.git", branch="devel")
-
-    # Then, read the `manifest.yml` file from the clone repository:
-    >>> manifest = local_manifest.get_manifest()
-
-    """
-
-    def __init__(self, clone_path: Path) -> None:
-        self.clone_path = clone_path
-
-    def update(self, url: str, *, branch: str) -> None:
-        if self.clone_path.exists():
-            self._reset_manifest_clone(url, branch=branch)
-        else:
-            self._clone_manifest(url, branch=branch)
+class LocalManifest(metaclass=abc.ABCMeta):
+    @abc.abstractmethod
+    def update(self) -> None:
+        pass
 
+    @abc.abstractmethod
     def get_manifest(self) -> tsrc.manifest.Manifest:
-        return tsrc.manifest.load(self.clone_path / "manifest.yml")
-
-    def _reset_manifest_clone(self, url: str, *, branch: str) -> None:
-        tsrc.git.run(self.clone_path, "remote", "set-url", "origin", url)
-
-        tsrc.git.run(self.clone_path, "fetch")
-        tsrc.git.run(self.clone_path, "checkout", "-B", branch)
-        # fmt: off
-        tsrc.git.run(
-            self.clone_path, "branch", branch,
-            "--set-upstream-to", f"origin/{branch}"
-        )
-        # fmt: on
-        ref = f"origin/{branch}"
-        tsrc.git.run(self.clone_path, "reset", "--hard", ref)
-
-    def _clone_manifest(self, url: str, *, branch: str) -> None:
-        parent = self.clone_path.parent
-        name = self.clone_path.name
-        parent.mkdir(parents=True, exist_ok=True)
-        tsrc.git.run(self.clone_path.parent, "clone", url, "--branch", branch, name)
+        pass

tsrc/workspace/manifest_copy.py

@@ -0,0 +1,15 @@
+from pathlib import Path
+
+import tsrc.manifest
+from tsrc.workspace.local_manifest import LocalManifest
+
+
+class ManifestCopy(LocalManifest):
+    def __init__(self, manifest_copy_path: Path):
+        self.manifest_copy_path = manifest_copy_path
+
+    def update(self) -> None:
+        pass
+
+    def get_manifest(self) -> tsrc.manifest.Manifest:
+        return tsrc.manifest.load(self.manifest_copy_path)