From 5bba0208022edc9e89394efe1572174be5657c8b Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Wed, 25 Oct 2023 09:00:19 +0800 Subject: [PATCH 1/5] Add test for file permission preservation. --- ...pPackagesMergeMixin__merge_app_packages.py | 7 +++++++ tests/utils.py | 19 +++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py b/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py index ddc73441e..40a49cd85 100644 --- a/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py +++ b/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py @@ -1,3 +1,4 @@ +import os import subprocess from pathlib import Path @@ -26,11 +27,13 @@ def test_merge(dummy_command, pre_existing, tmp_path): extra_content=[ ("second/other.py", "# other python"), ("second/different.py", "# different python"), + ("second/some-binary", "# A file with executable permissions", 0o755), ("second/sub1/module1.dylib", "dylib-gothic"), ("second/sub1/module2.so", "dylib-gothic"), ("second/sub1/module3.dylib", "dylib-gothic"), ], ) + # Create 2 packages in the "modern" architecture app package sources # The first package is pure, so it won't exist in the second app_packages. # The "second" package: @@ -110,6 +113,7 @@ def lipo(cmd, **kwargs): (Path("second/__init__.py"), ""), (Path("second/app.py"), "# This is the app"), (Path("second/different.py"), "# different python"), + (Path("second/some-binary"), "# A file with executable permissions"), (Path("second/other.py"), "# other python"), (Path("second/sub1"), None), (Path("second/sub1/module1.dylib"), "dylib-merged"), @@ -149,6 +153,9 @@ def lipo(cmd, **kwargs): ), } + # Check that the embedded binary has executable permissions + assert os.access(merged_path / "second" / "some-binary", 0o755) + def test_merge_problem(dummy_command, tmp_path): "If a binary cannot be merged, an exception is raised." diff --git a/tests/utils.py b/tests/utils.py index b9cb7e591..e39dde534 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -235,16 +235,22 @@ def create_installed_package( :param package: The name of the package in the wheel. Defaults to ``dummy`` :param version: The version number of the package. Defaults to ``1.2.3`` :param tag: The installation tag for the package. Defaults to a pure python wheel. - :param extra_content: Optional. A list of tuples of ``(path, content)`` that will be - added to the wheel. + :param extra_content: Optional. A list of tuples of ``(path, content)`` or + ``(path, content, chmod)`` that will be added to the wheel. If ``chmod`` is + not specified, default filesystem permissions will be used. """ - for filename, content in installed_package_content( + for entry in installed_package_content( package=package, version=version, tag=tag, extra_content=extra_content, ): - create_file(path / filename, content=content) + try: + filename, content, chmod = entry + except ValueError: + filename, content = entry + chmod = None + create_file(path / filename, content=content, chmod=chmod) def create_wheel( @@ -260,8 +266,9 @@ def create_wheel( :param package: The name of the package in the wheel. Defaults to ``dummy`` :param version: The version number of the package. Defaults to ``1.2.3`` :param tag: The installation tag for the package. Defaults to a pure python wheel. - :param extra_content: Optional. A list of tuples of ``(path, content)`` that - will be added to the wheel. + :param extra_content: Optional. A list of tuples of ``(path, content)`` or + ``(path, content, chmod)`` that will be added to the wheel. If ``chmod`` is + not specified, default filesystem permissions will be used. """ wheel_filename = path / f"{package}-{version}-{tag}.whl" From 94eed198a374dd3e3613f4a49d37111de7bbe73c Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Wed, 25 Oct 2023 09:00:31 +0800 Subject: [PATCH 2/5] Copy file permissions when merging. --- src/briefcase/platforms/macOS/utils.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/briefcase/platforms/macOS/utils.py b/src/briefcase/platforms/macOS/utils.py index 840de9fe1..6d5c8642f 100644 --- a/src/briefcase/platforms/macOS/utils.py +++ b/src/briefcase/platforms/macOS/utils.py @@ -246,6 +246,8 @@ def merge_app_packages( # The file doesn't exist yet; copy it as is, and store the # digest for later comparison self.tools.shutil.copyfile(source_path, target_path) + # Ensure permissions as well. + self.tools.shutil.copymode(source_path, target_path) digests[relative_path] = sha256_file_digest(source_path) # Call lipo on each dylib that was found to create the fat version. From 2eaf1defc379619eae06e260e90ff6b6ab647fe4 Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Wed, 25 Oct 2023 09:00:51 +0800 Subject: [PATCH 3/5] Add changenote. --- changes/1510.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/1510.bugfix.rst diff --git a/changes/1510.bugfix.rst b/changes/1510.bugfix.rst new file mode 100644 index 000000000..b015fd551 --- /dev/null +++ b/changes/1510.bugfix.rst @@ -0,0 +1 @@ +When merging dependencies on macOS, file permissions are now preserved. From 9f02eac92da7a687cb93b96ec28585c66fda7205 Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Wed, 25 Oct 2023 09:21:42 +0800 Subject: [PATCH 4/5] Correct mask used to check file access. --- .../macOS/test_AppPackagesMergeMixin__merge_app_packages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py b/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py index 40a49cd85..6e537ec67 100644 --- a/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py +++ b/tests/platforms/macOS/test_AppPackagesMergeMixin__merge_app_packages.py @@ -154,7 +154,7 @@ def lipo(cmd, **kwargs): } # Check that the embedded binary has executable permissions - assert os.access(merged_path / "second" / "some-binary", 0o755) + assert os.access(merged_path / "second" / "some-binary", os.X_OK) def test_merge_problem(dummy_command, tmp_path): From f0a77e2677c235b3608c7b1dd85ad38bbbe392ae Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Thu, 26 Oct 2023 06:53:48 +0800 Subject: [PATCH 5/5] Use copy rather than copyfile to preserve permissions. --- src/briefcase/platforms/macOS/utils.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/briefcase/platforms/macOS/utils.py b/src/briefcase/platforms/macOS/utils.py index 6d5c8642f..e0be7226c 100644 --- a/src/briefcase/platforms/macOS/utils.py +++ b/src/briefcase/platforms/macOS/utils.py @@ -243,11 +243,9 @@ def merge_app_packages( f"between sources; ignoring {source_app_packages.suffix[1:]} version." ) else: - # The file doesn't exist yet; copy it as is, and store the - # digest for later comparison - self.tools.shutil.copyfile(source_path, target_path) - # Ensure permissions as well. - self.tools.shutil.copymode(source_path, target_path) + # The file doesn't exist yet; copy it as is (including + # permissions), and store the digest for later comparison + self.tools.shutil.copy(source_path, target_path) digests[relative_path] = sha256_file_digest(source_path) # Call lipo on each dylib that was found to create the fat version.