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

Candidate for new master #15

Merged
merged 8 commits into from
Jul 6, 2024

Conversation

savetheclocktower
Copy link

This PR proves that the state of #13 is identical to what I've done here: keep all commits, except explicitly revert the parts of #1 that change the source code.

I have more faith that this will merge cleanly into master than #13 — which I made by starting from an earlier commit and cherry-picking everything except for #1.

@mauricioszabo, my reasoning here is that

  • we proved that these code changes were (somehow) the source of renderer process crashes in Pulsar while running package and editor tests in CI;
  • as I understand it, the changes in Node API upgrade to V8 >= 8 #1 don't do much on their own to achieve compatibility with newer Electron and newer V8 — we'd want the more comprehensive N-API stuff present in Migrate to N-API #10.

Once we merge this, we can point Pulsar's package.json to our copy of superstring for the first time — as has been made necessary by the fact that we must explicitly build our own copy of libiconv on macOS.

* Remove scripts we don't need.
* Report progress better on fetching/compiling `libiconv`.
* Preserve the license and README from `libiconv`.
The latest macOS GitHub Actions CI runners ship with a Python
that is configured as "This environment is externally managed".

This is a somewhat recent thing in the Python world, see:
- https://packaging.python.org/en/latest/specifications/externally-managed-environments/
- https://peps.python.org/pep-0668/

We can try using virtual environments ("venvs") to manage
manual package installs as the error message suggests.
We need setuptools for node-gyp to be able to do its thing
macOS 14 (`macos-latest`, `macos-14`) runners are implicitly
Apple Silicon runners, at least for runners we have access to
on the free tier.

Node 14 isn't built for Apple Silicon.

So, we can test Node 14 on older macOS, like... macOS 13.
That will get us an x64 macOS runner, an arch for which Node 14
should indeed be available.
@savetheclocktower
Copy link
Author

Also happy to adopt the CI stuff recently suggested by @DeeDeeG, though I'm happy to back it out again if they think it should be part of a separate PR.

Copy link

@mauricioszabo mauricioszabo left a comment

Choose a reason for hiding this comment

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

Let's merge this, sure!

Indeed, it's our best course of action to just try the N-API version instead.

@savetheclocktower
Copy link
Author

savetheclocktower commented Jul 6, 2024

Failures on Node 18 (on all platforms) are not necessarily expected, but not surprising either.

Failures on macOS on Node 14/16 are acceptable — two specs are failing on an obscure encoding.

Landing this! (Would land this if I had a green button to click.)

@DeeDeeG
Copy link
Member

DeeDeeG commented Jul 6, 2024

I'm fairly confident the Node 18 failures are from reverting the code we cherry-picked from NAN, unfortunately :/.

But this is fine for now. Progress in current Pulsar is good, doing custom stuff on separate branches for WIP branches of Pulsar is still an option.

EDIT to clarify: I'm referring to the code from #1.

@DeeDeeG DeeDeeG merged commit de97b49 into pulsar-edit:master Jul 6, 2024
5 of 10 checks passed
@savetheclocktower savetheclocktower deleted the candidate-2 branch July 7, 2024 00:51
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.

3 participants