From 4f5402dd50f98fe1661015ef6c1bf4a7d272486b Mon Sep 17 00:00:00 2001 From: Laurenz Date: Thu, 31 Oct 2024 12:12:19 +0100 Subject: [PATCH] Work around relative symlinks that break when replicated inside runfiles (#247) There is an edge case where rules_appimage creates a relative symlink that points to a file or dir which will not exist in the AppDir. This can happen in situations where `.../foo.runfiles/_main/_solib_k8/_U_A_A_Umain~_Urepo_Urules~foo_S_S_Clibfoo.so___Ulib/libfoo.so` is a symlink pointing to "libfoo.so.12" but which will not exist in the same dir but instead lives in `.../foo.runfiles/_main/_solib_k8/_U_A_A_Umain~_Urepo_Urules~foo_S_S_Clibfoo.so.12___Ulib/libfoo.so.12` (note the difference in directory names) For now we just don't create a relative symlink from libfoo.so to libfoo.so.12 but instead copy the resolved libfoo.so twice. mksquashfs will deduplicate it so no additional storage is needed regardless of file size (unless extracted). The downside is that the symlink structure will not look the same as in the source. --- appimage/private/mkappdir.py | 37 ++++++++++++++++++++++++++------ pyproject.toml | 2 +- tests/BUILD | 6 ++++-- tests/relative_symlink_so/BUILD | 37 ++++++++++++++++++++++++++++++++ tests/relative_symlink_so/foo.S | 1 + tests/test.cc | 38 ++++++++++++++++++++++++++++++--- 6 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 tests/relative_symlink_so/BUILD create mode 100644 tests/relative_symlink_so/foo.S diff --git a/appimage/private/mkappdir.py b/appimage/private/mkappdir.py index 0f222c8..9e504f9 100644 --- a/appimage/private/mkappdir.py +++ b/appimage/private/mkappdir.py @@ -197,6 +197,10 @@ def _move_relative_symlinks_in_files_to_their_own_section(manifest_data: _Manife new_manifest_data = copy.deepcopy(manifest_data) new_manifest_data.files.clear() new_manifest_data.relative_symlinks.clear() + files_that_will_exist = {entry.dst for entry in manifest_data.files} + dirs_that_will_exist: set[str] = set() + for file in files_that_will_exist: + dirs_that_will_exist.update(map(str, get_all_parent_dirs(file))) for entry in manifest_data.files: src = Path(entry.src) @@ -221,13 +225,14 @@ def _move_relative_symlinks_in_files_to_their_own_section(manifest_data: _Manife if target.is_symlink(): linkdst = target.readlink() - elif not target.is_absolute(): - # This was a relative symlink that we resolved in the last step already. - linkdst = target else: - # The src is a symlink pointing to the a regular file in the Bazel cache. - new_manifest_data.files.append(entry) - continue + if not target.is_absolute(): + # This was a relative symlink that we resolved in the last step already. + linkdst = target + else: + # The src is a symlink pointing to the a regular file in the Bazel cache. + new_manifest_data.files.append(entry) + continue if linkdst.is_absolute(): # Absolute symlinks are ok to keep in the files section because we are going to resolve them before copying. @@ -239,7 +244,25 @@ def _move_relative_symlinks_in_files_to_their_own_section(manifest_data: _Manife # (although squashfs would deduplicate it). # Note that the link target may _not_ be reachable if it is not declared as input itself. # Users need to ensure that whatever shall be available at runtime is properly declared as data dependency. - new_manifest_data.relative_symlinks.append(_ManifestLink(linkname=entry.dst, target=os.fspath(linkdst))) + full_linkdest = (Path(entry.dst).parent / linkdst).as_posix() + if Path(entry.src).is_dir(): + will_exist = os.path.normpath(full_linkdest) in dirs_that_will_exist + else: + will_exist = full_linkdest in files_that_will_exist + is_supposed_to_be_dangling = not Path(entry.src).exists() + if not will_exist and not is_supposed_to_be_dangling: + # Would create a symlink that points to a file or dir which will not exist in the AppDir. + # This can happen in situations where + # .../foo.runfiles/_main/_solib_k8/_U_A_A_Umain~_Urepo_Urules~foo_S_S_Clibfoo.so___Ulib/libfoo.so + # is a symlink pointing to "libfoo.so.12" but which will not exist in the same dir but instead lives in + # .../foo.runfiles/_main/_solib_k8/_U_A_A_Umain~_Urepo_Urules~foo_S_S_Clibfoo.so.12___Ulib/libfoo.so.12 + # For now we just don't create a symlink but copy the resolved file instead. mksquashfs will deduplicate + # it so no additional storage is needed regardless of file size (unless extracted). + # The downside is that the symlink structure will not look the same as in the source. + new_manifest_data.files.append(entry) + else: + # Create the entry as relative symlink + new_manifest_data.relative_symlinks.append(_ManifestLink(linkname=entry.dst, target=os.fspath(linkdst))) return new_manifest_data diff --git a/pyproject.toml b/pyproject.toml index 217d878..3bf7c7e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,7 +50,7 @@ select = [ "PERF", "RUF", ] -ignore = ["COM812", "D103", "D203", "D213", "PLR2004", "TRY003"] +ignore = ["COM812", "D103", "D203", "D213", "PLR2004", "PLR5501", "PLR0912", "TRY003"] [tool.ruff] line-length = 120 diff --git a/tests/BUILD b/tests/BUILD index 43a0453..86573e7 100644 --- a/tests/BUILD +++ b/tests/BUILD @@ -1,5 +1,5 @@ load("@rules_appimage_py_deps//:requirements.bzl", "requirement") -load("@rules_cc//cc:defs.bzl", "cc_binary") +load("@rules_cc//cc:defs.bzl", "cc_test") load("@rules_python//python:defs.bzl", "py_binary", "py_test") load("//appimage:appimage.bzl", "appimage", "appimage_test") load(":testrules.bzl", "declared_symlink", "runfiles_symlink", "transitioned_cc_binary") @@ -30,7 +30,7 @@ appimage_test( target_compatible_with = ["@platforms//os:linux"], ) -cc_binary( +cc_test( name = "test_cc", srcs = ["test.cc"], copts = ["-std=c++20"], @@ -38,6 +38,8 @@ cc_binary( "MY_APPIMAGE_ENV": "original", "MY_BINARY_ENV": "not lost", }, + tags = ["manual"], # This test is not supposed to work outside its appimage + deps = ["//tests/relative_symlink_so:libfoo"], ) appimage_test( diff --git a/tests/relative_symlink_so/BUILD b/tests/relative_symlink_so/BUILD new file mode 100644 index 0000000..ca91b16 --- /dev/null +++ b/tests/relative_symlink_so/BUILD @@ -0,0 +1,37 @@ +load("@rules_cc//cc:defs.bzl", "cc_import", "cc_library") + +package(default_visibility = ["//tests:__subpackages__"]) + +genrule( + name = "make_libfoo", + srcs = ["foo.S"], + outs = [ + "libfoo.so.1", + "libfoo.so", + ], + cmd = "\n".join([ + "$(CC) -nostdlib -fPIC -shared -o $(RULEDIR)/libfoo.so.1 $<", + "$(STRIP) --strip-unneeded $(RULEDIR)/libfoo.so.1", + "ln -s libfoo.so.1 $(RULEDIR)/libfoo.so", + ]), + target_compatible_with = ["@platforms//os:linux"], + toolchains = ["@bazel_tools//tools/cpp:current_cc_toolchain"], +) + +cc_import( + name = "libfoo_so", + shared_library = ":libfoo.so", +) + +cc_import( + name = "libfoo_so_1", + shared_library = ":libfoo.so.1", +) + +cc_library( + name = "libfoo", + deps = [ + ":libfoo_so", + ":libfoo_so_1", + ], +) diff --git a/tests/relative_symlink_so/foo.S b/tests/relative_symlink_so/foo.S new file mode 100644 index 0000000..13a0ba1 --- /dev/null +++ b/tests/relative_symlink_so/foo.S @@ -0,0 +1 @@ +.section .text diff --git a/tests/test.cc b/tests/test.cc index 08ee9a5..5e72eaa 100644 --- a/tests/test.cc +++ b/tests/test.cc @@ -2,9 +2,16 @@ #include #include +#include + +namespace fs = std::filesystem; + int main(int argc, char **argv, char **envp) { (void)argc; (void)argv; + + int ret{EXIT_SUCCESS}; + // Go through the environment variables and find the one we set in the BUILD. // When running inside the appimage, we want the env to not be lost. bool have_binary_env = false; @@ -23,11 +30,36 @@ int main(int argc, char **argv, char **envp) { } if (!have_binary_env) { std::cerr << "MY_BINARY_ENV not found or has wrong value" << std::endl; - return EXIT_FAILURE; + ret |= EXIT_FAILURE; } if (!have_appimage_env) { std::cerr << "MY_APPIMAGE_ENV not found or has wrong value" << std::endl; - return EXIT_FAILURE; + ret |= EXIT_FAILURE; + } + + // Check for broken symlinks + std::cout << "\n"; + bool libfoo_found{false}; // Make sure the test "libfoo.*.so" exists + for (const auto &entry : + fs::recursive_directory_iterator(fs::current_path())) { + if (entry.path().filename() == "libfoo.so") { + std::cout << "libfoo.so found" << std::endl; + libfoo_found = true; + } + if (fs::is_symlink(entry.path())) { + fs::path symlink_target = fs::read_symlink(entry.path()); + std::cerr << entry.path() << " -> " << symlink_target; + if (!fs::exists(entry.path().parent_path() / symlink_target)) { + std::cerr << " is broken" << std::endl; + ret |= EXIT_FAILURE; + } + std::cerr << std::endl; + } } - return EXIT_SUCCESS; + if (!libfoo_found) { + std::cerr << "libfoo.so not found" << std::endl; + ret |= EXIT_FAILURE; + } + + return ret; }