Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Files using tenacity for retrying renaming a directory on a opened filepath. #1826

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ venv*/
.eggs/
.tox/
/local
__pycache__/

/pip-wheel-metadata
# IntelliJ Idea family of suites
Expand Down
1 change: 1 addition & 0 deletions changes/1780.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added Files integration using tenacity for path dir rename retry
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,9 @@ dependencies = [
"python-dateutil >= 2.9.0.post0", # transitive dependency (beeware/briefcase#1428)
"requests >= 2.28, < 3.0",
"rich >= 12.6, < 14.0",
"tenacity >= 8.0, < 9.0",
"tomli >= 2.0, < 3.0; python_version <= '3.10'",
"tomli_w >= 1.0, < 2.0",
"tomli_w >= 1.0, < 2.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma on the end is intentional.

Suggested change
"tomli_w >= 1.0, < 2.0"
"tomli_w >= 1.0, < 2.0",

]

[project.optional-dependencies]
Expand Down
2 changes: 2 additions & 0 deletions src/briefcase/commands/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
)
from briefcase.integrations.base import ToolCache
from briefcase.integrations.download import Download
from briefcase.integrations.files import Files
from briefcase.integrations.subprocess import Subprocess
from briefcase.platforms import get_output_formats, get_platforms

Expand Down Expand Up @@ -188,6 +189,7 @@ def __init__(
# Immediately add tools that must be always available
Subprocess.verify(tools=self.tools)
Download.verify(tools=self.tools)
Files.verify(tools=self.tools)

if not is_clone:
self.validate_locale()
Expand Down
2 changes: 2 additions & 0 deletions src/briefcase/integrations/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
cookiecutter,
docker,
download,
files,
flatpak,
git,
java,
Expand All @@ -20,6 +21,7 @@
"cookiecutter",
"docker",
"download",
"files",
"flatpak",
"git",
"java",
Expand Down
5 changes: 3 additions & 2 deletions src/briefcase/integrations/android_sdk.py
Original file line number Diff line number Diff line change
Expand Up @@ -426,8 +426,9 @@ def install(self):
self.tools.shutil.rmtree(self.cmdline_tools_path)

# Rename the top level zip content to the final name
(self.cmdline_tools_path.parent / "cmdline-tools").rename(
self.cmdline_tools_path
self.tools.files.path_rename(
old_path=(self.cmdline_tools_path.parent / "cmdline-tools"),
new_path=self.cmdline_tools_path,
)

# Zip file no longer needed once unpacked.
Expand Down
2 changes: 2 additions & 0 deletions src/briefcase/integrations/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from briefcase.integrations.android_sdk import AndroidSDK
from briefcase.integrations.docker import Docker, DockerAppContext
from briefcase.integrations.download import Download
from briefcase.integrations.files import Files
from briefcase.integrations.flatpak import Flatpak
from briefcase.integrations.java import JDK
from briefcase.integrations.linuxdeploy import LinuxDeploy
Expand Down Expand Up @@ -149,6 +150,7 @@ class ToolCache(Mapping):
app_context: Subprocess | DockerAppContext
docker: Docker
download: Download
files: Files
flatpak: Flatpak
git: git_
java: JDK
Expand Down
30 changes: 30 additions & 0 deletions src/briefcase/integrations/files.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from __future__ import annotations

from pathlib import Path

from tenacity import retry, stop_after_attempt, wait_fixed

from briefcase.integrations.base import Tool, ToolCache


class Files(Tool):
name = "files"
full_name = "Files"

@classmethod
def verify_install(cls, tools: ToolCache, **kwargs) -> Files:
"""Make files available in tool cache."""
# short circuit since already verified and available
if hasattr(tools, "files"):
return tools.files

tools.files = Files(tools=tools)
return tools.files

@retry(wait=wait_fixed(0.2), stop=stop_after_attempt(25))
def path_rename(self, old_path: Path, new_path: object):
"""Using tenacity for a retry policy on pathlib rename.

Windows does not like renaming a dir in a path with an opened file.
"""
old_path.rename(new_path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a "retry on any error" handler - what happens for predictable failure - e.g., attempting to rename a file that doesn't exist? There are some failures that should be surfaced immediately.

5 changes: 4 additions & 1 deletion src/briefcase/integrations/java.py
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,10 @@ def install(self):
java_unpack_path = (
self.tools.base_path / f"jdk-{self.JDK_RELEASE}+{self.JDK_BUILD}"
)
java_unpack_path.rename(self.tools.base_path / self.JDK_INSTALL_DIR_NAME)
self.tools.files.path_rename(
old_path=java_unpack_path,
new_path=self.tools.base_path / self.JDK_INSTALL_DIR_NAME,
)

def uninstall(self):
"""Uninstall a JDK."""
Expand Down
2 changes: 2 additions & 0 deletions tests/integrations/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from briefcase.console import Log
from briefcase.integrations.base import ToolCache
from briefcase.integrations.download import Download
from briefcase.integrations.files import Files
from briefcase.integrations.subprocess import Subprocess
from tests.utils import DummyConsole

Expand Down Expand Up @@ -38,6 +39,7 @@ def mock_tools(tmp_path) -> ToolCache:

# Make Download and Subprocess always available
Download.verify(tools=mock_tools)
Files.verify(tools=mock_tools)
Subprocess.verify(tools=mock_tools)

return mock_tools
Expand Down
Empty file.
45 changes: 45 additions & 0 deletions tests/integrations/files/test_Files__rename.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import os
import sys
import threading
import time

import pytest
import tenacity


def test_rename_path(mock_tools, tmp_path):
def openclose(filepath):
handler = filepath.open(encoding="UTF-8")
time.sleep(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sleep needs to be shorter - 1 second is a long time in a test

handler.close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The close should be in a try:finally to ensure that if other parts of the process fail, the file will still be closed.


(tmp_path / "orig-dir-1").mkdir()
tl = tmp_path / "orig-dir-1/orig-file"
tl.touch()
file_access_thread = threading.Thread(target=openclose, args=(tl,))
rename_thread = threading.Thread(
target=mock_tools.files.path_rename,
args=(tmp_path / "orig-dir-1", tmp_path / "new-dir-1"),
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the rename thread needed? It should be possible to do this inline.


file_access_thread.start()
rename_thread.start()

rename_thread.join()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should join on the file_access_thread to ensure that it cleans up.


assert "new-dir-1" in os.listdir(tmp_path)


@pytest.mark.xfail(
sys.platform == "win32",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about this being a win32 only error. macOS and Linux won't fail because of a file rename, but they could fail for other reasons - we need to verify the "will eventually fail" path on all platforms.

raises=tenacity.RetryError,
reason="Windows can't rename folder in filepath when the file is open",
)
def test_rename_path_fail(mock_tools, tmp_path, monkeypatch):
(tmp_path / "orig-dir-2").mkdir()
(tmp_path / "orig-dir-2/orig-file").touch()

with (tmp_path / "orig-dir-2/orig-file").open(encoding="UTF-8"):

monkeypatch.setattr(tenacity.nap.time, "sleep", lambda x: True)
mock_tools.files.path_rename(tmp_path / "orig-dir-2", tmp_path / "new-dir-2")
11 changes: 11 additions & 0 deletions tests/integrations/files/test_Files__verify.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from briefcase.integrations.files import Files


def test_short_circuit(mock_tools):
"""Tool is not created if already cached."""
mock_tools.files = "tool"

tool = Files.verify(mock_tools)

assert tool == "tool"
assert tool == mock_tools.files