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

ppm: Update submodule to de6a852bd101e388b61fe67b5 #712

Closed
wants to merge 1 commit into from

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Sep 8, 2023

How to do this kind of update:

When the changes you want are live at the default branch of ppm repo (https://github.com/pulsar-edit/ppm), run the following command in the root of the core repo to update the ppm submodule in the core repo:

git submodule update --remote ppm.

Brief Summary of Changes

Includes a lot of decaf work from multiple contributors, a dependency bump, a small code refactor, a Windows postinstall script fix, and switching to our fork of npm 6 that includes node-gyp 9.x.

Also: Skip fetching updates for packages with repository https://github.com/pulsar-edit/pulsar, since these should be bundled packages, which we don't publish updates for on the package registry anymore, and for which we wish to save the API backend some traffic. (I forgot to mention this in the commit message bumping the ppm submodule, dang.)

Includes the following PR's from ppm repo:

Includes a lot of decaf work from multiple contributors,
a dependency bump, a small code refactor and a Windows postinstall fix
and switching to our fork of npm 6 that includes node-gyp 9.x.
@DeeDeeG DeeDeeG marked this pull request as ready for review September 8, 2023 05:19
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.

Assuming all tests pass, lets get this merged!

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 8, 2023

My hope is to let tests run, especially I am looking for the editor tests and settings-view package tests still being green/passing.

I'm also gonna grab a binary off the "Build Pulsar Binaries" CI job and see how it behaves.

@DeeDeeG DeeDeeG marked this pull request as draft September 10, 2023 00:22
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 10, 2023

So, we're seeing an issue I know a bit about.

/Users/runner/work/pulsar/pulsar/dist/mac/Pulsar.app: invalid destination for symbolic link in bundle

A change (that I authored 😬) in node-gyp (nodejs/node-gyp@b9ddcd5) leaves some symlinks that are meant to help node-gyp execute the right version of Python. Apple's signing process really doesn't like symlinks pointing to outside the signed binary's files, though. And it errors out with the above message.

There is a fix in node-gyp master already. It was merged last month, not in a tagged or published-to-npm-registry release yet. (nodejs/node-gyp@0f1f667)

There's a few other changes since the latest node-gyp release (v9.4.0), such as "Sync deps and engines with npm" is kinda big, but we are already out of sync with not much overlap between new node-gyp 9's deps and old npm 6's deps. The PR I'm referring to: https://github.com/nodejs/node-gyp/pull/2770.

We either need a different version of node-gyp in our forked npm, or back away from the node-gyp bump in ppm for a bit to get other ppm changes into core.

I suppose ideally I will update the references in pulsar's fork of npm 6 to point to a specific commit of node-gyp repo. But that will take some time. If I can't get to that right away, then I suggest we revert the "npm fork / bump node-gyp" stuff in ppm repo and do that over with something that won't break signing Pulsar editor on macOS.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 15, 2023

PR for ppm that hopefully can address the macOS signing situation, and thereby un-block bumping ppm in Pulsar core repo: pulsar-edit/ppm#94

@confused-Techie
Copy link
Member

With #725 open should we go ahead and close this one?

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Sep 20, 2023

@confused-Techie I have the other PR set to auto-close this one, so that should take care of it. If not, this can be manually closed. 👍

@DeeDeeG DeeDeeG closed this in #725 Sep 20, 2023
@DeeDeeG DeeDeeG deleted the update-ppm-de6a852bd1 branch September 20, 2023 02:44
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