fix(organize): use the part's install directories (#689)

Fixes an issue discovered via canonical/snapcraft#4654 where `organize_files()`
would organize in the cwd instead of the project's base_dir.

The problem was that `organize_files()` was redefining a part's install directories
incorrectly by creating relative filepaths.

To fix this, organize_files now accepts the part's install directories instead of
redefining them.

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>

craft_parts/executor/organize.py

@@ -27,22 +27,26 @@
 import shutil
 from glob import iglob
 from pathlib import Path
-from typing import Dict
+from typing import Dict, Mapping, Optional
 
 from craft_parts import errors
 from craft_parts.utils import file_utils, path_utils
 
 
 def organize_files(
-    *, part_name: str, mapping: Dict[str, str], base_dir: Path, overwrite: bool
+    *,
+    part_name: str,
+    file_map: Dict[str, str],
+    install_dir_map: Mapping[Optional[str], Path],
+    overwrite: bool,
 ) -> None:
     """Rearrange files for part staging.
 
     If partitions are enabled, source filepaths must be in the default partition.
 
     :param part_name: The name of the part to organize files for.
-    :param mapping: A mapping of source filepaths to destination filepaths.
-    :param base_dir: Directory containing files to organize.
+    :param file_map: A mapping of source filepaths to destination filepaths.
+    :param install_dir_map: A mapping of partition names to their install directories.
     :param overwrite: Whether existing files should be overwritten. This is
         only used in build updates, when a part may organize over files
         it previously organized.
@@ -52,7 +56,7 @@ def organize_files(
     :raises FileOrganizeError: If partitions are enabled and the source file is not from
         the default partition.
     """
-    for key in sorted(mapping, key=lambda x: ["*" in x, x]):
+    for key in sorted(file_map, key=lambda x: ["*" in x, x]):
         src_partition, src_inner_path = path_utils.get_partition_and_path(key)
 
         if src_partition and src_partition != "default":
@@ -64,27 +68,21 @@ def organize_files(
                 ),
             )
 
-        src = os.path.join(base_dir, src_inner_path)
+        src = os.path.join(install_dir_map[src_partition], src_inner_path)
 
         # Remove the leading slash so the path actually joins
         # Also trailing slash is significant, be careful if using pathlib!
         dst_partition, dst_inner_path = path_utils.get_partition_and_path(
-            mapping[key].lstrip("/")
+            file_map[key].lstrip("/")
         )
 
+        dst = os.path.join(install_dir_map[dst_partition], dst_inner_path)
+
+        # prefix the partition to the log-friendly version of the destination
         if dst_partition and dst_partition != "default":
-            dst = os.path.join(
-                "partitions",
-                dst_partition,
-                "parts",
-                part_name,
-                "install",
-                dst_inner_path,
-            )
-            partition_path = dst
+            dst_string = f"({dst_partition})/{dst_inner_path}"
         else:
-            dst = os.path.join(base_dir, dst_inner_path)
-            partition_path = str(dst_inner_path)
+            dst_string = str(dst_inner_path)
 
         sources = iglob(src, recursive=True)
 
@@ -107,18 +105,18 @@ def organize_files(
                     raise errors.FileOrganizeError(
                         part_name=part_name,
                         message=(
-                            f"multiple files to be organized into "
-                            f"{partition_path!r}. If this is "
-                            f"supposed to be a directory, end it with a slash."
+                            "multiple files to be organized into "
+                            f"{dst_string!r}. If this is "
+                            "supposed to be a directory, end it with a slash."
                         ),
                     )
                 else:
                     raise errors.FileOrganizeError(
                         part_name=part_name,
                         message=(
                             f"trying to organize file {key!r} to "
-                            f"{mapping[key]!r}, but "
-                            f"{partition_path!r} already exists"
+                            f"{file_map[key]!r}, but "
+                            f"{dst_string!r} already exists"
                         ),
                     )
 

craft_parts/executor/part_handler.py

@@ -879,8 +879,8 @@ def _organize(self, *, overwrite: bool = False) -> None:
         mapping = self._part.spec.organize_files
         organize_files(
             part_name=self._part.name,
-            mapping=mapping,
-            base_dir=self._part.part_install_dir,
+            file_map=mapping,
+            install_dir_map=self._part.part_install_dirs,
             overwrite=overwrite,
         )
 

tests/unit/executor/test_organize.py

@@ -142,6 +142,7 @@ def test_organize(new_dir, data):
         expected_message=data.get("expected_message"),
         expected_overwrite=data.get("expected_overwrite"),
         overwrite=False,
+        install_dirs={None: Path(new_dir / "install")},
     )
 
     # Verify that it can be organized again by overwriting
@@ -154,6 +155,7 @@ def test_organize(new_dir, data):
         expected_message=data.get("expected_message"),
         expected_overwrite=data.get("expected_overwrite"),
         overwrite=True,
+        install_dirs={None: Path(new_dir / "install")},
     )
 
 
@@ -167,15 +169,16 @@ def organize_and_assert(
     expected_message,
     expected_overwrite,
     overwrite,
+    install_dirs,
 ):
-    base_dir = Path(tmp_path / "install")
-    base_dir.mkdir(parents=True, exist_ok=True)
+    install_dir = Path(tmp_path / "install")
+    install_dir.mkdir(parents=True, exist_ok=True)
 
     for directory in setup_dirs:
-        (base_dir / directory).mkdir(exist_ok=True)
+        (install_dir / directory).mkdir(exist_ok=True)
 
     for file_entry in setup_files:
-        (base_dir / file_entry).touch()
+        (install_dir / file_entry).touch()
 
     if overwrite and expected_overwrite is not None:
         expected = expected_overwrite
@@ -185,22 +188,22 @@ def organize_and_assert(
         with pytest.raises(expected) as raised:
             organize_files(
                 part_name="part-name",
-                mapping=organize_map,
-                base_dir=base_dir,
+                file_map=organize_map,
+                install_dir_map=install_dirs,
                 overwrite=overwrite,
             )
         assert re.match(expected_message, str(raised.value)) is not None
 
     else:
         organize_files(
             part_name="part-name",
-            mapping=organize_map,
-            base_dir=base_dir,
+            file_map=organize_map,
+            install_dir_map=install_dirs,
             overwrite=overwrite,
         )
         expected = cast(List[Tuple[List[str], str]], expected)
         for expect in expected:
-            dir_path = (base_dir / expect[1]).as_posix()
+            dir_path = (install_dir / expect[1]).as_posix()
             dir_contents = os.listdir(dir_path)
             dir_contents.sort()
             assert dir_contents == expect[0]

tests/unit/features/partitions/executor/test_organize.py

@@ -121,7 +121,7 @@
             "expected": errors.FileOrganizeError,
             "expected_message": (
                 r".*trying to organize file '\(default\)/foo' to '\(our/special-part\)/bar', "
-                r"but 'partitions/our/special-part/parts/part-name/install/bar' already exists."
+                r"but '\(our/special-part\)/bar' already exists."
             ),
             "expected_overwrite": [
                 (["bar"], "../partitions/our/special-part/parts/part-name/install")
@@ -149,8 +149,7 @@
             "organize_map": {"*.conf": "(our/special-part)/dir"},
             "expected": errors.FileOrganizeError,
             "expected_message": (
-                r".*multiple files to be organized into "
-                r"'partitions/our/special-part/parts/part-name/install/dir'.*"
+                r".*multiple files to be organized into '\(our/special-part\)/dir'.*"
             ),
         },
         # *_for_directories
@@ -202,6 +201,14 @@
     ],
 )
 def test_organize(new_dir, data):
+    install_dirs = {
+        "default": new_dir / "install",
+        "mypart": new_dir / "partitions/mypart/parts/part-name/install",
+        "yourpart": new_dir / "partitions/yourpart/parts/part-name/install",
+        "our/special-part": new_dir
+        / "partitions/our/special-part/parts/part-name/install",
+    }
+
     organize_and_assert(
         tmp_path=new_dir,
         setup_dirs=data.get("setup_dirs", []),
@@ -211,6 +218,7 @@ def test_organize(new_dir, data):
         expected_message=data.get("expected_message"),
         expected_overwrite=data.get("expected_overwrite"),
         overwrite=False,
+        install_dirs=install_dirs,
     )
 
     # Verify that it can be organized again by overwriting
@@ -223,4 +231,5 @@ def test_organize(new_dir, data):
         expected_message=data.get("expected_message"),
         expected_overwrite=data.get("expected_overwrite"),
         overwrite=True,
+        install_dirs=install_dirs,
     )