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

Implement automatic publishing to NPM #16

Merged
merged 20 commits into from
Feb 3, 2025

Conversation

confused-Techie
Copy link
Member

This PR follows the same form used over in pulsar-edit/less-cache#7.
The only differences being:

  • I had to match the installation method used here already (such as installing python)
  • I had to create a lock file.

More on the lock file:
Using npm ci instead of npm install is recommended during publication, as it will make no attempt to bump any dependencies in your lock file, which means you can be more confident in it's functionality. Thing is, this repo didn't yet contain a lock file, and on my machine I was unable to get all of the dependencies happy enough to actually create one locally. So I had to rely on CI to do it for me. This does add some churn to my commits, but should have produced a successful and functioning lock file for us to use.

@savetheclocktower
Copy link

OK, so the CI failures here are interesting.

The current version of superstring works on Node 14 and (apparently) 16, but not on Node 18 because of API changes. That's fine; we can disable those tests for now, since Electron 12 ships with Node 14.

Separate from that, the tests are failing altogether on Mac because of an apparent inability to recognize the CP437 character encoding; I presume this is coming from libiconv. Code page 437 is an archaic character encoding that has little relevance except that I think it might still be the default character encoding of the command prompt on Windows. I don't know why this test fails on Mac, but it's not a new thing (not since July when we went through all the superstring macOS drama), so we could skip that test on macOS.

@savetheclocktower
Copy link

I've discovered that CP437 support on macOS can be added with an extra switch on line 63 of script/fetch-libiconv-61.sh:

  ./configure --enable-extra-encodings --prefix="$EXT" --libdir="$EXT/lib"

That was an oversight from July. Please make that change on this PR, then remove Node 18 from the matrix, and then CI should pass.

We might also need to remove the prepublishOnly line from the package.json — it tries to build the WASM version of superstring, but that only works if you install Emscripten, and that's something that was only part of the last CI workflow, as you can see from .circleci/config.yml. I think we can lose that path altogether.

Once that happens, I think we'll achieve what we want with this one — no building taking place on publish, then an implicit node-gyp rebuild taking place on an npm install (as described here) — resulting in automatic building of both the native bindings and libiconv.

If there were anyone else consuming this but us, I'd want to handle the downloading and installation of libiconv differently somehow — checking a universal .dylib into source control, or prebuilding for x86_64 and arm64… but the current setup works, so it's not worth it.

@confused-Techie
Copy link
Member Author

Thanks a ton for the review @savetheclocktower
I'll go ahead and implement those changes so we can see some green CI in this repo

API Changes result in breaking changes for this repository. But we also are not using it on NodeJS v18 yet, so we can just remove those tests
@confused-Techie
Copy link
Member Author

@savetheclocktower We are still seeing failures here in macOS.
I wonder if we can't just recreate the environment we are using over on flagship Pulsar for installation of these dependencies.

Or I wonder if @DeeDeeG might have any insight into what's gone wrong here.

Since it seems macOS is still reporting a lack of distutils even though we are installing setuptools

@savetheclocktower
Copy link

savetheclocktower commented Feb 1, 2025

I suspect npm just needs to know the location of Python — either by setting the PYTHON environment variable or doing npm config set python "[path to python]". It might be defaulting to python in the path, which is perhaps Python 2 on this image.

@savetheclocktower
Copy link

savetheclocktower commented Feb 1, 2025

You might also try introducing the actions/setup-python step from ci.yml, since that might do the necessary work for you. EDIT: Never mind, that's only used in ci.yml if the Node version is 14. Still trying to understand why this problem doesn't happen on master’s CI jobs.

@savetheclocktower
Copy link

This line from the CI output is telling:

gyp info find Python using Python version 3.13.1 found at "/opt/homebrew/opt/[email protected]/bin/python3.13"

It should be finding the bin/python3 defined in the venv.

@savetheclocktower
Copy link

I ran into this latest error on my fork. Bear with me while I try something to see if it resolves this problem; I'll open a new PR against this one.

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 2, 2025

macos-latest GitHub Actions runner is Apple Silicon at this point. I know we're not doing that at core repo as of yet. I'm not super familiar with ARM Macs and don't have access to one at the moment.

@savetheclocktower
Copy link

macos-latest GitHub Actions runner is Apple Silicon at this point. I know we're not doing that at core repo as of yet. I'm not super familiar with ARM Macs and don't have access to one at the moment.

Yeah, I can't explain why the tests are going bad on my machine but not on an Apple Silicon runner — but at least I now know I can fix it if it eventually goes wrong in CI.

@DeeDeeG
Copy link
Member

DeeDeeG commented Feb 2, 2025

FWIW, I did an npm install and npm pack locally, and the resulting .tar.gz looks reasonable, and I can move it to a folder and do npm init -y followed by npm install ./[superstring something something].tar.gz and it appears to install and build correctly. So I can say that much, at least!

(This simulates the "payload" tarball we would be uploading to the npm registry for the package.)

@savetheclocktower
Copy link

@confused-Techie will update this PR shortly with the stuff I had to do to get CI to pass. For posterity, here's a rough description of those changes.

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.

Looks good to me, I guess?!

The catch stuff seems kinda wild, but assuming it's in service of us having unit tests that still work, I'm all for it.

The main change, to publish to npm on GitHub Release creation, I trust the intent/work others have done to get here and am merely signing off on it.

CI passes now, which is certainly nice!

👍

@savetheclocktower savetheclocktower merged commit a79ffbd into pulsar-edit:master Feb 3, 2025
7 checks passed
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