Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fix getResourcePath so more apm commands work on Windows #906

Merged
merged 11 commits into from
Oct 21, 2020

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Oct 1, 2020

Description of the Change

  • This fixes getResourcePath so it finds the installation location on Windows. It results in the correct behavior of several apm commands (such as apm --version) when apm isn't bundled with Atom.

  • It also optimizes getResourcePath by storing a cache of the asarPath. This means asarPath is only calculated once.

Benefits

Allows more apm commands (examples: apm --version, apm install [package-name])
to work on Windows, even outside of a full Atom installation. For example:

  • Allows apm --version to work during the Atom bootstrap script.
  • Allows testing apm from inside its own git repository on Windows.

Possible Drawbacks

None.

Verification Process

The tests are included: https://github.com/atom/apm/pull/906/files#diff-845586f656569b8df7748376c8e75465R47

Manual tests:

Manually ran apm --version in the apm Git repo. Output is useful and not blank.

Sample output (click to expand):
PS C:\Users\User\Downloads\apm> .\bin\apm.cmd --version
apm  2.5.2-atomic.2
npm  6.14.8
node 12.4.0 x64
atom 1.51.0
python 2.7.18
git 2.27.0.windows.1
visual studio 2019

Manually ran .\bin\apm.cmd install hydrogen. Package now installs. (Before this PR, the command quickly exits with no logging or output.)

Also, CI should pass.

Applicable Issues

None.

Extra info

TL;DR

Info about what getResourcePath() does

The getResourcePath() function's job is to find an install of Atom on the disk. (Specifically, it gives the path to the app.asar application package/archive that holds most of Atom's JS code, bundled packages, various binaries, and so on.

Because Atom install paths on Windows are version-specific, and because at least the previous version is meant to stay around after an update, users may have more than one Atom install, and we have to pick one of the multiple installs. In this case, I just made sure to pick the newest install (among installs having a resources/app.asar file).

Long explanation (click to expand):

After adding this fallback behavior, apm should be able to find stable installations of Atom on the disk, even when apm isn't bundled with the Atom install it's looking for.

(This mostly requires that the user have an Atom installation on disk. But it also makes apm behave slightly better even when there is no Atom installed to disk. For example, apm --version used to print nothing at all on Windows when apm wasn't bundled with Atom. Now it will print all versions it can find. If there was no Atom install found on disk, the Atom version will now print as "unknown". apm install will give you the relevant error message Could not determine Electron version rather than exiting with no logging or output at all.)

Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks for adding the tests.

@sadick254 sadick254 merged commit 86e0eb8 into atom:master Oct 21, 2020
@aminya
Copy link
Contributor Author

aminya commented Oct 21, 2020

You're welcome!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants