Repository: AppImageCrafters/appimage-builder (language: python)

Issue:
Consider making Docker test cases container volumes writable
While writing test cases for the arguably not very well supported use case of running AppImages generated with appimage-builder on Alpine Linux (a musl-based environment), I've stumbled upon a behavior change from v0.9.2 in v1.0.0-alpha.1 onwards, apparently caused by https://github.com/AppImageCrafters/appimage-builder/commit/a44066ce4783dfb8093727c48b0a3a6b74fcc6d8: glibc-related files are now copied to `runtime/compat`, instead of their usual `/lib`, `/lib64` and similar folders.

To get the AppImage running on Alpine, previously it was enough to extract it on a glibc host and copy the AppDir contents to the root directory of the Alpine environment. This worked for me, as I'm using Alpine in disposable Docker containers, and it also worked on the test cases specified in the recipe too.

However, now it's necessary to run a command like `cp -R /runtime/compat/* /` for the AppRun executable to work on such an environment. The AppRun executable itself never ran anyway, because it's linked against glibc. I confirmed running that command worked on a disposable Docker container, but when changing the test case command to `sh -c 'cp -R runtime/compat/* . && ./AppRun --version'` in order to perform the copy before running the application, I was greeted with errors like:

```
cp: can't create directory './etc/ld.so.conf.d': Read-only file system
cp: can't create './lib/x86_64-linux-gnu/libgcc_s.so.1': Read-only file system
cp: can't create './lib/x86_64-linux-gnu/ld-2.33.so': Read-only file system
cp: can't create './lib/x86_64-linux-gnu/libBrokenLocale-2.33.so': Read-only file system
cp: can't create './lib/x86_64-linux-gnu/libSegFault.so': Read-only file system
cp: can't create './lib/x86_64-linux-gnu/libanl-2.33.so': Read-only file system
cp: can't create './lib/x86_64-linux-gnu/libc-2.33.so': Read-only file system
```

Given that modifying the test container filesystem is necessary to get the application to work, this is a regression in behavior for my use case from v0.9.2. I'd be very happy if the container filesystem was just made writable, but maybe it'd not be a bad idea to support Alpine better in general, if it's in scope for this project. I could help :smile: 


Discussion / hints:
Hello @AlexTMjugador as you already noticed some changes were made in the way glibc is dealt with which needs to be documented at some point. I'll try to make a quick summary for you. 

In "v0.9" we patched the executable elf files with a linker path on `/tmp/appimage-xxxxxxx-ld-linux.so` but this is an open vector for attacker who may replace that file and sneak into the application execution workflow. Therefore we moved on using relative paths for all interpreters on "v1". Now, before executing any bundled binary (or script) we chdir to `APPDIR/runtime/compat` (or `APPDIR/runtime/default` in case the system libc is newer that the one bundled). This makes the relative path valid and allows the resolution of the `ld-linux.so` or other interpreter after that we restore the working directory to the original location.

