-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Vendorize libiconv
on macOS
#11
Vendorize libiconv
on macOS
#11
Conversation
…to match the version that `superstring` expects. This is copied directly from what Homebrew installs into `$(brew --prefix)/opt/libiconv`, except that the install name has been changed on `libiconv.2.dylib` to `libiconv.2.dylib`. (When it's installed from Homebrew, its macOS library “install name” matches its original path, so that's where macOS looks for it. As part of the build, we rename the library with `install_name_tool` so that it can be resolved relative to the location of `superstring.node`. This can probably be simplified, but first let's see if it'll work.
libiconv
on macOSlibiconv
on macOS
It'd certainly be nice to get CI working on macOS, but that's a pre-existing issue and is probably less urgent than our need to get binaries building again. |
Just realized that I've probably vendorized some things that would actually need to be built on a per-platform basis. We could get around this by continuing to rely on the presence of Homebrew I'll look at the Homebrew recipe and try to understand it a bit better. We could vendorize |
…because we need a version built for the current machine's architecture. (If our current approach doesn't work, we could vendorize two different copies — one for each architecture.)
…by grabbing it from Homebrew or from a path specified by the user in an environment variable.
I did a test PR on the I tested the Intel Mac binary and it worked fine; I say “seemingly” here because CI produces an unsigned build for PRs. I have no reason to doubt that code signing would work. @meadowsys is our guinea pig for Apple Silicon builds, so I'll update when I hear back from them. I did notice this in the CirrusCI build logs…
…and I believe it would've happened when we used But then the code signing of the app itself appeared to work properly, so I'm hoping that's all we need. The post-build Playwright smoke tests also ran smoothly. (Edit: and if we do need to re-sign the |
I can vouch that, on my macOS machine (macOS Catalina 10.15.7, intel x86_64 processor), the steps under the Testing section of the PR text above worked as the instructions said they should. As for getting CI working, I've got a couple of commits here to make a bit of progress on that: https://github.com/DeeDeeG/superstring/commits/472c6db1c05879cad9600a357fda1a2082bdf056 (Gets Python Would also need to modify the test matrix so Node 14 isn't tested on |
…by adding an alternate strategy that we can adopt if we ever need to. Also copy over `libiconv`’s README and license file.
I addressed some feedback on Discord by making sure we copy @meadowsys reported on Discord that the Apple Silicon binary works fine, which… was a bit surprising to me? I was pretty sure it was going to complain about the code signature on |
Anyway, here's how we could proceed:
At that point, it'd be great to use this whole ordeal as an excuse to revisit #9 and #10 — neither of which is crucial in the short-term, but both of which would be nice to address. I think it's a good idea to start inching closer to the parts of Pulsar that make us nervous: |
Closing in favor of #15. |
After a long struggle with lots of wrong turns, I think this is how we fix things.
The root problem
On macOS 13 (or 14; I've lost track now), the system version of
libiconv
switched from the GNU implementation to the FreeBSD implementation. There are API incompatibilities between the two; this presents a problem forsuperstring
, which previously was happy to rely on the ambientlibiconv.dylib
already present on the default library load path.When I first noticed this problem back in January, I didn't investigate the root cause; it wasn't affecting our binaries yet because we were pinned to older macOS versions in CI. I just knew that I could fix it locally (i.e., when running from source via
ATOM_DEV_RESOURCE_PATH
) by installing Homebrew's version oflibiconv
and making sure its paths were present in myLDFLAGS
andCPPFLAGS
environment variables.So when CirrusCI automatically upgraded our macOS images and our Apple Silicon builds started failing with the same
iconv
errors, I figured the solution was the same. We modified the CI job to runbrew install libiconv
, modifyLDFLAGS
andCPPFLAGS
, and proceed as usual. That was enough to get Pulsar to compile, so we thought the job was done.The second problem
Except we hadn't solved anything. We'd gotten Pulsar to compile by telling it about a dynamic library that was present on the CI machine; but all that did was lead Pulsar to assume that that library would be present on everyone's machine. So we ended up producing a version of Pulsar that would refuse to run unless you had
libiconv
present at/opt/local/opt/libiconv
. (That isn't even the standardbrew --prefix
, so a local Homebrew installation oflibiconv
would still probably fail this test.)The fix
A day's worth of flailing ensued, interrupted by occasional research. The most fruitful results were this blog post (which summarized the dilemma rather well and hinted at the eventual solution), followed by this blog post (exposing me to this strange
install_name_tool
utility and its ability to resolve relative paths).Then, the breakthrough: searching GitHub for ways that other libraries use
binding.gyp
andinstall_name_tool
. (That search query isn't working right now, so I'm glad it was working last night.)Here's the short version:
dylib
file, it is tagged with an ID that also happens to be a path on disk. This is the install name of the library. This is how macOS disambiguates various versions/copies of the same dynamic library.dylib
from a known location, you will confuse the binary that relies on it. It will complain that it can't find the library.DYLD_LIBRARY_PATH
to get around this, but the better way to do it is to useinstall_name_tool
to point that binary to a different install name (and thus a different path on disk).install_name_tool
to tag adylib
with a different ID./Applications
folder. Luckily, Apple added a few magic tokens that made it possible to define library locations relative to the things that request them.So our first task is to vendorize
libiconv
. We were already doing this for Windows; now we're doing it for macOS as well.Since the vendored files we need are platform-specific, we can copy the files we need from the user's Homebrew installation or from a path that the user provides via an environment variable. We do this as a pre-compile task on macOS only.
In our case, the thing that needs
libiconv.dylib
is the builtsuperstring.node
module. So as a post-compile task, we make sure that the module we just built knows where to find our vendorized copy oflibiconv.dylib
. The@loader_path
magic token is what tells it to search relative to itself.This should get
superstring
working on macOS again — not just compiling but also running in a self-contained manner. I've been testing this by temporarily moving the Homebrew version oflibiconv
to a different place on my local machine just to confirm it doesn't confusesuperstring
. And the effects ofinstall_name_tool
seem to survive a trip throughelectron-rebuild
, so this isn't even something that thepulsar
repo needs to be aware of.How we proceed
I'm fine with landing this however we want to. We can land it on
master
and tag a new release; we can land it on a branch; doesn't matter much. In the short term, the goal is to be able to modify Pulsar’spackage.json
to pointsuperstring
to a tag/SHA on our repo instead of a version number on NPM. If we do that — and modify theresolutions
so thattext-buffer
uses that same copy for itssuperstring
dependency — then everything should work, and we'll be able to build Apple Silicon binaries again on macOS.(The same problem would've eventually struck us on the Intel Mac side, too. This way we get out ahead of it.)
Testing
For sanity's sake, I'd love your help testing this no matter which platform you're on. If you're on Windows or Linux, it'd be great to know for sure that I haven't broken anything (though CI should help there).
Any macOS users can try the steps below. If you're on an Intel Mac… well, so am I, but it'd still be good to have another data point. If you're on Apple Silicon, I'm especially interested in your experience.
npm run build:node
.libiconv
, the script should halt the build and tell you why.libiconv
(brew install libiconv
), the script should copy its files into itsvendor/libiconv
directory and build against them.cd build/Release
otool -L superstring.node
cd ../../vendor/libiconv/lib
ls -l
libiconv.2.dylib
is present.