-
Notifications
You must be signed in to change notification settings - Fork 51
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
Changes from 11 commits
d6ee128
303d163
9118f5a
d3486c0
73f56d6
7e865d2
485a125
a8fc883
7e8f9d8
7696092
5b3a464
3c6d5f9
447f746
98da288
d5ef7d3
745ed9d
4473289
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 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. 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 { | |
| } from 'src/migrations/migrate-from-wp-now-folder'; | ||
| 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'; | ||
|
|
@@ -333,7 +332,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'; | ||
| 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 > { | ||
|
||
| 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(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,22 +7,23 @@ import { isErrnoException } from 'common/lib/is-errno-exception'; | |
| import { sudoExec } from 'src/lib/sudo-exec'; | ||
| import { getMainWindow } from 'src/main-window'; | ||
| import { getResourcesPath } from 'src/storage/paths'; | ||
| import packageJson from '../../../../package.json'; | ||
| import packageJson from '../../../../../package.json'; | ||
|
|
||
| const cliSymlinkPath = '/usr/local/bin/studio'; | ||
|
|
||
| const binPath = path.join( getResourcesPath(), 'bin' ); | ||
| const cliPackagedPath = path.join( binPath, 'studio-cli.sh' ); | ||
| const installScriptPath = path.join( binPath, 'install-studio-cli.sh' ); | ||
| const uninstallScriptPath = path.join( binPath, 'uninstall-studio-cli.sh' ); | ||
|
|
||
| const ERROR_WRONG_PLATFORM = 'Studio CLI is only available on macOS'; | ||
| const ERROR_FILE_ALREADY_EXISTS = 'Studio CLI symlink path already occupied by non-symlink'; | ||
| // Defined in @vscode/sudo-prompt | ||
| const ERROR_PERMISSION = 'User did not grant permission.'; | ||
|
|
||
| export async function installCLIOnMacOSWithConfirmation() { | ||
| export async function installCliWithConfirmation() { | ||
| try { | ||
| await installCLI(); | ||
| await installCli(); | ||
| const mainWindow = await getMainWindow(); | ||
| await dialog.showMessageBox( mainWindow, { | ||
| type: 'info', | ||
|
|
@@ -65,9 +66,14 @@ export async function installCLIOnMacOSWithConfirmation() { | |
| } | ||
| } | ||
|
|
||
| export async function isCliInstalled() { | ||
| const currentSymlinkDestination = await getCurrentSymlinkDestination(); | ||
|
||
| return currentSymlinkDestination === cliPackagedPath; | ||
| } | ||
|
|
||
| // This function installs the Studio CLI on macOS. It creates a symlink at `cliSymlinkPath` pointing | ||
| // to the packaged Studio CLI JS file at `cliPackagedPath`. | ||
| async function installCLI(): Promise< void > { | ||
| async function installCli(): Promise< void > { | ||
| if ( process.platform !== 'darwin' ) { | ||
| throw new Error( ERROR_WRONG_PLATFORM ); | ||
| } | ||
|
|
@@ -86,10 +92,7 @@ async function installCLI(): Promise< void > { | |
| } | ||
| } | ||
|
|
||
| const currentSymlinkDestination = await getCurrentSymlinkDestination(); | ||
|
|
||
| // The CLI is already installed. | ||
| if ( currentSymlinkDestination === cliPackagedPath ) { | ||
| if ( await isCliInstalled() ) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -112,6 +115,69 @@ async function installCLI(): Promise< void > { | |
| } | ||
| } | ||
|
|
||
| export async function uninstallCliWithConfirmation() { | ||
| try { | ||
| await uninstallCli(); | ||
| const mainWindow = await getMainWindow(); | ||
| await dialog.showMessageBox( mainWindow, { | ||
| type: 'info', | ||
| title: __( 'CLI Uninstalled' ), | ||
| message: __( 'The CLI has been uninstalled successfully.' ), | ||
| } ); | ||
| } catch ( error ) { | ||
| Sentry.captureException( error ); | ||
| console.error( 'Failed to uninstall CLI', error ); | ||
|
|
||
| let message: string = __( | ||
| 'There was an unknown error. Please check the logs for more information.' | ||
| ); | ||
|
|
||
| if ( error instanceof Error ) { | ||
| message = error.message; | ||
| } | ||
|
|
||
| const mainWindow = await getMainWindow(); | ||
| await dialog.showMessageBox( mainWindow, { | ||
| type: 'error', | ||
| title: __( 'Failed to uninstall CLI' ), | ||
| message, | ||
| } ); | ||
| } | ||
| } | ||
|
|
||
| async function uninstallCli() { | ||
| if ( process.platform !== 'darwin' ) { | ||
| throw new Error( ERROR_WRONG_PLATFORM ); | ||
| } | ||
|
|
||
| try { | ||
| const stats = await lstat( cliSymlinkPath ); | ||
|
|
||
| if ( ! stats.isSymbolicLink() ) { | ||
| throw new Error( ERROR_FILE_ALREADY_EXISTS ); | ||
| } | ||
| } catch ( error ) { | ||
| if ( isErrnoException( error ) && error.code === 'ENOENT' ) { | ||
| // File does not exist, which means we can proceed | ||
| } else { | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| await unlink( cliSymlinkPath ); | ||
| } catch ( error ) { | ||
| // `/usr/local/bin` is not typically writable by non-root users, so in most cases, we run | ||
| // this uninstall script with admin privileges to remove the symlink. | ||
| await sudoExec( `/bin/sh "${ uninstallScriptPath }"`, { | ||
| name: packageJson.productName, | ||
| env: { | ||
| CLI_SYMLINK_PATH: cliSymlinkPath, | ||
| }, | ||
| } ); | ||
| } | ||
| } | ||
|
|
||
| async function getCurrentSymlinkDestination(): Promise< string | null > { | ||
| try { | ||
| return await readlink( cliSymlinkPath ); | ||
|
|
||
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.
It's a bit out of scope here, but as you are making more improvements around installing/uninstalling, I would like ask - what do you think about unsetting NODE_OPTIONS in the wrapper to fix the warning that can appear in console when it's set for the terminal session?
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 is fair 👍 I've made the change
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.
Thanks!
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.
For future reference, Wojtek was referring to the warning described in #2128