Regarding the AppRun now being a dynamically linked executable is because right now the only reliable way of getting the system libc version we know is by calling `gnu_get_libc_version`. I have a [StackOverflow question on this topic](https://stackoverflow.com/questions/71070969/how-to-extract-and-compare-the-libc-versions-at-runtime) it would be great if you could help us find a way that allow us keeping AppRun statically linked.

Regarding Alpine, I would love to also support it I'm just have been a bit busy with the above changes. Your help will be really appreciated.

Forgot to mention that AppImage are always mounted as readonly by design, so I don't think that would be the way to go.
Thank you for the quick summary of the libc-related changes. It was useful to understand the rationale behind them :smile: 

About getting the glibc version, indeed there barely are any elegant ways that strictly meet your requirements. However, I'm thinking of running the ELF dynamic link interpreter binary directly, which is part of glibc, with the `--version` command line switch. I know that parsing command output was not desired in that StackOverflow question you linked to, but it's worth mentioning that, since this command line switch was introduced two years ago, its format was not changed at all. It also is part of the upstream glibc code, so any modern distro should support it; see this [git blame](https://github.com/bminor/glibc/blame/b92a49359f33a461db080a33940d73f47c756126/elf/dl-usage.c#L43) for the relevant source code and change history. Parsing the first line to extract the version string is also easy, not requiring regexes or anything complex like that.

According to the seemingly distro-agnostic [`man 8 ld.so`](https://man7.org/linux/man-pages/man8/ld.so.8.html), this file should be at `/lib/ld-linux.so.2` for systems with glibc 2.xx. On my personal x64 Debian unstable environment I get the following output (note the glibc version at the end of the first line):

```
$ /lib/ld-linux.so.2 --version
ld.so (Debian GLIBC 2.33-7) release release version 2.33.
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.
```

For compatibility with other distros or environments that do not have the interpreter or a symlink to it at `/lib/ld-linux.so.2`, it seems feasible to hardcode additional common interpreter binary locations. If the interpreter is not found at any of these locations, does not support `--version` (it was introduced in glibc 2.33), or generates unexpected output, it sounds reasonable to assume that the system glibc version is unknown or pretty old, and the compat glibc should be used.

In the (seemingly unlikely) event that the version output format is changed, I guess it should be possible to add more code to handle both the newer and older formats. This is not pretty, but the alternatives of looking for magic bytes in the glibc binaries are not much better either, and from a practical perspective the glibc developers should think twice about making changes to the output of a binary that may be consumed by who knows what shell scripts and the like.

> Forgot to mention that AppImage are always mounted as readonly by design, so I don't think that would be the way to go.

I understand that while executing the AppRun the mount should be read-only, and that indeed does help catching problems. However, to me it sounds like it could be a good idea to add a `preparation-command` key to the recipe schema, mounting the volume read-write during its execution to allow it to do filesystem changes. When that command completes, the volume could, at least theoretically (I didn't look into whether this is really feasible), be remounted read-only, and the AppRun test `command` executed.

This way users can slightly tweak the Docker image they are using to their needs, while the AppRun still runs as it should in a read-only mount. The alternative is to bring your own Docker images, but that's clumsier, and for some reason uploading images to Docker Hub takes ages for me, not even remotely taking advantage of my available upload bandwidth.
I'm running KDE Neon based on Ubuntu focal which ships glibc 2.31 and the `--version` argument is not there yet. Therefore to adopt this approach we will have to wait for a few years :(
@AlexTMjugador By the way regarding the issue title, will a `before_command` script be useful to you?
Yes, that command would be pretty useful!

> Therefore to adopt this approach we will have to wait for a few years :(

If for AppImage purposes is enough to roughly classify between unknown or < 2.33 glibc, and then precisely between >= 2.33 glibc, it could work today too. Most applications nowadays probably target glibc 2.28 or 2.31, so, if the system glibc is < 2.33, then we don't lose much by using the bundled glibc, even if it's slightly older than what might be present in the system. Eventually, as distros update glibc, AppImages will start using the newer system glibc! :tada:

Fix patch:
diff --git a/appimagebuilder/modules/appimage.py b/appimagebuilder/modules/appimage.py
index 64321232..8e6dd95d 100644
--- a/appimagebuilder/modules/appimage.py
+++ b/appimagebuilder/modules/appimage.py
@@ -25,7 +25,9 @@ def __init__(self, context: Context):
         self.app_version = context.app_info.version
 
         self.target_arch = context.recipe.AppImage.arch()
-        self.update_information = context.recipe.AppImage["update-information"]() or "None"
+        self.update_information = (
+            context.recipe.AppImage["update-information"]() or "None"
+        )
         self.guess_update_information = False
 
         if self.update_information == "None":
diff --git a/appimagebuilder/modules/deploy/files/dependencies_resolver/elf_resolver.py b/appimagebuilder/modules/deploy/files/dependencies_resolver/elf_resolver.py
index 1d7cb898..880f6ee9 100644
--- a/appimagebuilder/modules/deploy/files/dependencies_resolver/elf_resolver.py
+++ b/appimagebuilder/modules/deploy/files/dependencies_resolver/elf_resolver.py
@@ -48,16 +48,21 @@ def _resolved_needed_using_ldd(file):
 
         # set locale to C to avoid output variations due localizations
         _proc_env = os.environ.copy()
-        _proc_env['LC_ALL'] = "C"
+        _proc_env["LC_ALL"] = "C"
 
-        _proc = subprocess.run([ldd_bin, str(file)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=_proc_env)
+        _proc = subprocess.run(
+            [ldd_bin, str(file)],
+            stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE,
+            env=_proc_env,
+        )
 
         # process output
         output = _proc.stdout.decode()
         needed_libraries = []
         for line in output.splitlines():
             # match paths in lines
-            path_search = re.search(r'(/.*)\s?\(', line)
+            path_search = re.search(r"(/.*)\s?\(", line)
             if path_search:
                 path = path_search.group(1)
                 needed_libraries.append(path.strip())
diff --git a/appimagebuilder/modules/deploy/files/dependencies_resolver/resolver.py b/appimagebuilder/modules/deploy/files/dependencies_resolver/resolver.py
index 25d00157..c1d21c6c 100644
--- a/appimagebuilder/modules/deploy/files/dependencies_resolver/resolver.py
+++ b/appimagebuilder/modules/deploy/files/dependencies_resolver/resolver.py
@@ -21,9 +21,7 @@ class Resolver(BaseResolver):
     """
 
     def __init__(self):
-        self.resolvers = [
-            ElfResolver()
-        ]
+        self.resolvers = [ElfResolver()]
 
     def resolve(self, files: [pathlib.Path]) -> [pathlib.Path]:
         results = set()
diff --git a/appimagebuilder/modules/setup/generator.py b/appimagebuilder/modules/setup/generator.py
index 1abf84f8..e2d48780 100644
--- a/appimagebuilder/modules/setup/generator.py
+++ b/appimagebuilder/modules/setup/generator.py
@@ -61,7 +61,9 @@ def generate(self):
         runtime_env = self._configure_runtime_environment()
 
         scanner = ExecutablesScanner(self.appdir_path, self.finder)
-        resolver = AppRunBinariesResolver(self.apprun_version, self.apprun_debug, self.context.build_dir)
+        resolver = AppRunBinariesResolver(
+            self.apprun_version, self.apprun_debug, self.context.build_dir
+        )
         patcher = ExecutablesPatcher()
 
         executables = self._find_executables(scanner)
@@ -216,9 +218,9 @@ def parse_env_input(self, user_env_input):
                 v = v.replace("${APPDIR}", self.appdir_path.__str__())
 
                 if (
-                        k == "PATH"
-                        or k == "APPDIR_LIBRARY_PATH"
-                        or k == "APPDIR_LIBC_LIBRARY_PATH"
+                    k == "PATH"
+                    or k == "APPDIR_LIBRARY_PATH"
+                    or k == "APPDIR_LIBC_LIBRARY_PATH"
                 ):
                     v = v.split(":")
 
@@ -227,7 +229,7 @@ def parse_env_input(self, user_env_input):
         return env
 
     def _deploy_apprun_hooks(
-            self, apprun_binaries_resolver: AppRunBinariesResolver, runtime_env: Environment
+        self, apprun_binaries_resolver: AppRunBinariesResolver, runtime_env: Environment
     ):
 
         for arch in self.apprun_arch:
diff --git a/appimagebuilder/modules/setup/icon_bundler.py b/appimagebuilder/modules/setup/icon_bundler.py
index 2247d9ff..f5aa1ef2 100644
--- a/appimagebuilder/modules/setup/icon_bundler.py
+++ b/appimagebuilder/modules/setup/icon_bundler.py
@@ -47,11 +47,11 @@ def _get_icon_path(self):
         # include target AppDir paths
         search_paths = [
             os.path.join(self.app_dir, "usr", "share"),
-            os.path.join(self.app_dir, "usr", "local", "share")
+            os.path.join(self.app_dir, "usr", "local", "share"),
         ]
 
         # include system data dirs
-        data_dirs = os.getenv('XDG_DATA_DIRS', default="")
+        data_dirs = os.getenv("XDG_DATA_DIRS", default="")
         search_paths.extend(data_dirs.split(":"))
 
         refined_search_paths = []
diff --git a/appimagebuilder/recipe/schema.py b/appimagebuilder/recipe/schema.py
index 62040350..bf31b851 100644
--- a/appimagebuilder/recipe/schema.py
+++ b/appimagebuilder/recipe/schema.py
@@ -48,6 +48,7 @@ def __init__(self):
             str: {
                 "image": str,
                 "command": str,
+                Optional("before_command"): self.script,
                 Optional("use_host_x"): bool,
                 Optional("env"): {str: Or(str, int, bool)},
             }
diff --git a/recipes/bash/AppImageBuilder.yml b/recipes/bash/AppImageBuilder.yml
index 91335689..3684f3a2 100644
--- a/recipes/bash/AppImageBuilder.yml
+++ b/recipes/bash/AppImageBuilder.yml
@@ -49,6 +49,7 @@ AppDir:
   test:
     debian:
       image: appimagecrafters/tests-env:debian-stable
+      before_command: "pwd"
       command: "./AppRun ./run_tests.sh"
     centos:
       image: appimagecrafters/tests-env:centos-7


Test patch:
diff --git a/appimagebuilder/commands/run_test.py b/appimagebuilder/commands/run_test.py
index 394c283d..a586fa89 100644
--- a/appimagebuilder/commands/run_test.py
+++ b/appimagebuilder/commands/run_test.py
@@ -47,8 +47,14 @@ def _load_tests(self, test_settings: {}):
             if isinstance(env, dict):
                 env = ["%s=%s" % (k, v) for k, v in env.items()]
 
-            test = ExecutionTest(appdir=self.app_dir, name=name, image=data_accessor.image(),
-                                 command=data_accessor.command(), env=env)
+            test = ExecutionTest(
+                appdir=self.app_dir,
+                name=name,
+                image=data_accessor.image(),
+                command=data_accessor.command(),
+                before_command=data_accessor.before_command(),
+                env=env,
+            )
             test_cases.append(test)
 
         return test_cases
diff --git a/appimagebuilder/modules/generate/recipe_sections/test_section_generator.py b/appimagebuilder/modules/generate/recipe_sections/test_section_generator.py
index c497d8bc..139295bf 100644
--- a/appimagebuilder/modules/generate/recipe_sections/test_section_generator.py
+++ b/appimagebuilder/modules/generate/recipe_sections/test_section_generator.py
@@ -25,8 +25,5 @@ def generate(self):
         section = {}
         for image in self.docker_images:
             test_case_title = image.rsplit(":", maxsplit=1)[1]
-            section[test_case_title] = {
-                "image": image,
-                "command": "./AppRun"
-            }
+            section[test_case_title] = {"image": image, "command": "./AppRun"}
         return section
diff --git a/appimagebuilder/modules/test/execution_test.py b/appimagebuilder/modules/test/execution_test.py
index c2e20caf..755e19ed 100644
--- a/appimagebuilder/modules/test/execution_test.py
+++ b/appimagebuilder/modules/test/execution_test.py
@@ -21,7 +21,9 @@
 
 
 class ExecutionTest:
-    def __init__(self, appdir: Path, name, image, command, env: [str] = None):
+    def __init__(
+        self, appdir: Path, name, image, command, before_command=None, env: [str] = None
+    ):
         if env is None:
             env = []
 
@@ -29,6 +31,7 @@ def __init__(self, appdir: Path, name, image, command, env: [str] = None):
         self.name = name
         self.image = image
         self.command = command
+        self.before_command = before_command
         self.env = env
 
         self.client = docker.from_env()
@@ -62,12 +65,16 @@ def run(self):
                 container,
                 accepted_exit_codes=[0, 9],
             )
+
             self._run_command(
                 "mkdir -p /home/%s/.config" % os.getenv("USER"),
                 container,
                 user=os.getenv("USER"),
             )
 
+            if self.before_command:
+                self._run_command(self.before_command, container)
+
             self.logger.info("command")
             self._run_command(self.command, container, user=os.getenv("USER"))
         finally:
@@ -93,7 +100,7 @@ def _print_container_logs(self, ctr):
     def _get_container_volumes(self):
         volumes = {
             self.appdir: {"bind": "/app", "mode": "ro"},
-            "/tmp/.X11-unix": {"bind": "/tmp/.X11-unix", "mode": "rw"}
+            "/tmp/.X11-unix": {"bind": "/tmp/.X11-unix", "mode": "rw"},
         }
 
         dbus_session_address = os.getenv("DBUS_SESSION_BUS_ADDRESS")
diff --git a/tests/commands/run_script.py b/tests/commands/run_script.py
index 541dd87f..01505786 100644
--- a/tests/commands/run_script.py
+++ b/tests/commands/run_script.py
@@ -44,9 +44,7 @@ def test_use_pass_env(self):
                 "fi",
             ]
         )
-        command = RunScriptCommand(
-            self.fake_context, script, env={"var": "value"}
-        )
+        command = RunScriptCommand(self.fake_context, script, env={"var": "value"})
         command()
 
     def test_builder_env_set(self):
diff --git a/tests/modules/deploy/file/dependencies_resolver/TestElfResolver.py b/tests/modules/deploy/file/dependencies_resolver/TestElfResolver.py
index 2128df62..448ae053 100644
--- a/tests/modules/deploy/file/dependencies_resolver/TestElfResolver.py
+++ b/tests/modules/deploy/file/dependencies_resolver/TestElfResolver.py
@@ -13,11 +13,12 @@
 import unittest
 from shutil import which
 
-from appimagebuilder.modules.deploy.files.dependencies_resolver.elf_resolver import ElfResolver
+from appimagebuilder.modules.deploy.files.dependencies_resolver.elf_resolver import (
+    ElfResolver,
+)
 
 
 class ElfResolverTestCase(unittest.TestCase):
-
     def setUp(self) -> None:
         self.bash_path = pathlib.Path(which("bash"))
         self.resolver = ElfResolver()
@@ -28,5 +29,5 @@ def test_resolve_needed_recursively(self):
         self.assertIn("/lib/x86_64-linux-gnu/libc.so.6", results)
 
 
-if __name__ == '__main__':
+if __name__ == "__main__":
     unittest.main()