Repository: pyinstaller/pyinstaller
Pull request: Allow users to re-enable the old onedir layout
Allow users to re-enable the old onedir layout (without contents directory) by settings the `--contents-directory` option (or the equivalent `contents_directory` argument to `EXE` in the .spec file) to `.`.

Problem statement:
Make it possible to put files outside of contents_diretory
I've been bundling my software with pyInstaller for ages. But version 6.0.0 breaks my software in an unexpected way. 
When I test the script from sources, it seeks for some files in the same folder the script itself resides - e. g. pictures in "assets" folder.
For new pyInstaller to work, I'll have to put all assets into _internal folder, but I don't want to change my code to comply with this change.
Can I just have an option to have "datas" variable in a ".spec" file accept a path outside of contents_directory, like this?
`datas = (('assets', '../assets'),)`

Thank you.

Hints from issue discussion:
PyInstaller collects files (e.g. the ones given via `datas`) into *top-level application directory*. In onedir builds, this used to be the directory with executable, and as of v6, this has been changed to the sub-directory (called `_internal` by default). In onefile builds, this is (and always was) the temporary directory where contents are extracted. The top-level application directory is also pointed to by `sys._MEIPASS` and modules' `__file__` attribute. 

> For new pyInstaller to work, I'll have to put all assets into _internal folder, but I don't want to change my code to comply with this change.

So if your resources are collected via `datas`, they are now automatically collected into sub-directory. And if your program is referring to them via `sys._MEIPASS` or modules' `__file__` attribute, it should make no difference. The cases when it now breaks is if you anchored your paths to `sys.executable` instead, or if you refer to your resources using relative paths, which makes them dependent on your current working directory (and also breaks in other scenarios, such as executable being called from an arbitrary location on the filesystem).

> Can I just have an option to have "datas" variable in a ".spec" file accept a path outside of contents_directory, like this?
`datas = (('assets', '../assets'),)`

No, because collecting outside of the top-level application directory makes no sense in any other PyInstaller build mode (onefile, macOS .app bundles).

If you really want files next to the executable (i.e., outside of the new top-level application directory of onedir mode) for whatever reason, you can copy them there yourself (either programmatically at the end of the spec, or as a post-processing step after calling pyinstaller).

