-
Notifications
You must be signed in to change notification settings - Fork 14
Add post-install tmux nudges #41
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { spawnSync } from "node:child_process"; | ||
| import { | ||
| access, | ||
| chmod, | ||
|
|
@@ -18,6 +19,46 @@ const CODEX_ALIAS_NAME = IS_WINDOWS ? "codex-loop.cmd" : "codex-loop"; | |
| const CANDIDATE_BINARIES = IS_WINDOWS | ||
| ? ["loop.exe", "loop"] | ||
| : ["loop", "loop.exe"]; | ||
| const TMUX_NOTE = "Note: tmux is not installed."; | ||
| const TMUX_DEFAULT_MODE_NOTE = | ||
| "The default 'loop' command opens a paired tmux workspace and will fail until tmux is installed."; | ||
| const TMUX_MACOS_HINT = "brew install tmux"; | ||
| const TMUX_LINUX_HINT = "your package manager (for example: apt install tmux)"; | ||
|
|
||
| const tmuxInstallHint = ( | ||
| platform: NodeJS.Platform = process.platform | ||
| ): string => { | ||
| if (platform === "darwin") { | ||
| return TMUX_MACOS_HINT; | ||
| } | ||
| if (platform === "linux") { | ||
| return TMUX_LINUX_HINT; | ||
| } | ||
| return "install tmux"; | ||
| }; | ||
|
Comment on lines
+28
to
+38
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. This function provides helpful installation hints. To improve support for other Unix-like systems, you could provide the generic Linux hint for platforms like FreeBSD and OpenBSD, instead of the less helpful default. A Also, consider adding test cases for these new platforms in const tmuxInstallHint = (
platform: NodeJS.Platform = process.platform
): string => {
switch (platform) {
case "darwin":
return TMUX_MACOS_HINT;
case "linux":
case "freebsd":
case "openbsd":
case "sunos":
return TMUX_LINUX_HINT;
default:
return "install tmux";
}
}; |
||
|
|
||
| const tmuxNudgeLines = ( | ||
| platform: NodeJS.Platform = process.platform | ||
| ): string[] => [ | ||
| "", | ||
| TMUX_NOTE, | ||
| TMUX_DEFAULT_MODE_NOTE, | ||
| `Install tmux with: ${tmuxInstallHint(platform)}`, | ||
| ]; | ||
|
|
||
| const hasTmuxInstalled = (): boolean => { | ||
| const result = spawnSync("tmux", ["-V"], { stdio: "ignore" }); | ||
| return !result.error && result.status === 0; | ||
| }; | ||
|
|
||
| const logMissingTmuxNudge = (): void => { | ||
| if (IS_WINDOWS || hasTmuxInstalled()) { | ||
| return; | ||
| } | ||
| for (const line of tmuxNudgeLines()) { | ||
| console.log(line); | ||
| } | ||
| }; | ||
|
|
||
| const findBuiltBinary = async (): Promise<string> => { | ||
| for (const name of CANDIDATE_BINARIES) { | ||
|
|
@@ -76,23 +117,32 @@ const installBinary = async (): Promise<void> => { | |
|
|
||
| if (IS_WINDOWS) { | ||
| await copyFile(source, target); | ||
| console.log(`Installed loop -> ${target}`); | ||
| await installAliases(); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await symlink(source, target); | ||
| } catch { | ||
| await copyFile(source, target); | ||
| } else { | ||
| try { | ||
| await symlink(source, target); | ||
| } catch { | ||
| await copyFile(source, target); | ||
| } | ||
| } | ||
|
|
||
| console.log(`Installed loop -> ${target}`); | ||
| await installAliases(); | ||
| logMissingTmuxNudge(); | ||
| }; | ||
|
|
||
| export const installInternals = { | ||
| tmuxInstallHint, | ||
| tmuxNudgeLines, | ||
| }; | ||
|
|
||
| const main = async (): Promise<void> => { | ||
| await installBinary(); | ||
| }; | ||
|
|
||
| installBinary().catch((error: unknown) => { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| console.error(`[loop] install failed: ${message}`); | ||
| process.exit(1); | ||
| }); | ||
| if (import.meta.main) { | ||
| main().catch((error: unknown) => { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| console.error(`[loop] install failed: ${message}`); | ||
| process.exit(1); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { installInternals } from "../src/install"; | ||
|
|
||
| test("tmux install hint uses brew on macOS", () => { | ||
| expect(installInternals.tmuxInstallHint("darwin")).toBe("brew install tmux"); | ||
| }); | ||
|
|
||
| test("tmux install hint stays generic on Linux", () => { | ||
| expect(installInternals.tmuxInstallHint("linux")).toBe( | ||
| "your package manager (for example: apt install tmux)" | ||
| ); | ||
| }); | ||
|
|
||
| test("tmux nudge explains why bare loop fails without tmux", () => { | ||
| expect(installInternals.tmuxNudgeLines("linux")).toEqual([ | ||
| "", | ||
| "Note: tmux is not installed.", | ||
| "The default 'loop' command opens a paired tmux workspace and will fail until tmux is installed.", | ||
| "Install tmux with: your package manager (for example: apt install tmux)", | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new function relies on the
$osvariable. It's worth noting that thecasestatement that sets$oson lines 52-56 only handlesDarwinandLinux. For other Unix-like systems (e.g., FreeBSD), it will fall back to the default case and exit the script with a 'Windows is not supported' error. This prevents the installer from running at all on those platforms.While the fix is outside of this diff, it's a significant correctness issue affecting this new functionality. The script should be updated to correctly identify other Unix-like systems to ensure the installer works on all supported platforms.