Work around relative symlinks that break when replicated inside runfi…

…les (#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

@@ -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
 

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

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,14 +30,16 @@ appimage_test(
     target_compatible_with = ["@platforms//os:linux"],
 )
 
-cc_binary(
+cc_test(
     name = "test_cc",
     srcs = ["test.cc"],
     copts = ["-std=c++20"],
     env = {
         "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(

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",
+    ],
+)

tests/relative_symlink_so/foo.S

@@ -0,0 +1 @@
+.section .text

tests/test.cc

@@ -2,9 +2,16 @@
 #include <iostream>
 #include <string>
 
+#include <filesystem>
+
+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;
 }