pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.24k stars 137 forks source link

Fix macOS binaries by vendorizing `libiconv` #1051

Closed savetheclocktower closed 2 months ago

savetheclocktower commented 2 months ago

This is a continuation of the saga described in #1048 and #1045.

The necessary fixes have been applied to the superstring repo.

Other than a quick and temporary change to CI — to verify that signed binaries build correctly without triggering Gatekeeper on macOS — this PR will fix our libiconv woes for the foreseeable future:

This PR will sit in draft until I'm able to verify that it produces a signed Pulsar release that passes Gatekeeper. After all the tests of the past few days, I'm satisfied that a working, signed binary on Intel demonstrates that the binary on Apple Silicon would have the same characteristics, so at that point I'll revert the temporary CI changes and take this PR out of draft.

I might then owe someone some money for all the CirrusCI minutes I used in the last week, but we can settle that later. ;-)

savetheclocktower commented 2 months ago

The Intel macOS binary works just fine on my machine and does not anger Gatekeeper. I've reverted the temporary CI changes and taken this PR out of draft. If it pleases you, leave a 👍 and we can get it landed.

DeeDeeG commented 2 months ago

Noting as slightly tangential commentary (arguably even off-topic, but it's sorta related)... since people are likely to run into this if revisiting superstring repo after all these changes -- just a heads-up: Due to reverting https://github.com/pulsar-edit/superstring/pull/1, I do believe building superstring (and by extension Pulsar) with Node 18 or higher on the PATH won't work, since https://github.com/pulsar-edit/superstring/pull/1 is a fix for building pulsar on Node 18 an higher.

Though as far as I understand, the version of Node on the PATH when building Pulsar has no bearing on the final built Pulsar app. Just a "DevOps" concern, making it easier to build Pulsar on more machines.

(Probably relevant for the built Pulsar app if updated to use any Electron version based on NodeJS 18 or higher, though!!)

It probably doesn't work on Pulsar master before merging this either, as superstring from the npm package registry also doesn't have https://github.com/pulsar-edit/superstring/pull/1 in it. Hence why this commentary is either tangential/off-topic, or else very on-topic, depending on how you slice it. In any case, just a heads-up.

This PR doesn't change these things for pulsar core repo. More of a commentary for those who've been deep in code in the superstring repo itself lately, I guess.