From 8fa6784d67e39ce02e5e27647519127138e89f9e Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 30 Oct 2025 17:09:50 -0400 Subject: [PATCH 1/4] fix: preserve xattrs in layers --- rockcraft/layers.py | 47 ++++++++++++++++++++++++++++++++++++++---- tests/unit/test_oci.py | 31 +++++++++++++++------------- 2 files changed, 60 insertions(+), 18 deletions(-) diff --git a/rockcraft/layers.py b/rockcraft/layers.py index 952e50874..69119a70b 100644 --- a/rockcraft/layers.py +++ b/rockcraft/layers.py @@ -20,11 +20,13 @@ import tarfile from collections import defaultdict from pathlib import Path +from tempfile import TemporaryDirectory from craft_cli import emit from craft_parts.executor.collisions import paths_collide from craft_parts.overlays import overlays from craft_parts.permissions import Permissions +from craft_parts.utils import process from rockcraft import errors @@ -45,14 +47,51 @@ def archive_layer( candidates = _gather_layer_paths(new_layer_dir, base_layer_dir) layer_paths = _merge_layer_paths(candidates) - with tarfile.open(temp_tar_file, mode="w") as tar_file: + if not temp_tar_file.parent.exists(): + temp_tar_file.parent.mkdir(parents=True) + + layer_contents: list[Path] = [] + with TemporaryDirectory() as tmpdir: + tmppath = Path(tmpdir) + # Just create an empty tar file if there's nothing to put in the layer + # GNU tar simply refuses to create an actual empty tar file though, so just use + # python's tarfile + if not layer_paths: + with tarfile.open(temp_tar_file, "w"): + pass + return # Iterate on sorted keys, so that the directories are always listed before # any files that they contain (otherwise tools like Docker might choke on # the layer tarball). - for arcname in sorted(layer_paths): + for arcname in sorted(layer_paths, reverse=True): filepath = layer_paths[arcname] - emit.debug(f"Adding to layer: {filepath} as '{arcname}'") - tar_file.add(filepath, arcname=arcname, recursive=False) + emit.debug(f"Adding to layer: {filepath} as {arcname!r}") + + # Construct a new file at `arcname` with the same contents as `filepath`. + # This emulates the `arcname` parameter of `tarfile.open()`, which is not + # present in GNU tar. + new_path = tmppath / arcname + layer_contents.append(new_path.relative_to(tmppath)) + if new_path.is_dir() and new_path.exists(): + continue + new_path.parent.mkdir(parents=True, exist_ok=True) + filepath.rename(new_path) + + # GNU tar is being used instead of Python's `tarfile` as it does not support + # special file attributes like xattrs. + tar_command: list[str | Path] = [ + "tar", + "-cf", + temp_tar_file.resolve(), + # Don't descend automatically into directories + "--no-recursion", + # Preserve all those fancy attributes + "--acls", + "--xattrs", + "--selinux", + *reversed(layer_contents), + ] + process.run(tar_command, cwd=tmppath, check=True) def prune_prime_files(prime_dir: Path, files: set[str], base_layer_dir: Path) -> None: diff --git a/tests/unit/test_oci.py b/tests/unit/test_oci.py index b73596d22..1aa0f62f1 100644 --- a/tests/unit/test_oci.py +++ b/tests/unit/test_oci.py @@ -17,7 +17,6 @@ import hashlib import json import os -import tarfile from pathlib import Path from typing import NamedTuple from unittest.mock import ANY, call, mock_open, patch @@ -284,30 +283,34 @@ def test_add_layer(self, mocker, mock_run, new_dir): Path("layer_dir").mkdir() Path("layer_dir/foo.txt").touch() pid = os.getpid() - - spy_add = mocker.spy(tarfile.TarFile, "add") - + mock_proc_run = mocker.patch("craft_parts.utils.process.run") image.add_layer("tag", Path("layer_dir")) - # The `Tarfile.add()` on the directory ends up calling the method multiple - # times (due to the recursion), but we're mainly interested that the first - # call was to add `layer_dir`. - assert spy_add.mock_calls[0] == call( - ANY, Path("layer_dir/foo.txt"), arcname="foo.txt", recursive=False - ) + expected_tar_file = new_dir / f"c/.temp_layer.{pid}.tar" + expected_tar_cmd = [ + "tar", + "-cf", + expected_tar_file, + "--no-recursion", + "--acls", + "--xattrs", + "--selinux", + Path("foo.txt"), + ] + mock_proc_run.assert_called_once_with(expected_tar_cmd, cwd=ANY, check=True) expected_cmd = [ "umoci", "raw", "add-layer", "--image", str(new_dir / "c/a:b"), - str(new_dir / f"c/.temp_layer.{pid}.tar"), + str(expected_tar_file), "--tag", "tag", ] - assert mock_run.mock_calls == [ - call([*expected_cmd, "--history.created_by", " ".join(expected_cmd)]) - ] + mock_run.assert_called_once_with( + [*expected_cmd, "--history.created_by", " ".join(expected_cmd)] + ) def test_add_new_user( self, From b0ce0369a7b055bc4f8038e5fa7ee8472b32dbe8 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 30 Oct 2025 17:16:26 -0400 Subject: [PATCH 2/4] fix: sort, not reverse --- rockcraft/layers.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rockcraft/layers.py b/rockcraft/layers.py index 69119a70b..3149cfda5 100644 --- a/rockcraft/layers.py +++ b/rockcraft/layers.py @@ -60,9 +60,11 @@ def archive_layer( with tarfile.open(temp_tar_file, "w"): pass return + # Iterate on sorted keys, so that the directories are always listed before # any files that they contain (otherwise tools like Docker might choke on # the layer tarball). + # Walk in reverse to avoid encountering not-yet created dirs for arcname in sorted(layer_paths, reverse=True): filepath = layer_paths[arcname] emit.debug(f"Adding to layer: {filepath} as {arcname!r}") @@ -89,7 +91,7 @@ def archive_layer( "--acls", "--xattrs", "--selinux", - *reversed(layer_contents), + *sorted(layer_contents), ] process.run(tar_command, cwd=tmppath, check=True) From 7677e8c1953fae7f955637e34c863728309dc054 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Thu, 30 Oct 2025 17:22:55 -0400 Subject: [PATCH 3/4] docs: improve code comments --- rockcraft/layers.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rockcraft/layers.py b/rockcraft/layers.py index 3149cfda5..4a5a07fbb 100644 --- a/rockcraft/layers.py +++ b/rockcraft/layers.py @@ -61,9 +61,6 @@ def archive_layer( pass return - # Iterate on sorted keys, so that the directories are always listed before - # any files that they contain (otherwise tools like Docker might choke on - # the layer tarball). # Walk in reverse to avoid encountering not-yet created dirs for arcname in sorted(layer_paths, reverse=True): filepath = layer_paths[arcname] @@ -91,6 +88,9 @@ def archive_layer( "--acls", "--xattrs", "--selinux", + # Tarball sorted files, so that the directories are always listed before + # any files that they contain (otherwise tools like Docker might choke on + # the layer tarball). *sorted(layer_contents), ] process.run(tar_command, cwd=tmppath, check=True) From 5e1d4a4de77bc19fa2ce48c8b9cb267cda5b7aa1 Mon Sep 17 00:00:00 2001 From: Imani Pelton Date: Fri, 31 Oct 2025 10:19:55 -0400 Subject: [PATCH 4/4] test(wip): spread test check --- rockcraft/layers.py | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/rockcraft/layers.py b/rockcraft/layers.py index 4a5a07fbb..d3a4f85c2 100644 --- a/rockcraft/layers.py +++ b/rockcraft/layers.py @@ -47,10 +47,8 @@ def archive_layer( candidates = _gather_layer_paths(new_layer_dir, base_layer_dir) layer_paths = _merge_layer_paths(candidates) - if not temp_tar_file.parent.exists(): - temp_tar_file.parent.mkdir(parents=True) - layer_contents: list[Path] = [] + transforms: list[str] = [] with TemporaryDirectory() as tmpdir: tmppath = Path(tmpdir) # Just create an empty tar file if there's nothing to put in the layer @@ -61,20 +59,24 @@ def archive_layer( pass return - # Walk in reverse to avoid encountering not-yet created dirs - for arcname in sorted(layer_paths, reverse=True): - filepath = layer_paths[arcname] - emit.debug(f"Adding to layer: {filepath} as {arcname!r}") - - # Construct a new file at `arcname` with the same contents as `filepath`. - # This emulates the `arcname` parameter of `tarfile.open()`, which is not - # present in GNU tar. - new_path = tmppath / arcname - layer_contents.append(new_path.relative_to(tmppath)) - if new_path.is_dir() and new_path.exists(): - continue - new_path.parent.mkdir(parents=True, exist_ok=True) - filepath.rename(new_path) + # # Walk in reverse to avoid encountering not-yet created dirs + # for arcname in sorted(layer_paths, reverse=True): + # filepath = layer_paths[arcname] + # emit.debug(f"Adding to layer: {filepath} as {arcname!r}") + + # # Construct a new file at `arcname` with the same contents as `filepath`. + # # This emulates the `arcname` parameter of `tarfile.open()`, which is not + # # present in GNU tar. + # new_path = tmppath / arcname + # layer_contents.append(new_path.relative_to(tmppath)) + # if new_path.is_dir() and new_path.exists(): + # continue + # new_path.parent.mkdir(parents=True, exist_ok=True) + # filepath.rename(new_path) + + for arcname, oldname in layer_paths.items(): + oldname = str(oldname).removeprefix("/") + transforms.extend(["--transform", f"s|{str(oldname)}|{arcname}|"]) # GNU tar is being used instead of Python's `tarfile` as it does not support # special file attributes like xattrs. @@ -88,10 +90,11 @@ def archive_layer( "--acls", "--xattrs", "--selinux", + *transforms, # Tarball sorted files, so that the directories are always listed before # any files that they contain (otherwise tools like Docker might choke on # the layer tarball). - *sorted(layer_contents), + *sorted(str(p) for p in layer_paths.values()), ] process.run(tar_command, cwd=tmppath, check=True)