Skip to content
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

Enable multiple release channels to be installed side-by-side on Windows #17813

Merged
merged 31 commits into from Jul 16, 2019

Conversation

@daviwil
Copy link
Contributor

commented Aug 9, 2018

Description of the Change

This change updates Atom's release build, packaging, and installer creation so that unique installations for each Atom channel can be used simultaneously on Windows. This is achieved by using a specific app title including the channel name when assigning the "product name" during packaging and installer creation.

NOTE: I've also added a new atom.getAppName() API to AtomEnvironment to make it easy to access the channel-qualified app title in the renderer process. This name probably isn't ideal so I'd be happy to hear opinions on possible alternatives:

  • getAppTitle()
  • getAtomTitle()
  • getTitle()

Alternate Designs

Based on how Electron apps are packaged on all 3 platforms, this is the only possible approach.

Why Should This Be In Core?

This affects the release builds and some app code for Atom's release channels.

Benefits

The primary benefit is that Atom users on Windows will now be able to install Atom Stable, Atom Beta, and Atom Nightly and then use and update them independently of one another. A secondary benefit is that now Windows and Linux builds of Atom now reference the correct channel in their title bars (e.g. "Atom Beta" or "Atom Nightly").

Possible Drawbacks

There aren't any real drawbacks or tradeoffs here, but proper verification will be needed to make sure that no regressions are introduced.

Verification Process

  • Ensure that all release channels can be built and installed simultaneously on Windows
    • Atom Stable
    • Atom Beta
    • Atom Nightly
  • Ensure that macOS and Linux builds still function correctly with these packaging changes
    • macOS
      • Atom Stable
      • Atom Beta
      • Atom Nightly
    • Linux
      • Atom
      • Atom Beta
      • Atom Nightly
  • Existing CI tests pass after added channel-specific Atom title bar strings
  • Updating existing release channels on Windows doesn't cause errors or bad user experience (these updates will be simulated through local testing)
    • 1.29.0 -> 1.29.1
    • 1.30.0-beta1 -> 1.30.0-beta2
    • 1.31.0-nightly5 -> 1.31.0-nightly6
  • Ensure that atom.cmd/apm.cmd launches the right Atom channel (might need to add atom-<channel>.cmd to differentiate)
  • All three Atom icons appear separately on the taskbar when running
  • Ensure that all channels are added to PATH on Windows
    • Ensure that Atom stable is added to PATH when installing
    • Ensure that Atom beta is added to PATH when installing
    • Ensure that Atom nightly is added to PATH when installing
  • Ensure that we can run all channels from the CLI
    • Atom stable
    • Atom beta
    • Atom nightly
  • Ensure that shell integration works (Right click menu in explorer for example)
    • Atom stable
    • Atom beta
    • Atom nightly

Applicable Issues

Fixes #9247.

@Ben3eeE

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

❤️ ❤️ ❤️ ❤️ ❤️

@daviwil daviwil changed the title WIP: Enable multiple release channels to be installed simultaneously on Windows WIP: Enable multiple release channels to be installed side-by-side on Windows Aug 10, 2018

@Ben3eeE

This comment was marked as outdated.

Copy link
Member

commented Aug 10, 2018

@daviwil We should also test CLI/shell integration:

  • Ensure that all channels are added to PATH on Windows
    • Ensure that Atom stable is added to PATH when installing
    • Ensure that Atom beta is added to PATH when installing
    • Ensure that Atom nightly is added to PATH when installing
  • Ensure that we can run all channels from the CLI
    • Atom stable
    • Atom beta
    • Atom nightly
  • Ensure that shell integration works (Right click menu in explorer for example)
    • Atom stable
    • Atom beta
    • Atom nightly

The PATH/cli test ensures that the commands in (Currently) %LOCALAPPDATA%\atom\bin are unique so we can run any channel from the command line. I believe this is done with atom-beta/atom on macOS now?

The shell integration test ensures that the shell integration works for any channel. These are the settings you can configure in the settings-view system panel on Windows.

@daviwil daviwil force-pushed the dw-windows-separate-channels branch from 4bb2d83 to 4174887 Sep 18, 2018

David Wilson added some commits Sep 18, 2018

@CollinChaffin

This comment has been minimized.

Copy link

commented Oct 6, 2018

