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

Fix macOS binaries by vendorizing libiconv #1051

Merged
merged 3 commits into from
Jul 7, 2024

Conversation

savetheclocktower
Copy link
Contributor

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:

  • All macOS versions of superstring will be compiled with a known good version of libiconv. This should safeguard our builds from further macOS upgrades on the CI machines that automatically build our binaries.
  • During the build process of superstring, we tell the native module where to find our special version of libiconv in a way that will not anger macOS or make it show a scary “this app is unsigned” dialog (even though it actually is signed).
  • Both the superstring we consume as a direct dependency and the superstring that is a transitive dependency of text-buffer will point to the same place.

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 savetheclocktower marked this pull request as ready for review July 7, 2024 01:30
@savetheclocktower
Copy link
Contributor Author

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.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Would like to specify the exact SHA of superstring that we want in the package.json (and consequently in yarn.lock).

Reason being: Otherwise someone can stealth-update superstring if some adverse actor gets ahold of superstring repo, or as much likelier than that, one of us can make a bone-headed move and sabotage mainline Pulsar without intending to... As it'd be exclusively a change to yarn.lock if we don't specif and exact SHA... the diff of which file (yarn.lock) is often hidden in the github.com UI for PR reviews, and which people don't tend to review the content of regardless.

So, for that reason it's my understanding that "exact SHA so your dependency remains fully, immutably content-addressable" is the standard for git repo URLs in package.jsons in the project (or IMO should be -- I'm sure I've advocated for it before elsewhere besides this PR).

Not going on and on to beat the point to death, just wanting to explain the reason thoroughly.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Also, would prefer if the temporary commits were backed out entirely (force push removing the commit, rather than a revert commit), so the commit history of .cirrus.yml remains clean for blame and log purposes and such.

Although, for the record and posterity: Commit 98bc66b is looking clean and green in CI. Including the temporary ARM macOS run (Apple Silicon).

Not a huge deal, IMO, not really a blocker either.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

I can verify that the updated superstring dependency in package.json and yarn.lock is done correctly as intended.

And that the .cirrus.yml changes look good to me -- only removing outdated code for the prior attempt to get usable libiconv for superstring purposes, which is now obsoleted by the recent changes in our fork of superstring repo itself.

I have tested a built binary from CI myself, and have found that the zipped Pulsar.app binary worked on my machine (an intel Mac running macOS 10.15.7). I haven't fully gotten through building locally and testing that just yet, though yarn install works in CI and locally, at least, and I'm running the rest as I type this. The PR author and others in Discord have verified that Pulsar with the updated superstring works for them on recent macOS, including on intel and Apple Silicon (ARM) Macs.

All that said, looks good to me! Thanks.

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 7, 2024

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 pulsar-edit/superstring#1, I do believe building superstring (and by extension Pulsar) with Node 18 or higher on the PATH won't work, since pulsar-edit/superstring#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 pulsar-edit/superstring#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.

@savetheclocktower savetheclocktower merged commit 3c93cf0 into master Jul 7, 2024
103 checks passed
@savetheclocktower savetheclocktower deleted the vendorize-libiconv branch July 7, 2024 03:11
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