If moving your data files breaks your code, then your code is referencing them wrong. [Our docs](https://pyinstaller.org/en/stable/runtime-information.html#using-file) are very explicit on how data files should be referenced.
Hello, using `sys.MEIPASS` and `__file__` to access data files in relative directories is indeed a reliable solution. 

However, for older projects that haven't utilized these techniques, engineers would have to modify the code and explain the reasons for the changes to others. 

PyInstaller is a very convenient and compatible tool. It would be great if the new version could consider compatibility with old code while cleaning up the cluttered working directory. This way, engineers wouldn't have to modify already audited old code. 

Thank you.



> PyInstaller collects files (e.g. the ones given via `datas`) into _top-level application directory_. In onedir builds, this used to be the directory with executable, and as of v6, this has been changed to the sub-directory (called `_internal` by default). In onefile builds, this is (and always was) the temporary directory where contents are extracted. The top-level application directory is also pointed to by `sys._MEIPASS` and modules' `__file__` attribute.
> 
> > For new pyInstaller to work, I'll have to put all assets into _internal folder, but I don't want to change my code to comply with this change.
> 
> So if your resources are collected via `datas`, they are now automatically collected into sub-directory. And if your program is referring to them via `sys._MEIPASS` or modules' `__file__` attribute, it should make no difference. The cases when it now breaks is if you anchored your paths to `sys.executable` instead, or if you refer to your resources using relative paths, which makes them dependent on your current working directory (and also breaks in other scenarios, such as executable being called from an arbitrary location on the filesystem).
> 
> > Can I just have an option to have "datas" variable in a ".spec" file accept a path outside of contents_directory, like this?
> > `datas = (('assets', '../assets'),)`
> 
> No, because collecting outside of the top-level application directory makes no sense in any other PyInstaller build mode (onefile, macOS .app bundles).
> 
> If you really want files next to the executable (i.e., outside of the new top-level application directory of onedir mode) for whatever reason, you can copy them there yourself (either programmatically at the end of the spec, or as a post-processing step after calling pyinstaller).

I can second that.
In our case there are other parts of the process that rely on the file structure as it was, like the installer (icons, scripts, manuals). Those are now in different places and are not affected by the Python variables.
I can change that of course, but the impact is quite big. I don't fully understand why for example setting data directory to be the exe directory is explicitly blocked.
> Hello, using `sys.MEIPASS` and `__file__` to access data files in relative directories is indeed a reliable solution.
> 
> However, for older projects that haven't utilized these techniques, engineers would have to modify the code and explain the reasons for the changes to others.

From my perspective, having your application reference resources using relative paths is a bug in your code, as it makes your program depend on the current working directory - and that applies to your unfrozen code as well, not just to PyInstaller-frozen application.

> In our case there are other parts of the process that rely on the file structure as it was, like the installer (icons, scripts, manuals). Those are now in different places and are not affected by the Python variables.

Then these files are not really a part of PyInstaller-frozen application bundle, but rather part of some higher-level application bundle. And as such, they should be managed there. (And while this distinction is new for onedir builds, it was always there for onefile ones).

> I can change that of course, but the impact is quite big. I don't fully understand why for example setting data directory to be the exe directory is explicitly blocked.

If you mean why you cannot get the old structure where the top-level application directory (`sys._MEIPASS`) is the executable's parent directory, then that is a valid complaint. (But if you are referring to the lack of ability to set the target for `datas` outside of the `sys._MEIPASS`, then that is blocked due to security implications).

I am considering allowing `contents_directory` to be set to an empty string (and/or `None`), which would result in the old  "cluttered" onedir structure from v5 and earlier, so that users can decide which structure to go with.

Initially, we did not go for that because testing is concern, but I think if `sys._MEIPASS` directory is fully relocatable (i.e., it does not matter what the name of contents directory is), there should be no issues with it being the same as the executable's parent directory, either (except for the possible clash between a collected file and executable name on non-Windows).
@rokm Thanks for your reply . 

I don't intend to argue whether putting data files in the working directory is a bug in the program (in fact, it can be considered a portable solution, regardless of whether it aligns with our aesthetic taste).

My point is, as PyInstaller is a very successful open-source tool, could it consider being compatible with old code (regardless of whether it is perfect)?

Here's my personal feeling: Handling sys._MEIPASS in business code itself increases the coupling of the program. Business engineers may question why they have to deal with the complexity of checking a PyInstaller internal parameter when retrieving files.

maybe  `--add-data=/path/to/file:@/file`  ? `@` means executable's directory .
I actually would propose to allow data_directory to ".". (Edit: indeed not"..")
> I am considering allowing contents_directory to be set to an empty string (and/or None), which would result in the old "cluttered" onedir structure from v5 and earlier, so that users can decide which structure to go with.

I'm still opposed to this. The number of conditionals required to keep the `MERGE()` related code in the bootloader will be horrific. Maybe if `MERGE()` was dropped and macOS app bundles were redesigned to be assembled directly rather than through an intermediate one dir bundle then it would be more achievable but, messy though `MERGE()` is, I think it's still more valuable than the ability to skip that `_internal` directory.

> Here's my personal feeling: Handling sys._MEIPASS in business code itself increases the coupling of the program. Business engineers may question why they have to deal with the complexity of checking a PyInstaller internal parameter when retrieving files.

If you use `__file__` instead of `sys._MEIPASS` and choose the right `dest` value for your `--add-data` flags correctly, then there should be no PyInstaller specific code required. (And I know our docs do seem to promote using `sys._MEIPASS` – they shouldn't do.)

> My point is, as PyInstaller is a very successful open-source tool, could it consider being compatible with old code (regardless of whether it is perfect)?

It may be a popular tool but that doesn't change the fact that it only has 3 maintainers (where only one of them actually has any time to do more than issue triage). We have to pick our battles. Compatibility code is expensive and if it's main purpose it supporting broken code then that is definitely not a good choice in my book.
> > I am considering allowing contents_directory to be set to an empty string (and/or None), which would result in the old "cluttered" onedir structure from v5 and earlier, so that users can decide which structure to go with.
> 
> I'm still opposed to this. The number of conditionals required to keep the `MERGE()` related code in the bootloader will be horrific. Maybe if `MERGE()` was dropped and macOS app bundles were redesigned to be assembled directly rather than through an intermediate one dir bundle then it would be more achievable but, messy though `MERGE()` is, I think it's still more valuable than the ability to skip that `_internal` directory.

As far as `MERGE` is concerned, it is two path checks that would need to be expanded to handle case where`contents_directory` is set and case where it is not:
https://github.com/pyinstaller/pyinstaller/blob/e70c723d8166c0c28f19bc3f0f3da3c814cc061c/bootloader/src/pyi_launch.c#L214
and
https://github.com/pyinstaller/pyinstaller/blob/e70c723d8166c0c28f19bc3f0f3da3c814cc061c/bootloader/src/pyi_launch.c#L223

(That code needs a closer scrutiny anyway, because both conditions look the same to me...)

With a bit of refactoring, it wouldn't even look that bad (e.g., if the if/else if/else block was split into separate if blocks, with additional if blocks before for path formatting cases). Not exactly pretty, but given the overall MERGE code, not that (much more) awful, either.

EDIT: there's actually one more use of `contents_directory` below, but that shouldn't change things much.

(And while we could go with `.` like Safihre suggested, I would prefer clean paths, just to be sure we won't be running into issues with naive string comparison and path normalization in some dark corner of PyInstaller. I think '.' is actually a good choice for the command-line, as long as we map it onto `contents_directory` bootloader-option not being set later on).

---

macOS app bundles are not affected by `contents_directory` in any way. `BUNDLE` only takes the `toc` and couple of settings from `COLLECT`, so I don't think it would require a `COLLECT` to be actually built. So we could (and probably should) introduce a separate .spec template for `--windowed` on macOS where there is no intermediate `COLLECT`. The only real benefit of the current `COLLECT` + `BUNDLE` combo is that our CI tests both regular POSIX build and the .app bundle wherever we use `--windowed` argument.

Ah, did you mention macOS .app bundles because of #7314 (which can now be triggered only by intermediate `COLLECT`)? 

I don't think that should be a problem. In existing codebases, users had to take care of this anyway (by renaming the offending package, or by renaming the executable). If we allow `contents_directory` to be disabled to obtain old onedir layout, then this issue could be reintroduced only if the user explicitly went for that option - but there is actually no reason to do so if the end-goal is an .app bundle.

Commit message: bootloader: allow pyi-contents-directory option to be unset

Allow pyi-contents-directory option to be unset, which re-enables
the old layout for onedir builds, where files are placed next to
the executable (instead of placing them into sub-directory).

Patch to review:
diff --git a/bootloader/src/pyi_archive.c b/bootloader/src/pyi_archive.c
index 18a4be4b893..9f2b6f0f085 100644
--- a/bootloader/src/pyi_archive.c
+++ b/bootloader/src/pyi_archive.c
@@ -521,13 +521,18 @@ pyi_arch_setup(ARCHIVE_STATUS *status, char const *archive_path, char const *exe
         pyi_path_join(status->homepath, contents_dir, "Frameworks");
     } else {
         const char *contents_directory = pyi_arch_get_option(status, "pyi-contents-directory");
-        if (!contents_directory) {
-            FATALERROR("pyi-contents-directory option not found in onedir bundle archive!");
-            return false;
+        if (contents_directory) {
+            /* NOTE: this also applies contents-directory to status->homepath
+             * in onefile mode, which is, strictly speaking, incorrect. But
+             * it does not seem to cause any issues, because corresponding
+             * codepaths use status->temppath
+             */
+            char root_path[PATH_MAX];
+            pyi_path_dirname(root_path, archive_path);
+            pyi_path_join(status->homepath, root_path, contents_directory);
+        } else {
+            pyi_path_dirname(status->homepath, archive_path);
         }
-        char root_path[PATH_MAX];
-        pyi_path_dirname(root_path, archive_path);
-        pyi_path_join(status->homepath, root_path, contents_directory);
     }
 
     /*
diff --git a/bootloader/src/pyi_launch.c b/bootloader/src/pyi_launch.c
index bfc4a5138f1..3a807388c5d 100644
--- a/bootloader/src/pyi_launch.c
+++ b/bootloader/src/pyi_launch.c
@@ -65,18 +65,19 @@ _format_and_check_path(char *buf, const char *fmt, ...)
     };
     va_end(args);
 
-    VS("Checking for file %s\n", buf);
     return stat(buf, &tmp);
 }
 
-/* Splits the item in the form path:filename */
+/* Splits the item in the form path:filename. The first part is the path
+ * to the other executable (which contains the dependency); the path is
+ * relative to the current executable. The second part is the filename of
+ * dependency, relative to the top-level application directory. */
 /* NOTE: must be visible outside of this unit (i.e., non-static) due to tests! */
 int
 _split_dependency_name(char *path, char *filename, const char *item)
 {
     char *p;
 
-    VS("LOADER: Splitting dependency name into path and filename\n");
     // copy directly into destination buffer and manipulate there
     if (snprintf(path, PATH_MAX, "%s", item) >= PATH_MAX) {
         return -1;
@@ -180,72 +181,100 @@ _extract_dependency_from_archive(ARCHIVE_STATUS *status, const char *filename)
 static int
 _extract_dependency(ARCHIVE_STATUS *archive_pool[], const char *item)
 {
-    ARCHIVE_STATUS *status = NULL;
-    ARCHIVE_STATUS *archive_status = archive_pool[0];
-    char path[PATH_MAX];
+    const ARCHIVE_STATUS *this_archive_status = archive_pool[0];
+    char other_executable[PATH_MAX];
+    char other_executable_dir[PATH_MAX];
     char filename[PATH_MAX];
-    char srcpath[PATH_MAX];
-    char archive_path[PATH_MAX];
-
-    char dirname[PATH_MAX];
-    char homepath_parent[PATH_MAX];
-
-    VS("LOADER: Extracting dependency/reference %s\n", item);
-
-    if (_split_dependency_name(path, filename, item) == -1) {
-        return -1;
-    }
-
-    pyi_path_dirname(dirname, path);
-    pyi_path_dirname(homepath_parent, archive_status->homepath);
-    const char *contents_directory = pyi_arch_get_option(archive_pool[0], "pyi-contents-directory");
-    if (!contents_directory) {
-        FATALERROR("pyi-contents-directory option not found in onedir bundle archive!");
+    char this_executable_dir[PATH_MAX];
+    char full_srcpath[PATH_MAX];
+
+    const char *contents_directory;
+    int ret;
+
+    VS("LOADER: Processing dependency reference: %s\n", item);
+
+    /* Dependency reference consists of two parts, separated by a colon, for example
+     *   (../)other_program:path/to/file
+     * if other_program is a onefile executable, or
+     *   (../)other_program/other_program:path/to/file
+     * if other_program is a onefile executable.
+     *
+     * The first part is path to the other executable (which contains the dependency),
+     * relative to the current executable. On Windows, the executable name does NOT
+     * contain .exe suffix. The second part is the filename of dependency, relative to
+     * the top-level application directory.
+     */
+    if (_split_dependency_name(other_executable, filename, item) == -1) {
         return -1;
     }
 
-    /* We need to identify and handle three situations:
-     *  1) dependencies are in a onedir archive next to the current onefile archive,
-     *  2) dependencies are in a onedir/onefile archive next to the current onedir archive,
-     *  3) dependencies are in a onefile archive next to the current onefile archive.
+    /* Determine parent directories of this executable (absolute path) and the other
+     * executable (relative to this executable). If executables are co-located
+     * (e.g., two onefile builds), the other executable's parent directory will be ".".
      */
-    VS("LOADER: homepath is %s\n", archive_status->homepath);
-    /* TODO implement pyi_path_join to accept variable length of arguments for this case. */
-    if (_format_and_check_path(srcpath, "%s%c%s%c%s%c%s", homepath_parent, PYI_SEP, dirname, PYI_SEP, contents_directory, PYI_SEP, filename) == 0) {
-        VS("LOADER: File %s found, assuming onedir reference\n", srcpath);
+    pyi_path_dirname(this_executable_dir, this_archive_status->executablename);
+    pyi_path_dirname(other_executable_dir, other_executable);
 
-        if (_copy_dependency_from_dir(archive_status, srcpath, filename) == -1) {
-            FATALERROR("Failed to copy %s\n", filename);
-            return -1;
-        }
-        /* TODO implement pyi_path_join to accept variable length of arguments for this case. */
+    /* Retrieve contents-directory setting, from THIS executable, assuming it is
+     * the same across all multi-package executables. In practice, this should matter
+     * only if multi-package involves onedir builds.
+     */
+    contents_directory = pyi_arch_get_option(this_archive_status, "pyi-contents-directory");
+
+    /* If dependency is located in a onedir build, we should be able to find
+     * it on the filesystem (accounting for contents sub-directory settings).
+     * If dependency is located in a onefile build, we need to look up the
+     * executable (or external PKG archive in case of side-loading). As the
+     * executable (with embedded or external PKG archive) is also available
+     * in onedir builds, we need to first check for the onedir option.
+     *
+     * Note that the path relations between different programs in a multipackage
+     * should already be handled by the path encoded in the reference, and thus
+     * reflected in the `other_executable_dir`:
+     *  - for a onefile program referencing a dependency in a onefile program,
+     *    it is "."
+     *  - for a onedir program referencing a dependency in a onefile program,
+     *    it is ".."
+     *  - for a onefile program referencing a dependency in a onedir program,
+     *    it is "other_program"
+     *  - for a onedir program referencing a dependency in a onedir program,
+     *    it is "../other_program"
+     */
+    if (contents_directory) {
+        ret = _format_and_check_path(full_srcpath, "%s%c%s%c%s%c%s", this_executable_dir, PYI_SEP, other_executable_dir, PYI_SEP, contents_directory, PYI_SEP, filename);
+    } else {
+        ret = _format_and_check_path(full_srcpath, "%s%c%s%c%s", this_executable_dir, PYI_SEP, other_executable_dir, PYI_SEP, filename);
     }
-    else if (_format_and_check_path(srcpath, "%s%c%s%c%s%c%s", homepath_parent, PYI_SEP, dirname, PYI_SEP, contents_directory, PYI_SEP, filename) == 0) {
-        VS("LOADER: File %s found, assuming onedir reference\n", srcpath);
+    if (ret == 0) {
+        VS("LOADER: File %s found on filesystem (%s), assuming onedir reference.\n", filename, full_srcpath);
 
-        if (_copy_dependency_from_dir(archive_status, srcpath, filename) == -1) {
-            FATALERROR("Failed to copy %s\n", filename);
+        if (_copy_dependency_from_dir(this_archive_status, full_srcpath, filename) == -1) {
+            FATALERROR("Failed to copy file %s from %s!\n", filename, full_srcpath);
             return -1;
         }
-    }
-    else {
-        VS("LOADER: File %s not found, assuming onefile reference.\n", srcpath);
-
-        /* TODO implement pyi_path_join to accept variable length of arguments for this case. */
-        if ((_format_and_check_path(archive_path, "%s%c%s%c%s.pkg", homepath_parent, PYI_SEP, contents_directory, PYI_SEP, path) != 0) &&
-            (_format_and_check_path(archive_path, "%s%c%s.exe", homepath_parent, PYI_SEP, path) != 0) &&
-            (_format_and_check_path(archive_path, "%s%c%s", homepath_parent, PYI_SEP, path) != 0)) {
-            FATALERROR("Archive not found: %s\n", archive_path);
+    } else {
+        ARCHIVE_STATUS *other_archive_status = NULL;
+        char other_archive_path[PATH_MAX];
+
+        VS("LOADER: File %s not found on filesystem, assuming onefile reference.\n", filename);
+
+        /* First check for the presence of external .pkg archive, located
+         * next to the executable, to account for side-loading mode.
+         */
+        if ((_format_and_check_path(other_archive_path, "%s%c%s.pkg", this_executable_dir, PYI_SEP, other_executable) != 0) &&
+            (_format_and_check_path(other_archive_path, "%s%c%s.exe", this_executable_dir, PYI_SEP, other_executable) != 0) &&
+            (_format_and_check_path(other_archive_path, "%s%c%s", this_executable_dir, PYI_SEP, other_executable) != 0)) {
+            FATALERROR("Referenced dependency archive %s not found.\n", other_executable);
             return -1;
         }
 
-        if ((status = _get_archive(archive_pool, archive_path)) == NULL) {
-            FATALERROR("Archive not found: %s\n", archive_path);
+        if ((other_archive_status = _get_archive(archive_pool, other_archive_path)) == NULL) {
+            FATALERROR("Failed to open referenced dependency archive %s.\n", other_archive_path);
             return -1;
         }
 
-        if (_extract_dependency_from_archive(status, filename) == -1) {
-            FATALERROR("Failed to extract %s\n", filename);
+        if (_extract_dependency_from_archive(other_archive_status, filename) == -1) {
+            FATALERROR("Failed to extract %s from referenced dependency archive %s.\n", filename, other_archive_path);
             /* Do not free the archive ("status") here, because its
              * pointer is stored in the archive pool that is cleaned up
              * by the caller.


Code review comments:
diff_hunk: @@ -468,8 +469,9 @@ def __init__(self, *args, **kwargs):
             logger.warning('Ignoring hide_console; supported only on Windows!')
             self.hide_console = None
 
-        if self.contents_directory in ("", ".", "..") \
-                or "/" in self.contents_directory or "\\" in self.contents_directory:
+        if self.contents_directory in ("", "."):
+            self.contents_directory = None  # Re-enable old onedir layout without contents directory.
+        elif self.contents_directory == ".." or "/" in self.contents_directory or "\\" in self.contents_directory:
             raise SystemExit(
                 f'Invalid value "{self.contents_directory}" passed to `--contents-directory` or `contents_directory`. '
                 'Exactly one directory level is required.'
path: PyInstaller/building/api.py
text: @user1:
Guess we'll need to mention `.` in the error message too

@author:
You mean `"Exactly one directory level (or . to disable directory) is required."`?

@user1:
Yeah