One WIN7x64 systems, the above changes in this open issue have changed the system PATH which upon upgrading to 1.32.0-beta3, leaves a now invalid orphan entry in the system PATH of C:\Users\%USERNAME%\Appdata\Local\Atom\bin when the called shim exe now sits a level higher at C:\Users\%USERNAME%\Appdata\Local\Atom with the shim calling the actual active binaries in a subfolder at C:\Users\%USERNAME%\Appdata\Local\Atom\app-1.32.0-beta3, neither of which are actually even re-added with the upgrade.

So, two parts to this issue:

  1. Determine (and add back to future installers) if atom should be properly added to system PATH (and is it the SHIM exe I assume if I were to add manually for the time being)?
  2. Clean up/Delete (add to installers for upgrading actions) prior (now depreciated) bin path (which I assume is okay manually since bin no longer exists after upgrade)?
@daviwil

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Some notes on the work that remains for this PR:

One problem I came across is that Squirrel wants to see the .nupkg files named with the name field as in package.json of the release build. This means that an Atom Beta release would now have files named atom-beta-1.33.0-beta0-full.nupkg instead of atom-1.33.0-beta0-full.nupkg as it is now.

The implication here is that we'd have to change atom.io's download and update controller logic and our release publishing logic to expect these different filenames for non-stable release channels. This is certainly possible but the work would need to be coordinated with the timing of this PR so that everything continues to work as expected in future releases.

@rafeca

This comment was marked as resolved.

Copy link
Contributor

commented May 31, 2019

Hey! 👋

We have recently started using prettier and this has caused major style changes on the Atom JS codebase (you can check the related PR).

This is good news: now the Atom code is more consistent and it's much easier to re-format the code to follow the code style (now you only need to run script/lint --fix).

This change caused conflicts on your PR that we have automatically solved, hope you don't mind 😄

With ❤️, the Atom team.

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@Ben3eeE do you want to takeover this PR and drive it to the finish line? I can provide any needed support and it will be a really impactful improvement to Windows users 😃

Regarding @daviwil comment:

One problem I came across is that Squirrel wants to see the .nupkg files named with the name field as in package.json of the release build. This means that an Atom Beta release would now have files named atom-beta-1.33.0-beta0-full.nupkg instead of atom-1.33.0-beta0-full.nupkg as it is now.

The implication here is that we'd have to change atom.io's download and update controller logic and our release publishing logic to expect these different filenames for non-stable release channels. This is certainly possible but the work would need to be coordinated with the timing of this PR so that everything continues to work as expected in future releases.

I can take care of it: I've found a way to be able to make the needed changes on atom.io independently from this PR (they just need to happen before this gets merged).

@rafeca rafeca referenced this pull request Jun 25, 2019
0 of 8 tasks complete

rafeca and others added some commits Jun 25, 2019

Specify exe in the metadata
Because we changed it to be atom-beta.exe on beta for example this is 
required for electron-winstaller to find the executable
Use a different name depending on channel
This makes Atom beta install in %LOCALAPPDATA%\atom-beta and stable in 
%LOCALAPPDATA%\atom so that installs are side by side
@Ben3eeE

This comment was marked as resolved.

Copy link
Member

commented Jun 25, 2019

I had some issues getting this to build and work initially. I pushed two commits that enabled me to install stable and beta side by side. I've tested running the atom and atom-beta command and that works.

I noticed a few issues:

  • The settings view systems tab doesn't open. This seems to be because appName is no longer exported from win-shell. Among other things.
  • The title is still Atom even when on beta. Expected is that it's Atom Beta based on the PR body description
  • Exception thrown when using project wide search and clicking results that I don't see on nightly
  • Unable to pin both stable and beta to the task bar. This is because the AppUserModelID is the same for both. Adding the executable path (process.execPath) makes it so we can pin them separately again but doesn't work because then we start showing duplicate items on the task bar on Windows 10 again 😅
    // NB: This prevents Win10 from showing dupe items in the taskbar
    app.setAppUserModelId('com.squirrel.atom.' + process.arch);

rafeca added some commits Jun 26, 2019

