macOS CLI script tweaks #1148

fredrikekelund · 2025-03-28T09:39:16Z

Related issues

Follow-up to Implement CLI installer for macOS #1132 and studio cli boostrap files #1111

Proposed Changes

In the POSIX ln utility documentation, the source and target terms refer to the opposite of what you'd expect (if you expect that "target" means link destination and "source" means symlink path). To avoid additional confusion, I've changed the terms to "symlink path" and "packaged path".
As an additional safety measure, we now abort the installation if something other than a symlink already lives at /usr/local/bin/studio
Tweaked the macOS CLI bootstrap script per @mcsf's recommendations in studio cli boostrap files #1111.
- Remove the use of command prefixes.
- Use a static name for the Electron executable file.
- Verify the existence of the Electron executable file.
- Use exec to replace the shell process with the node process.

Tip

An interesting piece of trivia is that the GNU ln utility apparently uses the opposite meaning for "target".

Testing Instructions

Same as #1132

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

mcsf

Thanks for addressing my feedback! One of my comments here is opinion, but the other one should be addressed, I think.

mcsf · 2025-03-28T11:19:55Z

src/modules/cli/lib/install-macos.ts

+	const fileType = await getFileType( cliSymlinkPath );
+
+	// Avoid overwriting an existing file if it's not a symlink.
+	if ( fileType === FileType.NonSymlink ) {
+		throw new Error( InstallError.FileAlreadyExists );


Studio isn't my project, so please take this as opinion beyond the scope of reviewing the shell script. :)

IMO these changes go beyond YAGNI and add unnecessary indirection. getFileType isn't used anywhere else, nor is FileType. There will be less code to maintain and fewer jumps required of the reader if we inline a throw/catch construct with the system call in plain sight.

That's fair. I've inlined this logic

mcsf · 2025-03-28T12:09:07Z

src/modules/cli/lib/install-macos.ts

+		let message = __( 'Please ensure you grant Studio admin permissions when prompted.' );
+		if ( error instanceof Error && error.message === InstallError.FileAlreadyExists ) {
+			message = sprintf(
+				/* translators: 1: Installation path */
+				__(
+					'The installation path %1$s is already occupied by a file or directory. Please remove it and try again.'
+				),
+				cliSymlinkPath
+			);
+		}
+


This fails to propagate legitimate errors occurred during execution of the sudo'd command. Try it yourself by exiting early from install-studio-cli.sh with exit 1, or even echoing an error and exiting with 1. The code assumes a failure will be due to permissions not being granted, but that can be verified by looking at error.message:

Problem error.message

Permission not granted User did not grant permission.

Non-zero exit with no echoed output Command failed: /bin/sh "/Applications/Studio.app/Contents/Resources/bin/install-studio-cli.sh"

Non-zero exit with echoed output Command failed: /bin/sh "/Applications/Studio.app/Contents/Resources/bin/install-studio-cli.sh"\nError: Foo

This is true, but I think it's fair to assume that most errors generated here will be due to missing permissions. Moreover, the logic in install-studio-cli.sh is unlikely to generate errors (especially considering that we verify that cliSymlinkPath is not occupied before running the script).

Still, I added some additional error message parsing here for completeness' sake, so we now also distinguish between known permission errors and unknown errors.

nightnei

LGTM and works still well 👍
Left just two comments about enum and translations.

nightnei · 2025-04-08T13:14:48Z

bin/install-studio-cli.sh

+# This script is used to install the Studio CLI on macOS. It creates a symlink at CLI_SYMLINK_PATH
+# (e.g. /usr/local/bin/studio) pointing to the packaged Studio CLI JS file at CLI_PACKAGED_PATH.


CLI_SYMLINK_PATH and CLI_PACKAGED_PATH are great names 👍
Similar to what I proposed ( CLI_SYMLINK_PATH / CLI_TARGET_PATH ), but CLI_PACKAGED_PATH is much clear 👍

nightnei · 2025-04-08T13:28:30Z

src/modules/cli/lib/install-macos.ts

 const installScriptPath = path.join( binPath, 'install-studio-cli.sh' );

+enum InstallError {


Hm, it seems, I remember that either Tumblr or Dotcom or both didn't recommend using enums, due to tree shaking issues or something like that. Do you remember something about it? Or maybe I remember something incorrectly 😅

Let's wrap strings with translation function.

These error messages aren't displayed to users, so we shouldn't need to translate them. You're right that we don't need an enum, though. I've changed it to regular constants.

mcsf · 2025-04-09T10:19:55Z

src/modules/cli/lib/install-macos.ts

+	try {
+		const stats = await lstat( cliSymlinkPath );
+
+		if ( ! stats.isSymbolicLink() ) {
+			throw new Error( InstallError.FileAlreadyExists );
+		}
+	} catch ( error ) {
+		// File does not exist, do nothing
 	}


Thanks for addressing my feedback in general.

I don't want to be the one delaying this PR any longer, so please use your best judgment here. I just wanted to point out, though, that — although they are unlikely — lstat can potentially throw 13 different errors. So this empty catch block seems a bit bold to me. This is what I meant earlier with propagating the error.

(Currently traveling back from a meetup, so I won't be very responsive.)

Checking for ENOENT errors specifically is fair 👍 I've made that change

sentry-io · 2025-04-09T19:57:23Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ TypeError: Object has been destroyed <anonymous>(main/studio/./src/screenshot-window) View Issue

_{Did you find this useful? React with a 👍 or 👎}

fredrikekelund added 4 commits March 28, 2025 08:56

First pass at improved terminology

7e906f6

More terminology tweaks

f3bb260

Don't overwrite existing non-symlink at installation path

6feef63

Tweak macOS bootstrap script per @mcsf recommendations

034cb5a

fredrikekelund requested review from mcsf, youknowriad, nightnei, bcotrim and a team March 28, 2025 09:39

fredrikekelund self-assigned this Mar 28, 2025

mcsf reviewed Mar 28, 2025

View reviewed changes

fredrikekelund added 2 commits April 8, 2025 13:42

Simplification

676bf78

More nuanced error messages

cf0d512

fredrikekelund requested a review from mcsf April 8, 2025 12:23

nightnei approved these changes Apr 8, 2025

View reviewed changes

mcsf reviewed Apr 9, 2025

View reviewed changes

fredrikekelund added 2 commits April 9, 2025 13:32

Filter for ENOENT exceptions

b0ff16a

Constants over enum

2b6db40

fredrikekelund merged commit 142fa09 into trunk Apr 9, 2025
8 checks passed

fredrikekelund deleted the f26d/cli-install-script-tweaks branch April 9, 2025 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

macOS CLI script tweaks #1148

macOS CLI script tweaks #1148

fredrikekelund commented Mar 28, 2025

mcsf left a comment

mcsf Mar 28, 2025

fredrikekelund Apr 8, 2025

mcsf Mar 28, 2025

fredrikekelund Apr 8, 2025

nightnei left a comment

nightnei Apr 8, 2025

nightnei Apr 8, 2025

nightnei Apr 8, 2025

fredrikekelund Apr 9, 2025

mcsf Apr 9, 2025

fredrikekelund Apr 9, 2025

sentry-io bot commented Apr 9, 2025

Problem	`error.message`
Permission not granted	User did not grant permission.
Non-zero exit with no echoed output	Command failed: /bin/sh "/Applications/Studio.app/Contents/Resources/bin/install-studio-cli.sh"
Non-zero exit with echoed output	Command failed: /bin/sh "/Applications/Studio.app/Contents/Resources/bin/install-studio-cli.sh"`\n`Error: Foo

		# This script is used to install the Studio CLI on macOS. It creates a symlink at CLI_SYMLINK_PATH
		# (e.g. /usr/local/bin/studio) pointing to the packaged Studio CLI JS file at CLI_PACKAGED_PATH.

		const installScriptPath = path.join( binPath, 'install-studio-cli.sh' );

		enum InstallError {

macOS CLI script tweaks #1148

macOS CLI script tweaks #1148

Conversation

fredrikekelund commented Mar 28, 2025

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

mcsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentry-io bot commented Apr 9, 2025

Suspect Issues