-
Notifications
You must be signed in to change notification settings - Fork 55
Make Windows CLI installation tolerate app reinstalls #2463
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
Make Windows CLI installation tolerate app reinstalls #2463
Conversation
| // `unversionedBinDirPath` resolves to C:\Users\<USERNAME>\AppData\Local\studio\bin | ||
| const unversionedBinDirPath = path.resolve( path.dirname( app.getPath( 'exe' ) ), '../bin' ); | ||
| // `stableBinDirPath` resolves to C:\Users\<USERNAME>\AppData\Local\studio\bin | ||
| const stableBinDirPath = path.resolve( path.dirname( app.getPath( 'exe' ) ), '../bin' ); |
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.
I found this name easier to understand.
📊 Performance Test ResultsComparing 214cd88 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
| if ( this.isStudioCliInPath( currentPath ) ) { | ||
| return; | ||
| } |
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 check is redundant. Better to do this before calling WindowsCliInstallationManager::installPath
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.
I take it back. Without this check, we might install the same path multiple times
| if ( await this.isCliInstalled() ) { | ||
| if ( await this.isStudioCliDirInPath() ) { |
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 really the core change I made: instead of checking both whether the CLI path is added to the registry PATH and whether the CLI path exists, we only check the registry PATH.
| // Return true if we are running the development version of the app and the production CLI is installed | ||
| if ( process.env.NODE_ENV !== 'production' && process.env.LOCALAPPDATA ) { | ||
| const prodStudioCliDir = path.join( process.env.LOCALAPPDATA, 'studio', 'bin' ); | ||
| if ( this.isStudioCliInPath( currentPath, prodStudioCliDir ) ) { | ||
| return true; | ||
| } | ||
| } |
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.
I moved this logic to the isStudioCliDirInPath method.
ivan-ottinger
left a comment
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.
Thank you for including detailed description and comments in the code. That helped.
The changes look good to me and testing steps work as expected. CLI stayed operational after app update. I did not observe any regressions.
|
Update: Sorry, false alarm, I just got confused by the version in the About window. All is good. 👍🏼 |
gcsecsey
left a comment
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.
The changes make sense to me, but I ran into an issue when testing this on Parallels.
After following these steps:
- Installed Studio 1.6.8 release
- Installed the CLI through the modal
- Checked that the
C:\Users\<USERNAME>\AppData\Local\studio\bindirectory is appended toPATHand that thestudiocommand works - Installed the dev build
- Checked the CLI status on the modal
I can see the modal showing CLI as not installed. However, clicking the "Command Prompt" button then invoking studio command works as expected, the C:\Users\<USERNAME>\AppData\Local\studio\bin still exists and it's in PATH.
|
Hmm, let me test that same flow to check, @gcsecsey |
|
We're investigating the issue @gcsecsey uncovered here ☝️ It doesn't seem related to the changes in this PR, though, so we're landing this first and potentially following up with another PR for the AppX issue. |

Related issues
Proposed Changes
Note
From the user's perspective, I don't believe it's completely necessary to change this behavior. However, it will make the CLI installation more resilient in our internal smoke testing, which seems like a noble cause in and of itself.
When installing the CLI on Windows,
WindowsCliInstallationManagerdoes two things:C:\Users\<USERNAME>\AppData\Local\studio\binto thePATHvariable in the registryC:\Users\<USERNAME>\AppData\Local\studio\bindirectory and writes astudio.batfile to itThe reason we do it this way is that the app resources live in a versioned directory (i.e.
C:\Users\<USERNAME>\AppData\Local\studio\app-1.7.0-beta3). Instead of changing thePATHvariable in the registry every time the app starts, we use a known, stable path and change the contents ofC:\Users\<USERNAME>\AppData\Local\studio\bin\studio.batwhen the app starts.This logic plays nicely with the auto-updater. However, it breaks when the app is manually reinstalled. When that happens, the
PATHvariable in the registry isn't touched, but the entireC:\Users\<USERNAME>\AppData\Local\studiodirectory is wiped, effectively uninstalling the CLI.The
WindowsCliInstallationManager::updateWindowsCliVersionedPathIfNeededlogic (which runs when the app starts and updates the contents ofC:\Users\<USERNAME>\AppData\Local\studio\bin\studio.bat) relies on the presence of theC:\Users\<USERNAME>\AppData\Local\studiodirectory. This PR changes that method to only look forC:\Users\<USERNAME>\AppData\Local\studio\binin the registry PATH and proceed with writing thestudio.batscript if the registry key is there.Testing Instructions
studiocommand is available in the command promptPre-merge Checklist