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

src: Update default Pulsar install paths #49

Merged
merged 3 commits into from
Feb 24, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Jan 4, 2023

Changes:

Updates the default install paths for each OS, based on where our new bundled packages (made by electron-builder) install to.

Relatedly, adjust logic which finds the app on macOS, for the rebrand.


Notes:

  • A relative path from ppm's install dir in the bundled package is tried first, and then the default absolute install path of the whole app is hard-coded as a fallback. The relative path should work for any fully bundled/packaged copy of Pulsar.
  • This hard-coded fallback shouldn't be needed in actual installs of Pulsar -- this commit is mostly useful for situations where ppm is not bundled with the editor, such as when developing or testing ppm in the context of its own git repo.
  • on macOS, ppm also uses a search-on-disk "mdfind" command to find other potential install paths, before the hard-coded one, for whatever historical reason.

Context:

This install path is used primarily to locate and read the editor's package.json, which gives various useful metadata for ppm to use, but also to find whatever files are needed from the editor bundle.

For example: this metadata is notably used to determine what Electron version to build native C/C++ code for, in whatever packages ppm is used to build.

(ppm reads the "electronVersion" field from Pulsar's package.json, which is located in Pulsar's "app.asar" bundle.)
(See: src/command.coffee)

Pulsar's package.json is also read to get Pulsar's version number. (See: src/command.coffee, src/apm-cli.coffee)

It is also read to get the list of bundled packages in the editor. (See: src/install.coffee, src/list.coffee)

The install path is also used to find/load the src/compile-cache.js module from among the editor's own source files.
(See: src/install.coffee)

Likewise, it is used to find/load the src/module-cache.js module from among the editor's own source files.
(See: src/rebuild-module-cache.coffee)

@DeeDeeG DeeDeeG force-pushed the update-hardcoded-pulsar-install-paths branch 2 times, most recently from a3b452e to 65f5995 Compare January 4, 2023 20:26
   Changes:

Updates the default install paths for each OS, based on where our new
bundled packages (made by electron-builder) install to.

Relatedly, adjust logic which finds the app on macOS, for the rebrand.

---

   Notes:

- A relative path from ppm's install dir in the bundled package
is tried first, and then the default absolute install path of the
whole app is hard-coded as a fallback. The relative path should work
for any fully bundled/packaged copy of Pulsar.

- This hard-coded fallback shouldn't be needed in actual installs
of Pulsar -- this commit is mostly useful for situations where ppm
is not bundled with the editor, such as when developing or testing ppm
in the context of its own git repo.

- On macOS, ppm also uses a search-on-disk "mdfind" command to find
other potential install paths, before the hard-coded one, for whatever
historical reason.

---

   Context:

This install path is used primarily to locate and read the editor's
package.json, which gives various useful metadata for ppm to use,
but also to find whatever files are needed from the editor bundle.

For example: this metadata is notably used to determine
what Electron version to build native C/C++ code for,
in whatever packages ppm is used to build.

(ppm reads the "electronVersion" field from Pulsar's package.json,
which is located in Pulsar's "app.asar" bundle.)
(See: src/command.coffee)

Pulsar's package.json is also read to get Pulsar's version number.
(See: src/command.coffee, src/apm-cli.coffee)

It is also read to get the list of bundled packages in the editor.
(See: src/install.coffee, src/list.coffee)

The install path is also used to find/load the src/compile-cache.js
module from among the editor's own source files.
(See: src/install.coffee)

Likewise, it is used to find/load the src/module-cache.js module
from among the editor's own source files.
(See: src/rebuild-module-cache.coffee)
@DeeDeeG DeeDeeG force-pushed the update-hardcoded-pulsar-install-paths branch from 65f5995 to ca316fb Compare January 4, 2023 20:27
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jan 4, 2023

I verified that the default install paths for Linux, macOS and Windows were as I adjusted them in this commit, as of December 22.

If it's changed since then, this PR is out of date. Might be after the recent macOS and Windows build metadata improvement PRs over at Pulsar?

EDIT: I think I need to check C:\Program Files\Pulsar now too, for system-wide installs, now that pulsar-edit/pulsar#279 is merged in Pulsar core.

Note: I updated the macOS identifier from com.github.atom initially to com.pulsar-edit.pulsar for our app bundle, but after the macOS build improvement PR over at Pulsar core repo (pulsar-edit/pulsar#280), I amended the commit to point to dev.pulsar-edit.pulsar starting properly with .dev.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

I've gone ahead and tried to help out adding the fallback machine install directory of where the asar archive may exist.

Now fair warning I'm unfamiliar with this code so I'd recommend double checking it with you being a bit more familiar as the author of the PR.

But you are right that this also needs to be a fallback on Windows systems after the recent binary build changes.

DeeDeeG and others added 2 commits January 5, 2023 00:07
This is the default path used when the user installs system-wide,
rather than installing per-user.

Co-authored-by: confused_techie <[email protected]>
Use the new capitalization of the per-user install path, even though
Windows is generally case-insensitive for filenames/paths,
and in theory/in testing it doesn't matter. I'm just playing it safe.
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Jan 5, 2023

Thanks for that fix, I tested it and it's working exactly as you wrote it, so I went ahead and committed it.

(Also, I went ahead and capitalized the /Pulsar for the per-user install based on what I saw for the suggested per-user install path on the latest Windows installer from master. Should have no effect per my testing, it works exactly the same with either capitalization or just lowercase, due to Windows being case-insensitive about paths/filenames. But just to note that the non-interactive 1.100.0-beta installer had installed itself per-user to .../pulsar (lowercase) and the new interactive installer from the latest master build I grabbed is suggesting to install to .../Pulsar (capitalized). But the real reason I did it is secretly so they would match and the code would look more pleasing to the eye. I guess maybe Windows Subsystem for Linux, or Bash shells, could somehow make this relevant??? But I think Node is setting the behavior here and it should be standardized that way? But, yeah. Just playing it safe. And making the code slightly more aesthetically nice, lol.)

(And thanks again for adding the system-wide install path! Was easier to just test what you wrote than to have to reverse-engineer it myself and be less confident when testing it.)

After that super long sidebar... This is ready for review again!

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Approving this one, great work here @DeeDeeG would be happy to get this merged

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 24, 2023

Thanks for the review, I confirmed the Windows installer shows these same paths, and I 'm not expecting the other OSes to have changed...

Merging. 🎉

EDIT: Just noting the tests pass for me on Linux, when rebased on master.

@DeeDeeG DeeDeeG merged commit 68257a4 into master Feb 24, 2023
@DeeDeeG DeeDeeG deleted the update-hardcoded-pulsar-install-paths branch March 8, 2023 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants