From fb0e43e0793327c040987039ea5f7fde1f80963f Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Sun, 28 Jul 2024 19:30:20 +0200 Subject: [PATCH 1/9] fix: replace absolute symlinks from cache with correct version --- src/cache.rs | 13 ++++++++++++- src/source/copy_dir.rs | 25 +++++++++++++++++++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index e8b35dba8..23bc93e7e 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -16,7 +16,7 @@ use crate::{ packaging::{contains_prefix_binary, contains_prefix_text, content_type, Files}, recipe::parser::{Dependency, Requirements}, render::resolved_dependencies::{resolve_dependencies, FinalizedDependencies}, - source::copy_dir::{copy_file, CopyOptions}, + source::copy_dir::{copy_file, create_symlink, CopyOptions}, }; /// Error type for cache key generation @@ -113,6 +113,17 @@ impl Output { let source = &cache_dir.join("prefix").join(file); copy_file(source, &dest, &mut paths_created, ©_options).into_diagnostic()?; + // check if the symlink starts with the old prefix, and if yes, make the symlink absolute + // with the new prefix + if source.is_symlink() { + let symlink_target = fs::read_link(&source).into_diagnostic()?; + if let Ok(rest) = symlink_target.strip_prefix(&cache_prefix) { + let new_symlink_target = self.prefix().join(rest); + fs::remove_file(&dest).into_diagnostic()?; + create_symlink(&new_symlink_target, &dest).into_diagnostic()?; + } + } + if *has_prefix { replace_prefix(&dest, &cache_prefix, self.prefix())?; } diff --git a/src/source/copy_dir.rs b/src/source/copy_dir.rs index 7e2ecf29b..0d976e46e 100644 --- a/src/source/copy_dir.rs +++ b/src/source/copy_dir.rs @@ -34,6 +34,26 @@ impl Default for CopyOptions { } } +/// Cross platform way of creating a symlink +pub(crate) fn create_symlink( + from: impl AsRef, + to: impl AsRef, +) -> Result<(), SourceError> { + let from = from.as_ref(); + let to = to.as_ref(); + + if to.exists() { + fs_err::remove_file(to)?; + } + + #[cfg(unix)] + fs_err::os::unix::fs::symlink(from, to)?; + #[cfg(windows)] + std::os::windows::fs::symlink_file(from, to)?; + + Ok(()) +} + /// Copy a file or directory, or symlink to another location. /// Use reflink if possible. pub(crate) fn copy_file( @@ -57,10 +77,7 @@ pub(crate) fn copy_file( // if file is a symlink, copy it as a symlink if path.is_symlink() { let link_target = fs_err::read_link(path)?; - #[cfg(unix)] - fs_err::os::unix::fs::symlink(link_target, dest_path)?; - #[cfg(windows)] - std::os::windows::fs::symlink_file(link_target, dest_path)?; + create_symlink(link_target, dest_path)?; Ok(()) } else { if dest_path.exists() { From 099961ff01e6da015f372cbbdbd58bf8536d7c9e Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Sun, 28 Jul 2024 20:07:29 +0200 Subject: [PATCH 2/9] add symlinks to the package, even if the target is not part of the package --- src/cache.rs | 8 +++++++- src/packaging/metadata.rs | 6 +++--- src/source/copy_dir.rs | 12 ++++++------ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/cache.rs b/src/cache.rs index 23bc93e7e..9f830b370 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -117,10 +117,16 @@ impl Output { // with the new prefix if source.is_symlink() { let symlink_target = fs::read_link(&source).into_diagnostic()?; + println!("Current symlink target: {:?}", symlink_target); if let Ok(rest) = symlink_target.strip_prefix(&cache_prefix) { + println!("Rest: {:?}", rest); let new_symlink_target = self.prefix().join(rest); + println!("Creating symlink: {:?} -> {:?}", dest, new_symlink_target); + println!("Removing old symlink: {:?}", dest); fs::remove_file(&dest).into_diagnostic()?; - create_symlink(&new_symlink_target, &dest).into_diagnostic()?; + create_symlink(&dest, &new_symlink_target).into_diagnostic()?; + } else { + println!("Didn't change symlink target"); } } diff --git a/src/packaging/metadata.rs b/src/packaging/metadata.rs index e9e4f490e..5f67f1059 100644 --- a/src/packaging/metadata.rs +++ b/src/packaging/metadata.rs @@ -352,14 +352,14 @@ impl Output { if !p.exists() { if p.is_symlink() { tracing::warn!( - "Symlink target does not exist: {:?} -> {:?}", + "symlink target not part of _this_ package: {:?} -> {:?}", &p, fs::read_link(p)? ); + } else { + tracing::warn!("file does not exist: {:?}", &p); continue; } - tracing::warn!("File does not exist: {:?} (TODO)", &p); - continue; } if meta.is_dir() { diff --git a/src/source/copy_dir.rs b/src/source/copy_dir.rs index 0d976e46e..d2f45881c 100644 --- a/src/source/copy_dir.rs +++ b/src/source/copy_dir.rs @@ -42,14 +42,14 @@ pub(crate) fn create_symlink( let from = from.as_ref(); let to = to.as_ref(); - if to.exists() { - fs_err::remove_file(to)?; + if from.exists() { + fs_err::remove_file(from)?; } - + println!("Creating symlink from {:?} to {:?}", from, to); #[cfg(unix)] - fs_err::os::unix::fs::symlink(from, to)?; + fs_err::os::unix::fs::symlink(to, from)?; #[cfg(windows)] - std::os::windows::fs::symlink_file(from, to)?; + std::os::windows::fs::symlink_file(to, from)?; Ok(()) } @@ -77,7 +77,7 @@ pub(crate) fn copy_file( // if file is a symlink, copy it as a symlink if path.is_symlink() { let link_target = fs_err::read_link(path)?; - create_symlink(link_target, dest_path)?; + create_symlink(dest_path, link_target)?; Ok(()) } else { if dest_path.exists() { From a70feff0a6243138aa73d1efd5b53c97edec3e5f Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Wed, 31 Jul 2024 11:31:33 +0200 Subject: [PATCH 3/9] test symlinks with multiple output situation --- src/cache.rs | 29 +++-- src/source/copy_dir.rs | 30 +++-- test-data/recipes/cache/recipe-symlinks.yaml | 27 +++++ .../test_simple/test_symlink_recipe.json | 6 + test/end-to-end/conftest.py | 37 ++++++ test/end-to-end/helpers.py | 82 +++++++++++++ test/end-to-end/test_simple.py | 112 +----------------- test/end-to-end/test_symlinks.py | 55 +++++++++ 8 files changed, 241 insertions(+), 137 deletions(-) create mode 100644 test-data/recipes/cache/recipe-symlinks.yaml create mode 100644 test/end-to-end/conftest.py create mode 100644 test/end-to-end/helpers.py create mode 100644 test/end-to-end/test_symlinks.py diff --git a/src/cache.rs b/src/cache.rs index 9f830b370..20b1ab86d 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -116,17 +116,11 @@ impl Output { // check if the symlink starts with the old prefix, and if yes, make the symlink absolute // with the new prefix if source.is_symlink() { - let symlink_target = fs::read_link(&source).into_diagnostic()?; - println!("Current symlink target: {:?}", symlink_target); + let symlink_target = fs::read_link(source).into_diagnostic()?; if let Ok(rest) = symlink_target.strip_prefix(&cache_prefix) { - println!("Rest: {:?}", rest); let new_symlink_target = self.prefix().join(rest); - println!("Creating symlink: {:?} -> {:?}", dest, new_symlink_target); - println!("Removing old symlink: {:?}", dest); fs::remove_file(&dest).into_diagnostic()?; - create_symlink(&dest, &new_symlink_target).into_diagnostic()?; - } else { - println!("Didn't change symlink target"); + create_symlink(&new_symlink_target, &dest).into_diagnostic()?; } } @@ -216,15 +210,20 @@ impl Output { let dest = &prefix_cache_dir.join(stripped); copy_file(file, dest, &mut creation_cache, ©_options).into_diagnostic()?; - // check if the file contains the prefix - let content_type = content_type(file).into_diagnostic()?; - let has_prefix = if content_type.map(|c| c.is_text()).unwrap_or(false) { - contains_prefix_text(file, self.prefix(), self.target_platform()) + // Defend against broken symlinks here! + if !file.is_symlink() { + // check if the file contains the prefix + let content_type = content_type(file).into_diagnostic()?; + let has_prefix = if content_type.map(|c| c.is_text()).unwrap_or(false) { + contains_prefix_text(file, self.prefix(), self.target_platform()) + } else { + contains_prefix_binary(file, self.prefix()) + } + .into_diagnostic()?; + copied_files.push((stripped.to_path_buf(), has_prefix)); } else { - contains_prefix_binary(file, self.prefix()) + copied_files.push((stripped.to_path_buf(), false)); } - .into_diagnostic()?; - copied_files.push((stripped.to_path_buf(), has_prefix)); } // save the cache diff --git a/src/source/copy_dir.rs b/src/source/copy_dir.rs index d2f45881c..dc8df02df 100644 --- a/src/source/copy_dir.rs +++ b/src/source/copy_dir.rs @@ -35,21 +35,29 @@ impl Default for CopyOptions { } /// Cross platform way of creating a symlink +/// Creates a symlink from `link` to `original` +/// The file that is newly created is the `link` file pub(crate) fn create_symlink( - from: impl AsRef, - to: impl AsRef, + original: impl AsRef, + link: impl AsRef, ) -> Result<(), SourceError> { - let from = from.as_ref(); - let to = to.as_ref(); + let original = original.as_ref(); + let link = link.as_ref(); - if from.exists() { - fs_err::remove_file(from)?; + if link.exists() { + fs_err::remove_file(link)?; } - println!("Creating symlink from {:?} to {:?}", from, to); + #[cfg(unix)] - fs_err::os::unix::fs::symlink(to, from)?; + fs_err::os::unix::fs::symlink(original, link)?; #[cfg(windows)] - std::os::windows::fs::symlink_file(to, from)?; + { + if original.is_dir() { + std::os::windows::fs::symlink_dir(original, link)?; + } else { + std::os::windows::fs::symlink_file(original, link)?; + } + } Ok(()) } @@ -77,7 +85,7 @@ pub(crate) fn copy_file( // if file is a symlink, copy it as a symlink if path.is_symlink() { let link_target = fs_err::read_link(path)?; - create_symlink(dest_path, link_target)?; + create_symlink(link_target, dest_path)?; Ok(()) } else { if dest_path.exists() { @@ -509,7 +517,7 @@ mod test { .use_gitignore(false) .run() .unwrap(); - println!("{:?}", copy_dir.copied_paths()); + assert_eq!(copy_dir.copied_paths().len(), 2); let expected = [ dest_dir_3.join("test_dir/test.md"), diff --git a/test-data/recipes/cache/recipe-symlinks.yaml b/test-data/recipes/cache/recipe-symlinks.yaml new file mode 100644 index 000000000..05fb44d56 --- /dev/null +++ b/test-data/recipes/cache/recipe-symlinks.yaml @@ -0,0 +1,27 @@ +recipe: + name: cache-symlinks + version: 1.0.0 + +cache: + build: + script: | + touch $PREFIX/foo.txt + ln -s $PREFIX/foo.txt $PREFIX/foo-symlink.txt + ln -s $PREFIX/foo.txt $PREFIX/absolute-symlink.txt + ln -s $PREFIX/non-existent-file $PREFIX/broken-symlink.txt + ln -s ./foo.txt $PREFIX/relative-symlink.txt + +outputs: + - package: + name: cache-symlinks + build: + files: + include: + - "**/*" + exclude: + - "absolute-symlink.txt" + - package: + name: absolute-cache-symlinks + build: + files: + - "absolute-symlink.txt" diff --git a/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json b/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json index f19669170..3fe102ba1 100644 --- a/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json +++ b/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json @@ -1,5 +1,11 @@ { "paths": [ + { + "_path": "bin/broken", + "path_type": "softlink", + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "size_in_bytes": 9 + }, { "_path": "bin/symlink", "path_type": "softlink", diff --git a/test/end-to-end/conftest.py b/test/end-to-end/conftest.py new file mode 100644 index 000000000..bce17b8ea --- /dev/null +++ b/test/end-to-end/conftest.py @@ -0,0 +1,37 @@ +import os +from pathlib import Path + +import pytest +from helpers import RattlerBuild +from syrupy.extensions.json import JSONSnapshotExtension + + +@pytest.fixture +def rattler_build(): + if os.environ.get("RATTLER_BUILD_PATH"): + return RattlerBuild(os.environ["RATTLER_BUILD_PATH"]) + else: + base_path = Path(__file__).parent.parent.parent + executable_name = "rattler-build" + if os.name == "nt": + executable_name += ".exe" + + release_path = base_path / f"target/release/{executable_name}" + debug_path = base_path / f"target/debug/{executable_name}" + + if release_path.exists(): + return RattlerBuild(release_path) + elif debug_path.exists(): + return RattlerBuild(debug_path) + + raise FileNotFoundError("Could not find rattler-build executable") + + +@pytest.fixture +def snapshot_json(snapshot): + return snapshot.use_extension(JSONSnapshotExtension) + + +@pytest.fixture +def recipes(): + return Path(__file__).parent.parent.parent / "test-data" / "recipes" diff --git a/test/end-to-end/helpers.py b/test/end-to-end/helpers.py new file mode 100644 index 000000000..88d19c111 --- /dev/null +++ b/test/end-to-end/helpers.py @@ -0,0 +1,82 @@ +from pathlib import Path +from subprocess import STDOUT, CalledProcessError, check_output +from typing import Any, Optional + +from conda_package_handling.api import extract + + +class RattlerBuild: + def __init__(self, path): + self.path = path + + def __call__(self, *args: Any, **kwds: Any) -> Any: + try: + return check_output([str(self.path), *args], **kwds).decode("utf-8") + except CalledProcessError as e: + print(e.output) + print(e.stderr) + raise e + + def build_args( + self, + recipe_folder: Path, + output_folder: Path, + variant_config: Optional[Path] = None, + custom_channels: Optional[list[str]] = None, + extra_args: list[str] = None, + ): + if extra_args is None: + extra_args = [] + args = ["build", "--recipe", str(recipe_folder), *extra_args] + if variant_config is not None: + args += ["--variant-config", str(variant_config)] + args += ["--output-dir", str(output_folder)] + args += ["--package-format", str("tar.bz2")] + + if custom_channels: + for c in custom_channels: + args += ["--channel", c] + + return args + + def build( + self, + recipe_folder: Path, + output_folder: Path, + variant_config: Optional[Path] = None, + custom_channels: Optional[list[str]] = None, + extra_args: list[str] = None, + ): + args = self.build_args( + recipe_folder, + output_folder, + variant_config=variant_config, + custom_channels=custom_channels, + extra_args=extra_args, + ) + return self(*args) + + def test(self, package, *args: Any, **kwds: Any) -> Any: + return self("test", "--package-file", package, *args, stderr=STDOUT, **kwds) + + +def get_package(folder: Path, glob="*.tar.bz2"): + if "tar.bz2" not in glob: + glob += "*.tar.bz2" + if "/" not in glob: + glob = "**/" + glob + package_path = next(folder.glob(glob)) + return package_path + + +def get_extracted_package(folder: Path, glob="*.tar.bz2"): + package_path = get_package(folder, glob) + + if package_path.name.endswith(".tar.bz2"): + package_without_extension = package_path.name[: -len(".tar.bz2")] + elif package_path.name.endswith(".conda"): + package_without_extension = package_path.name[: -len(".conda")] + + extract_path = folder / "extract" / package_without_extension + extract(str(package_path), dest_dir=str(extract_path)) + return extract_path diff --git a/test/end-to-end/test_simple.py b/test/end-to-end/test_simple.py index 7ec9bfa43..df0bef906 100644 --- a/test/end-to-end/test_simple.py +++ b/test/end-to-end/test_simple.py @@ -4,94 +4,11 @@ import platform from pathlib import Path from subprocess import DEVNULL, STDOUT, CalledProcessError, check_output -from typing import Any, Optional import pytest import requests import yaml -from conda_package_handling.api import extract -from syrupy.extensions.json import JSONSnapshotExtension - - -class RattlerBuild: - def __init__(self, path): - self.path = path - - def __call__(self, *args: Any, **kwds: Any) -> Any: - try: - return check_output([str(self.path), *args], **kwds).decode("utf-8") - except CalledProcessError as e: - print(e.output) - print(e.stderr) - raise e - - def build_args( - self, - recipe_folder: Path, - output_folder: Path, - variant_config: Optional[Path] = None, - custom_channels: Optional[list[str]] = None, - extra_args: list[str] = None, - ): - if extra_args is None: - extra_args = [] - args = ["build", "--recipe", str(recipe_folder), *extra_args] - if variant_config is not None: - args += ["--variant-config", str(variant_config)] - args += ["--output-dir", str(output_folder)] - args += ["--package-format", str("tar.bz2")] - - if custom_channels: - for c in custom_channels: - args += ["--channel", c] - - return args - - def build( - self, - recipe_folder: Path, - output_folder: Path, - variant_config: Optional[Path] = None, - custom_channels: Optional[list[str]] = None, - extra_args: list[str] = None, - ): - args = self.build_args( - recipe_folder, - output_folder, - variant_config=variant_config, - custom_channels=custom_channels, - extra_args=extra_args, - ) - return self(*args) - - def test(self, package, *args: Any, **kwds: Any) -> Any: - return self("test", "--package-file", package, *args, stderr=STDOUT, **kwds) - - -@pytest.fixture -def rattler_build(): - if os.environ.get("RATTLER_BUILD_PATH"): - return RattlerBuild(os.environ["RATTLER_BUILD_PATH"]) - else: - base_path = Path(__file__).parent.parent.parent - executable_name = "rattler-build" - if os.name == "nt": - executable_name += ".exe" - - release_path = base_path / f"target/release/{executable_name}" - debug_path = base_path / f"target/debug/{executable_name}" - - if release_path.exists(): - return RattlerBuild(release_path) - elif debug_path.exists(): - return RattlerBuild(debug_path) - - raise FileNotFoundError("Could not find rattler-build executable") - - -@pytest.fixture -def snapshot_json(snapshot): - return snapshot.use_extension(JSONSnapshotExtension) +from helpers import RattlerBuild, get_extracted_package, get_package def test_functionality(rattler_build: RattlerBuild): @@ -100,33 +17,6 @@ def test_functionality(rattler_build: RattlerBuild): assert text[0] == f"Usage: rattler-build{suffix} [OPTIONS] [COMMAND]" -@pytest.fixture -def recipes(): - return Path(__file__).parent.parent.parent / "test-data" / "recipes" - - -def get_package(folder: Path, glob="*.tar.bz2"): - if "tar.bz2" not in glob: - glob += "*.tar.bz2" - if "/" not in glob: - glob = "**/" + glob - package_path = next(folder.glob(glob)) - return package_path - - -def get_extracted_package(folder: Path, glob="*.tar.bz2"): - package_path = get_package(folder, glob) - - if package_path.name.endswith(".tar.bz2"): - package_without_extension = package_path.name[: -len(".tar.bz2")] - elif package_path.name.endswith(".conda"): - package_without_extension = package_path.name[: -len(".conda")] - - extract_path = folder / "extract" / package_without_extension - extract(str(package_path), dest_dir=str(extract_path)) - return extract_path - - def test_license_glob(rattler_build: RattlerBuild, recipes: Path, tmp_path: Path): rattler_build.build(recipes / "globtest", tmp_path) pkg = get_extracted_package(tmp_path, "globtest") diff --git a/test/end-to-end/test_symlinks.py b/test/end-to-end/test_symlinks.py new file mode 100644 index 000000000..ded61842b --- /dev/null +++ b/test/end-to-end/test_symlinks.py @@ -0,0 +1,55 @@ +import json +import os +from pathlib import Path + +import pytest +from helpers import RattlerBuild, get_extracted_package + + +@pytest.skipif(os.name == "nt", reason="recipe does not support execution on windows") +def test_symlink_cache( + rattler_build: RattlerBuild, recipes: Path, tmp_path: Path, snapshot_json +): + rattler_build.build( + recipes / "cache/recipe-symlinks.yaml", tmp_path, extra_args=["--experimental"] + ) + + pkg = get_extracted_package(tmp_path, "absolute-cache-symlinks") + assert pkg.exists() + link_file = pkg / "absolute-symlink.txt" + assert link_file.is_symlink() + # assure that this is a relative link + assert link_file.readlink() == Path("foo.txt") + + link_target = link_file.resolve() + assert link_target == (pkg / "foo.txt") + + pkg = get_extracted_package(tmp_path, "cache-symlinks") + + paths_json = pkg / "info/paths.json" + j = json.loads(paths_json.read_text()) + assert snapshot_json == j + + paths = j["paths"] + assert len(paths) == 4 + for p in paths: + if p["_path"].endswith("symlink.txt"): + assert p["path_type"] == "softlink" + assert ( + p["sha256"] + == "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + ) + + foo_symlink = pkg / "foo-symlink.txt" + assert foo_symlink.exists() + assert foo_symlink.is_symlink() + assert not foo_symlink.readlink().is_absolute() + + broken_symlink = pkg / "broken-symlink.txt" + assert broken_symlink.is_symlink() + assert broken_symlink.readlink() == Path("non-existent-file") + assert not broken_symlink.resolve().exists() + + relative_symlink = pkg / "relative-symlink.txt" + assert relative_symlink.is_symlink() + assert relative_symlink.readlink() == Path("foo.txt") From c90276282cd4115ffcd419c0e20b4fdb9d870733 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Wed, 31 Jul 2024 13:07:43 +0200 Subject: [PATCH 4/9] fix test --- test/end-to-end/test_symlinks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/end-to-end/test_symlinks.py b/test/end-to-end/test_symlinks.py index ded61842b..c8c8768df 100644 --- a/test/end-to-end/test_symlinks.py +++ b/test/end-to-end/test_symlinks.py @@ -6,7 +6,9 @@ from helpers import RattlerBuild, get_extracted_package -@pytest.skipif(os.name == "nt", reason="recipe does not support execution on windows") +@pytest.mark.skipif( + os.name == "nt", reason="recipe does not support execution on windows" +) def test_symlink_cache( rattler_build: RattlerBuild, recipes: Path, tmp_path: Path, snapshot_json ): From 0c841af4e0649bdbf031dca237e0199902aa2c3f Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Wed, 31 Jul 2024 13:18:05 +0200 Subject: [PATCH 5/9] do not package broken links --- src/packaging/metadata.rs | 28 +++++++++++++++---- .../test_simple/test_symlink_recipe.json | 6 ---- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/packaging/metadata.rs b/src/packaging/metadata.rs index 5f67f1059..883670f24 100644 --- a/src/packaging/metadata.rs +++ b/src/packaging/metadata.rs @@ -351,11 +351,29 @@ impl Output { if !p.exists() { if p.is_symlink() { - tracing::warn!( - "symlink target not part of _this_ package: {:?} -> {:?}", - &p, - fs::read_link(p)? - ); + // check if the file is in the prefix + if let Ok(link_target) = p.read_link() { + if link_target.is_relative() { + let resolved_path = temp_files.encoded_prefix.join(&link_target); + if !resolved_path.exists() { + tracing::warn!( + "symlink target not part of _this_ package: {:?} -> {:?}", + &p, + &link_target + ); + // Think about continuing here or packaging broken symlinks + continue; + } + } else { + tracing::warn!( + "packaging an absolute symlink to outside the prefix {:?} -> {:?}", + &p, + link_target + ); + } + } else { + tracing::warn!("could not read symlink {:?}", &p); + } } else { tracing::warn!("file does not exist: {:?}", &p); continue; diff --git a/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json b/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json index 3fe102ba1..f19669170 100644 --- a/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json +++ b/test/end-to-end/__snapshots__/test_simple/test_symlink_recipe.json @@ -1,11 +1,5 @@ { "paths": [ - { - "_path": "bin/broken", - "path_type": "softlink", - "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", - "size_in_bytes": 9 - }, { "_path": "bin/symlink", "path_type": "softlink", From 441c1154d0d68699f3c67c5125f242a2a6c2eed0 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Wed, 31 Jul 2024 13:59:21 +0200 Subject: [PATCH 6/9] add missing snapshot --- .../test_symlinks/test_symlink_cache.json | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json diff --git a/test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json b/test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json new file mode 100644 index 000000000..f24fa9695 --- /dev/null +++ b/test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json @@ -0,0 +1,23 @@ +{ + "paths": [ + { + "_path": "foo-symlink.txt", + "path_type": "softlink", + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "size_in_bytes": 7 + }, + { + "_path": "foo.txt", + "path_type": "hardlink", + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "size_in_bytes": 0 + }, + { + "_path": "relative-symlink.txt", + "path_type": "softlink", + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "size_in_bytes": 9 + } + ], + "paths_version": 1 +} From eba49290a2efba32e3add832866e0e9c5a5937fd Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Wed, 31 Jul 2024 14:01:55 +0200 Subject: [PATCH 7/9] do not add broken symlinks --- test/end-to-end/test_symlinks.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/end-to-end/test_symlinks.py b/test/end-to-end/test_symlinks.py index c8c8768df..0822dc921 100644 --- a/test/end-to-end/test_symlinks.py +++ b/test/end-to-end/test_symlinks.py @@ -33,7 +33,7 @@ def test_symlink_cache( assert snapshot_json == j paths = j["paths"] - assert len(paths) == 4 + assert len(paths) == 3 for p in paths: if p["_path"].endswith("symlink.txt"): assert p["path_type"] == "softlink" @@ -48,9 +48,10 @@ def test_symlink_cache( assert not foo_symlink.readlink().is_absolute() broken_symlink = pkg / "broken-symlink.txt" - assert broken_symlink.is_symlink() - assert broken_symlink.readlink() == Path("non-existent-file") - assert not broken_symlink.resolve().exists() + assert not broken_symlink.exists() + # assert broken_symlink.is_symlink() + # assert broken_symlink.readlink() == Path("non-existent-file") + # assert not broken_symlink.resolve().exists() relative_symlink = pkg / "relative-symlink.txt" assert relative_symlink.is_symlink() From c8146aa9168f3ab6f323d660cb0c73008107db54 Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Wed, 31 Jul 2024 19:41:54 +0200 Subject: [PATCH 8/9] use the proper relative link --- src/packaging/metadata.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/packaging/metadata.rs b/src/packaging/metadata.rs index 883670f24..82e48ebba 100644 --- a/src/packaging/metadata.rs +++ b/src/packaging/metadata.rs @@ -354,13 +354,20 @@ impl Output { // check if the file is in the prefix if let Ok(link_target) = p.read_link() { if link_target.is_relative() { - let resolved_path = temp_files.encoded_prefix.join(&link_target); + let Some(relative_path_parent) = relative_path.parent() else { + tracing::warn!("could not get parent of symlink {:?}", &p); + continue; + }; + + let resolved_path = temp_files.encoded_prefix.join(relative_path_parent).join(&link_target); + if !resolved_path.exists() { tracing::warn!( - "symlink target not part of _this_ package: {:?} -> {:?}", + "symlink target not part of this package: {:?} -> {:?}", &p, &link_target ); + // Think about continuing here or packaging broken symlinks continue; } From 21666c886915f9befeb604d4a9bdb7f06b38773f Mon Sep 17 00:00:00 2001 From: Wolf Vollprecht Date: Wed, 31 Jul 2024 21:03:00 +0200 Subject: [PATCH 9/9] add test --- src/packaging/metadata.rs | 5 ++++- test-data/recipes/cache/recipe-symlinks.yaml | 6 ++++++ .../test_symlinks/test_symlink_cache.json | 12 ++++++++++++ test/end-to-end/test_symlinks.py | 15 ++++++++++----- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/packaging/metadata.rs b/src/packaging/metadata.rs index 82e48ebba..736244983 100644 --- a/src/packaging/metadata.rs +++ b/src/packaging/metadata.rs @@ -359,7 +359,10 @@ impl Output { continue; }; - let resolved_path = temp_files.encoded_prefix.join(relative_path_parent).join(&link_target); + let resolved_path = temp_files + .encoded_prefix + .join(relative_path_parent) + .join(&link_target); if !resolved_path.exists() { tracing::warn!( diff --git a/test-data/recipes/cache/recipe-symlinks.yaml b/test-data/recipes/cache/recipe-symlinks.yaml index 05fb44d56..8c9ec28fc 100644 --- a/test-data/recipes/cache/recipe-symlinks.yaml +++ b/test-data/recipes/cache/recipe-symlinks.yaml @@ -5,6 +5,10 @@ recipe: cache: build: script: | + mkdir -p $PREFIX/bin + touch $PREFIX/bin/exe + ln -s $PREFIX/bin/exe $PREFIX/bin/exe-symlink + ln -s $PREFIX/bin/exe $PREFIX/bin/absolute-exe-symlink touch $PREFIX/foo.txt ln -s $PREFIX/foo.txt $PREFIX/foo-symlink.txt ln -s $PREFIX/foo.txt $PREFIX/absolute-symlink.txt @@ -20,8 +24,10 @@ outputs: - "**/*" exclude: - "absolute-symlink.txt" + - "bin/absolute-exe-symlink" - package: name: absolute-cache-symlinks build: files: - "absolute-symlink.txt" + - "bin/absolute-exe-symlink" diff --git a/test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json b/test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json index f24fa9695..d2d735b8a 100644 --- a/test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json +++ b/test/end-to-end/__snapshots__/test_symlinks/test_symlink_cache.json @@ -1,5 +1,17 @@ { "paths": [ + { + "_path": "bin/exe", + "path_type": "hardlink", + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "size_in_bytes": 0 + }, + { + "_path": "bin/exe-symlink", + "path_type": "softlink", + "sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "size_in_bytes": 3 + }, { "_path": "foo-symlink.txt", "path_type": "softlink", diff --git a/test/end-to-end/test_symlinks.py b/test/end-to-end/test_symlinks.py index 0822dc921..e269a37ff 100644 --- a/test/end-to-end/test_symlinks.py +++ b/test/end-to-end/test_symlinks.py @@ -26,6 +26,10 @@ def test_symlink_cache( link_target = link_file.resolve() assert link_target == (pkg / "foo.txt") + link_file = pkg / "bin/absolute-exe-symlink" + assert link_file.is_symlink() + assert link_file.readlink() == Path("exe") + pkg = get_extracted_package(tmp_path, "cache-symlinks") paths_json = pkg / "info/paths.json" @@ -33,9 +37,9 @@ def test_symlink_cache( assert snapshot_json == j paths = j["paths"] - assert len(paths) == 3 + assert len(paths) == 5 for p in paths: - if p["_path"].endswith("symlink.txt"): + if "symlink" in p["_path"]: assert p["path_type"] == "softlink" assert ( p["sha256"] @@ -49,10 +53,11 @@ def test_symlink_cache( broken_symlink = pkg / "broken-symlink.txt" assert not broken_symlink.exists() - # assert broken_symlink.is_symlink() - # assert broken_symlink.readlink() == Path("non-existent-file") - # assert not broken_symlink.resolve().exists() relative_symlink = pkg / "relative-symlink.txt" assert relative_symlink.is_symlink() assert relative_symlink.readlink() == Path("foo.txt") + + relative_symlink = pkg / "bin/exe-symlink" + assert relative_symlink.is_symlink() + assert relative_symlink.readlink() == Path("exe")