-
Notifications
You must be signed in to change notification settings - Fork 151
feature/auto updates FIRE 799 #49
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
Changes from all commits
38ca5ab
b7e1c0e
635ba61
1ddb6e7
dcbce6a
af6ecdc
452c24d
b00689f
417a437
b8845cd
e5bd2bc
743587b
e3e3595
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |||
| import platformdirs | ||||
| import requests | ||||
| from loguru import logger | ||||
| from rich.console import Console | ||||
|
|
||||
|
|
||||
| class RogueTuiInstaller: | ||||
|
|
@@ -43,15 +44,23 @@ def _os(self) -> str: | |||
|
|
||||
| def _get_latest_github_release(self) -> Optional[dict]: | ||||
| """Get the latest release information from GitHub.""" | ||||
| console = Console() | ||||
|
|
||||
| try: | ||||
| url = f"https://api.github.com/repos/{self._repo}/releases/latest" | ||||
| response = requests.get( | ||||
| url, | ||||
| timeout=10, | ||||
| headers=self._headers, | ||||
| ) | ||||
| response.raise_for_status() | ||||
| return response.json() | ||||
|
|
||||
| with console.status( | ||||
| "[bold blue]Fetching latest release information...", | ||||
| spinner="dots", | ||||
| ): | ||||
| response = requests.get( | ||||
| url, | ||||
| timeout=10, | ||||
| headers=self._headers, | ||||
| verify=False, # nosec: B501 | ||||
| ) | ||||
| response.raise_for_status() | ||||
| return response.json() | ||||
| except Exception: | ||||
yuval-qf marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| logger.exception("Error fetching latest release") | ||||
| return None | ||||
|
|
@@ -76,6 +85,8 @@ def _find_asset_for_platform( | |||
| return None | ||||
|
|
||||
| def _download_rogue_tui_to_temp(self) -> str: | ||||
| console = Console() | ||||
|
|
||||
| # Get latest release | ||||
| release_data = self._get_latest_github_release() | ||||
| if not release_data: | ||||
|
|
@@ -90,25 +101,29 @@ def _download_rogue_tui_to_temp(self) -> str: | |||
| ) | ||||
| raise Exception("No suitable binary found for current platform.") | ||||
|
|
||||
| logger.info(f"Downloading: {download_url}") | ||||
|
|
||||
| response = requests.get( | ||||
| download_url, | ||||
| timeout=60, | ||||
| headers={ | ||||
| "Accept": "application/octet-stream", | ||||
| **self._headers, | ||||
| }, | ||||
| ) | ||||
| response.raise_for_status() | ||||
| # Show spinner during download | ||||
| with console.status( | ||||
| "[bold green]Downloading rogue-tui binary...", | ||||
| spinner="dots", | ||||
| ): | ||||
| response = requests.get( | ||||
| download_url, | ||||
| timeout=60, | ||||
| headers={ | ||||
| "Accept": "application/octet-stream", | ||||
| **self._headers, | ||||
| }, | ||||
| verify=False, # nosec: B501 | ||||
drorIvry marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| ) | ||||
| response.raise_for_status() | ||||
|
|
||||
|
Comment on lines
+104
to
119
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stream download and write to same-volume temp file; avoid verify=False and large memory spike.
As per coding guidelines Apply: - with console.status(
+ with console.status(
"[bold green]Downloading rogue-tui binary...",
spinner="dots",
):
- response = requests.get(
- download_url,
- timeout=60,
- headers={
- "Accept": "application/octet-stream",
- **self._headers,
- },
- verify=False, # nosec: B501
- )
- response.raise_for_status()
-
- # Create a temporary file
- with tempfile.NamedTemporaryFile(
- delete=False,
- suffix="-rogue-tui",
- ) as tmp_file:
- tmp_file.write(response.content)
- tmp_path = tmp_file.name
+ with requests.get(
+ download_url,
+ timeout=60,
+ headers={"Accept": "application/octet-stream", **self._headers, "User-Agent": "rogue-updater"},
+ stream=True,
+ ) as response:
+ response.raise_for_status()
+ install_dir = self._get_install_path().parent
+ Path(install_dir).mkdir(parents=True, exist_ok=True)
+ with tempfile.NamedTemporaryFile(
+ delete=False,
+ dir=install_dir,
+ prefix=".rogue-tui.",
+ suffix=".tmp",
+ ) as tmp_file:
+ for chunk in response.iter_content(chunk_size=1_048_576):
+ if chunk:
+ tmp_file.write(chunk)
+ tmp_path = tmp_file.nameOptional: prefer asset["browser_download_url"] when available to avoid the API indirection requirement for Accept header. Also applies to: 125-132 🤖 Prompt for AI Agents
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @drorIvry streaming the file instead of keeping it all in memory sounds like a good idea
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||
| # Create a temporary file | ||||
| with tempfile.NamedTemporaryFile( | ||||
| delete=False, | ||||
| suffix="-rogue-tui", | ||||
| ) as tmp_file: | ||||
| tmp_file.write(response.content) | ||||
| tmp_path = tmp_file.name | ||||
| # Create a temporary file | ||||
| with tempfile.NamedTemporaryFile( | ||||
| delete=False, | ||||
| suffix="-rogue-tui", | ||||
| ) as tmp_file: | ||||
| tmp_file.write(response.content) | ||||
| tmp_path = tmp_file.name | ||||
|
|
||||
| # Make it executable | ||||
| os.chmod(tmp_path, 0o755) # nosec: B103 | ||||
|
|
@@ -134,7 +149,6 @@ def _handle_path_env(self, install_dir: Path) -> None: | |||
| sep = ";" | ||||
|
|
||||
| if str(install_dir) not in os.environ.get("PATH", "").split(sep): | ||||
| logger.info(f"Adding {install_dir} to PATH environment variable.") | ||||
| os.environ["PATH"] += sep + str(install_dir) | ||||
| # TODO update shellrc file to update the path | ||||
|
|
||||
|
|
@@ -153,33 +167,41 @@ def _is_rogue_tui_installed(self) -> bool: | |||
| else: | ||||
| return False | ||||
|
|
||||
| def install_rogue_tui(self) -> bool: | ||||
| def install_rogue_tui( | ||||
| self, | ||||
| upgrade: bool = False, | ||||
| ) -> bool: | ||||
| """Install rogue-tui from GitHub releases if not already installed.""" | ||||
| console = Console() | ||||
| # Check if rogue-tui is already available | ||||
| if self._is_rogue_tui_installed(): | ||||
| logger.info("rogue-tui is already installed.") | ||||
| if self._is_rogue_tui_installed() and not upgrade: | ||||
| console.print("[green]✅ rogue-tui is already installed.[/green]") | ||||
| return True | ||||
|
|
||||
|
Comment on lines
170
to
180
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Upgrade path can fail on Windows: use atomic replace instead of shutil.move. When upgrade=True and the destination exists, shutil.move can fail on Windows if the file exists/in use. Use Path.replace (os.replace) for atomic overwrite. Apply: - install_path = self._get_install_path()
- shutil.move(tmp_path, install_path)
+ install_path = self._get_install_path()
+ # Atomic overwrite (works on Windows and POSIX)
+ Path(tmp_path).replace(install_path)Also applies to: 177-184
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @drorIvry we should test this but it sounds good
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||
| logger.info("rogue-tui not found. Installing from GitHub releases...") | ||||
|
|
||||
| # Get platform information | ||||
| logger.info(f"Detected platform: {self._os}-{self._architecture}") | ||||
| console.print( | ||||
| "[yellow]📦 Installing rogue-tui from GitHub releases...[/yellow]", | ||||
| ) | ||||
|
|
||||
| try: | ||||
| tmp_path = self._download_rogue_tui_to_temp() | ||||
| except Exception: | ||||
| console.print("[red]❌ Failed to download rogue-tui.[/red]") | ||||
| logger.exception("Failed to download rogue-tui.") | ||||
| return False | ||||
|
|
||||
| try: | ||||
| # Move to final location | ||||
| install_path = self._get_install_path() | ||||
| shutil.move(tmp_path, install_path) | ||||
|
|
||||
| with console.status("[bold yellow]Installing rogue-tui...", spinner="dots"): | ||||
| shutil.move(tmp_path, install_path) | ||||
| except Exception: | ||||
| console.print("[red]❌ Failed to install rogue-tui.[/red]") | ||||
| logger.exception("Failed to install rogue-tui.") | ||||
| return False | ||||
|
|
||||
| self._handle_path_env(install_path.parent) | ||||
|
|
||||
| logger.info(f"rogue-tui installed successfully to {install_path}") | ||||
| console.print("[green]✅ rogue-tui installed successfully![/green]") | ||||
| # logger.debug(f"rogue-tui installed to {install_path}") | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
| return True | ||||
Uh oh!
There was an error while loading. Please reload this page.