-
Notifications
You must be signed in to change notification settings - Fork 50
Add toggle to settings modal to un-/install CLI #2041
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
d6ee128
303d163
9118f5a
d3486c0
73f56d6
7e865d2
485a125
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 |
|---|---|---|
| @@ -1,13 +1,19 @@ | ||
| #!/bin/sh | ||
|
|
||
| # This script is assumed to live in `/Applications/Studio.app/Contents/Resources/bin/studio-cli.sh` | ||
| # The default assumption is that this script lives in `/Applications/Studio.app/Contents/Resources/bin/studio-cli.sh` | ||
| CONTENTS_DIR=$(dirname "$(dirname "$(dirname "$(realpath "$0")")")") | ||
| ELECTRON_EXECUTABLE="$CONTENTS_DIR/MacOS/Studio" | ||
| CLI_SCRIPT="$CONTENTS_DIR/Resources/cli/main.js" | ||
|
|
||
| if ! [ -x "$ELECTRON_EXECUTABLE" ]; then | ||
| echo >&2 "'Studio' executable not found" | ||
| exit 1 | ||
| fi | ||
| if [ -x "$ELECTRON_EXECUTABLE" ]; then | ||
| ELECTRON_RUN_AS_NODE=1 exec "$ELECTRON_EXECUTABLE" "$CLI_SCRIPT" "$@" | ||
| else | ||
| # If the default script path is not found, assume that this script lives in the development directory | ||
| # and look for the CLI JS bundle in the `./dist` directory | ||
| if ! [ -f "$CLI_SCRIPT" ]; then | ||
| SCRIPT_DIR=$(dirname $(dirname "$(realpath "$0")")) | ||
| CLI_SCRIPT="$SCRIPT_DIR/dist/cli/main.js" | ||
| fi | ||
|
|
||
| ELECTRON_RUN_AS_NODE=1 exec "$ELECTRON_EXECUTABLE" "$CLI_SCRIPT" "$@" | ||
| exec node "$CLI_SCRIPT" "$@" | ||
| fi |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| #!/bin/sh | ||
|
|
||
| # This script is used to uninstall the Studio CLI on macOS. It removes the symlink at | ||
| # CLI_SYMLINK_PATH (e.g. /usr/local/bin/studio) | ||
|
|
||
| # Exit if any command fails | ||
| set -e | ||
|
|
||
| if [ -z "$CLI_SYMLINK_PATH" ]; then | ||
| echo >&2 "Error: CLI_SYMLINK_PATH environment variable must be set" | ||
| exit 1 | ||
| fi | ||
|
|
||
| rm "$CLI_SYMLINK_PATH" | ||
|
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. Security: Missing Symlink Validation The script doesn't verify that the symlink actually points to Studio CLI before removing it. Consider adding validation to prevent accidental deletion of similarly-named symlinks: if [ -L "$CLI_SYMLINK_PATH" ]; then
TARGET=$(readlink "$CLI_SYMLINK_PATH")
# Optionally verify TARGET matches expected Studio CLI path
rm "$CLI_SYMLINK_PATH"
else
echo >&2 "Warning: $CLI_SYMLINK_PATH is not a symlink"
exit 1
fiThis would prevent accidental deletion if something else creates a non-symlink file at this path. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,6 @@ import { | |
| import { migrateAllDatabasesInSitu } from 'src/migrations/move-databases-in-situ'; | ||
| import { removeSitesWithEmptyDirectories } from 'src/migrations/remove-sites-with-empty-dirs'; | ||
| import { renameLaunchUniquesStat } from 'src/migrations/rename-launch-uniques-stat'; | ||
| import { installCLIOnWindows } from 'src/modules/cli/lib/install-windows'; | ||
| import { setupWPServerFiles, updateWPServerFiles } from 'src/setup-wp-server-files'; | ||
| import { stopAllServersOnQuit } from 'src/site-server'; | ||
| import { loadUserData, lockAppdata, saveUserData, unlockAppdata } from 'src/storage/user-data'; | ||
|
|
@@ -335,7 +334,6 @@ async function appBoot() { | |
| 'monthly' | ||
| ); | ||
|
|
||
| await installCLIOnWindows(); | ||
| getWordPressProvider(); | ||
|
|
||
|
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. Removal of Auto-Install: Document why this was removed The removal of automatic CLI installation on Windows startup is a significant behavior change. While it's mentioned in the PR description, consider adding a comment in the code explaining why this was removed: // Previously, CLI was auto-installed on Windows startup. This has been moved
// to a user-controlled toggle in Settings to give users more control.
getWordPressProvider(); |
||
| finishedInitialization = true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import { dialog } from 'electron'; | ||
|
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. Looking at No action needed, just wondering your opinion as you know the codebase better and I would like to understand :) |
||
| import { __ } from '@wordpress/i18n'; | ||
| import { getMainWindow } from 'src/main-window'; | ||
| import { | ||
| installCliWithConfirmation as installCliMacOS, | ||
| isCliInstalled as isCliInstalledMacOS, | ||
| uninstallCliWithConfirmation as uninstallCliOnMacOS, | ||
| } from 'src/modules/cli/lib/installation/macos'; | ||
| import { | ||
| installCli as installCliOnWindows, | ||
| isCliInstalled as isCliInstalledWindows, | ||
| uninstallCli as uninstallCliOnWindows, | ||
| } from 'src/modules/cli/lib/installation/windows'; | ||
|
|
||
| export async function isStudioCliInstalled(): Promise< boolean > { | ||
|
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. Code Quality: Linux support should be documented The function returns export async function isStudioCliInstalled(): Promise< boolean > {
switch ( process.platform ) {
case 'darwin':
return await isCliInstalledMacOS();
case 'win32':
return await isCliInstalledWindows();
default:
// CLI installation is not yet supported on Linux
return false;
}
} |
||
| switch ( process.platform ) { | ||
| case 'darwin': | ||
| return await isCliInstalledMacOS(); | ||
| case 'win32': | ||
| return await isCliInstalledWindows(); | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| export async function installStudioCli(): Promise< void > { | ||
| if ( process.env.NODE_ENV !== 'production' ) { | ||
| const mainWindow = await getMainWindow(); | ||
| const { response } = await dialog.showMessageBox( mainWindow, { | ||
| type: 'warning', | ||
| buttons: [ __( 'Proceed' ), __( 'Cancel' ) ], | ||
| title: 'You are running a development version of Studio', | ||
| message: | ||
| 'If you proceed with the CLI installation, the CLI will use the system-level `node` runtime to execute commands instead of the Electron node runtime (which is what is used in production).', | ||
| } ); | ||
|
|
||
| if ( response === 1 ) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if ( process.platform === 'darwin' ) { | ||
| await installCliMacOS(); | ||
| } else if ( process.platform === 'win32' ) { | ||
| await installCliOnWindows(); | ||
| } | ||
| } | ||
|
|
||
| export async function uninstallStudioCli(): Promise< void > { | ||
| if ( process.env.NODE_ENV !== 'production' ) { | ||
| const mainWindow = await getMainWindow(); | ||
| const { response } = await dialog.showMessageBox( mainWindow, { | ||
| type: 'warning', | ||
| buttons: [ __( 'Proceed' ), __( 'Cancel' ) ], | ||
| title: 'You are running a development version of Studio', | ||
| message: | ||
| 'By uninstalling the CLI, you may be removing a version that uses the Electron runtime to execute commands. If you install the CLI again using this version of the app, a different node runtime will be used to execute commands.', | ||
| } ); | ||
|
|
||
| if ( response === 1 ) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if ( process.platform === 'darwin' ) { | ||
| await uninstallCliOnMacOS(); | ||
| } else if ( process.platform === 'win32' ) { | ||
| await uninstallCliOnWindows(); | ||
| } | ||
| } | ||
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.
Security: Missing validation of symlink target
Before removing the symlink, consider verifying that it actually points to a Studio CLI location. This would prevent accidental deletion of symlinks that happen to have the same name but point elsewhere.
Additionally, consider checking if the path is actually a symlink before attempting to remove it to provide better error messages.