Create get-app-name module that returns correct application name
Before, in order to retrieve the application name, Electron's
`getName()` method was used (https://electronjs.org/docs/api/app#appgetname).

Now, instead, we also use the Atom version in order to calculate the release
channel and be able to have it on the app name (e.g `Atom Nightly`).

@rafeca rafeca force-pushed the dw-windows-separate-channels branch from 73c1981 to f7b7545 Jul 4, 2019

Revert breaking changes on the WinShell module
In earlier commits from this PR, some breaking changes were done to the
WinShell module, which cause some issues on the `settings` package (and
potentially other packages).

Since these breaking changes are not needed (and they don't provide even
a better API), this PR reverts them to keep the previous contract.

@rafeca rafeca force-pushed the dw-windows-separate-channels branch from 40154b5 to cbe5495 Jul 4, 2019

Ben3eeE and others added some commits Jul 4, 2019

Append the release channel to the AppUserModelId
This allows each release channel to be pinned separately on Windows
Make get-app-name module compatible with the renderer process
This module is required both from the main process and the renderer
process, since `win-shell.js` is also required from both processes
(which is nuts).

In order to make it work when used from the main process, `get-app-name`
just falls back to use the `atom-environment` `getAppName()` method.
Fix creation of binary folders
On cbe5495 I forgot to update the
callers of handleStartupEvent() and restartAtom() which no longer expect
an app object to be passed.
Rename the sh commands in the bin folder
This fixes an issue where atom stable would not launch correctly from 
powershell if it was not the first entry in path because it tries to 
execute the shell script from another channel
Do not add the release channel on stable versions of Atom
This is done to avoid changing the application user model id on Atom
stable, which prevents pins from currently installed Atom stable
versions to stop working.

@rafeca rafeca force-pushed the dw-windows-separate-channels branch from 419d583 to 603800f Jul 15, 2019

@rafeca rafeca changed the title WIP: Enable multiple release channels to be installed side-by-side on Windows Enable multiple release channels to be installed side-by-side on Windows Jul 15, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Quick update! After an amazing job done by @Ben3eeE identifying issues and fixing them, we're ready to merge this PR! 🎉

We've done an exhaustive testing on it, but there's still risk that this introduces potential issues since it changes a few things around the Windows installer. We're still going to merge it and use the beta period to be able to identify potential issues.

@rafeca rafeca merged commit d7f7b4f into master Jul 16, 2019

1 check passed

Atom Pull Requests #20190715.2 succeeded
Details

@rafeca rafeca deleted the dw-windows-separate-channels branch Jul 16, 2019

@rafeca rafeca self-assigned this Jul 16, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Quick update: Atom v1.40-nightly25 includes this PR and can be installed along other Atom versions already! 🎉

I've done some tests and previous nightly versions get automatically upgraded to the new one, but I could find two issues after the upgrade:

  1. The upgrade is installed on %APPDATA%/Local/atom (which is the same folder as the previous installation, to override it), while a new installation of the new nightly builds will get installed on %APPDATA%/Local/atom-nightly. This prevents users from having side by side installations via automatic upgrades (once they install the stable version of Atom, it'll override the new nightly).
  2. The desktop shortcut does not get updated, so it still says Atom instead of Atom Nightly and points to %APPDATA%/Local/atom/atom.exe instead of %APPDATA%/Local/atom/atom-nightly.exe. This prevents it from working correctly.

(note that these two issues won't happen on the stable version of Atom, since the naming hasn't changed there).

These issues are quite bad (specially the desktop icon one), but they are caused by Squirrel autoupdate workflow, so it may be hard to fix them.

Since they will only happen once (when auto-upgrading to the first version with the new Atom namings), the visibility is low (only beta or nightly Windows users are affected, which is ~0.34% of Atom users) and the workaround is simple enough (download a new nightly/beta version of Atom and re-install it), I think it should be fine to leave this issue as a known problem. If we decide to do so, we can document it on the release notes for v1.40-beta (cc/ @jasonrudolph ).

Alternative solutions

  1. We could try to fix the root issue on Squirrell.Windows, but without being familiar with it it looks like a really big effort for the rare edge case of changing an app name and expecting the upgrade process to run smoothly (maybe @shiftkey can provide some suggestions here).
  2. We could prevent older versions of Atom Beta/Nightly to get upgraded to the version with the new names (and even display a message telling users to reinstall Atom to get upgrades). I would suggest doing that if this affected the stable Atom version, but since it only affects Beta/Nightly I think that the effort of implementing this is not worth it and we can communicate the same message to users via the release notes instead.

Thoughts?

@UziTech

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Looks like this breaks a few things that rely on knowing where atom.exe is located.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.