-
Notifications
You must be signed in to change notification settings - Fork 665
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
RFC: prebuilt addon dependencies #1114
Comments
It fails in a Maybe take a look at how node-ffmpeg-installer handles their prebuilt binaries? Also, I'd suggest linking here from #1083. Many people will land there directly after a search and won't otherwise see your request. |
Can you elaborate a bit on the error? Is it failing on the optional dependency or the addon bundled with Unfortunately |
I tried this with Node.js 14.18.1 + npm v6.14.15 on macOS 12.1 (Monterey) on an Apple Silicon MacBook Pro, with this result:
I assume |
Sure, I ran
Then tried the install:
|
With Node v16.13.1 + npm v8.1.2 (on macOS 12.1 (Monterey) on an Apple Silicon MacBook Pro) the npm logs are a bit more cryptic, can’t really tell if it’s successfully using a prebuilt binary, but the tests pass. |
I didn't write up enough detail on this either, sorry. I haven't actually looked at either implementation myself, just mentioned that installer because ffmpeg also cares about hardware features and so thought their approach might be worth investigating. Along similar lines of rough ideas/suggestions, a fallback idea comes to mind. Something to consider if the build headaches get unpleasant. You could use an explicit opt-in approach where someone npm installs the plugin externally themselves. Then either it gets picked up using feature detection or the dev passes it in directly (like |
That's the bundled addon, so that makes sense. npm 7 or 8 started making optional dependency install scripts silent unless there's a failure, so it appears It would be interesting to see if it found a compatible binary or if it built from source. Not sure if adding
That's not really an ideal approach as I think most people wouldn't know to do that or would want to go through the extra effort. Having npm pull it in as an optional dependency is the ideal mechanism and is what we're currently using (and doing detection at runtime as you mention). The whole point to having this dependency at all is to make the default handshake parameters be more optimal "out of the box" depending on the CPU and its features.
It's good to see it fell back to building and that worked. Does |
It does, (I just checked, on my Intel Mac, it also returns |
Yeah. I wonder if it's possible to stuff that part of the install into a child process? Then filter/modify the actual output. |
Windows 11 Logs attached |
Ok, I see the bug with getting the macOS version. I will fix that soon.
I'm not sure what you mean here. What I was saying is npm is already doing this, as we
Technically (especially since the "build" process is now greatly expanded) we could always hide the output and always return zero so that nothing ever gets seen, this would work for all npm versions. However, my concern is the lack of any messaging. With that kind of setup, during installation you won't know whether dependencies will be available or not. The alternative is to display something or emit a warning using the standard node.js core mechanism for doing so, but I think that would annoy people even more. Currently the only way to know if the default handshake parameters will be optimized out of the box is by looking at debug output, since I do include that information there. |
A couple of interesting issues there. Thanks. I will probably add a flag for printing environment/debug information to help identify why no compatible binary was found, etc. |
Filtering not hiding. There will be ongoing failure cases: new hardware and OS releases, build dependencies missing in environment, etc. I'd suggest tuning the messaging, with no stacktrace displayed in stdout/stderr. Eg: "Unable to install or build optional 'cpu-features' ssh2 dependency. This can have a significant impact on performance, see /tmp/ssh2_install_error/ssh2_cpuFeatures_optionalInstallError.20220106abc123.log for details. More information at https://github.com/mscdex/ssh2/wiki/cpu-features" |
The mechanism being used here ( If we want to display anything we need to intentionally fail the install script, otherwise npm v8 users will never see anything. That's not exactly ideal as it's a workaround, but the real problem is figuring out what to display, especially when falling back to compilation and it fails. [aside] |
I strongly recommend not inventing your own native dependency install library ( I would suggest looking at prebuildify. I don't know if it does everything you want, so it might not be a replacement, but the way that tool bundles all the prebuilt binaries inside of the npm package would negate the need to invent and maintain a new install tool, with the added benefit of faster installs (no uncached http request to fetch the native binary) at the cost of a couple of mb more disk space |
Too late.
I'm not sure what you're describing here exactly. As far as Electron support goes, there should be no difference between prebuildify and build/install-addons. They'd both generate binaries for Electron and then either use a compatible binary or fall back to building if none exist.
I won't go into all of the reasons why existing solutions did not work, but specifically comparing with prebuildify here are a few reasons why I didn't use it (through the lens of using build-addons/install-addons generally, not specific to
Believe me, the last thing I wanted to do was create my own binary generator and consumer, but after looking at all of the currently available solutions, nothing met all of my criteria. Ideally it would be great to have some of this logic (e.g. the |
So, I can run If you don't even have a bindings.gyp in your repository then expect the tools to not even realise it is a native library, which will likely break electron builds for the current environment as it wont know to do anything to it.
Yeah, its a balancing act of install speed vs disk space. The prebuildify approach may not be the right fit for
Yes, I agree these are pain point of most of the tools I have looked at.
In my experience with a few other native libraries, most of these have never come up as an issue that requires different builds. But maybe cpu-features is different and needs to hook into many more places in the os. I cant answer that, but this has been my experience with other projects.
I agree. Ive read at least one proposal for support for this, but I havent seen anything happen on this front yet |
Fails on Windows with Node 18.0.0, Visual Studio 2022 if that matters
|
@jeffrson Those errors are unrelated and are already fixed in the current version of |
Okay. You've seen that "Crypto binding not available"? |
@jeffrson That just means the bundled addon failed to build during |
@mscdex How are things going with the prebuilt testing? Any idea on when this might get merged into a mainline release? |
@dlong500 I haven't touched it in a long while because of various roadblocks. At this point I'm not optimistic that something like this is very feasible given the current state of node, package managers, and other pieces that need to come together to make prebuilt addon installations as smooth as possible (or in some cases, work at all). Maybe at some point in the future things will be different for the better, but until then building from scratch is really the best we have unfortunately. |
I'm going to go ahead and close this because unfortunately the current node.js package management ecosystem makes what I wanted to achieve impossible. Maybe some day in the future that will no longer be the case. Thank you to everyone who participated in my little addon experiment. |
I'm working on making some addon dependencies that provide prebuilt versions to avoid the need for a build environment. So far I'm only testing this with
cpu-features
. If the entire process (including prebuilt addons working on the systems they were designed for and falling back to building from source) proves to be reliable, I will merge the necessary changes into the master branch.I've tested the experimental branch on a few different systems of various configurations with success, but it would be great if others could test it as well, especially on platforms like mac and Windows (as I don't own such systems). All that's needed is to
npm install mscdex/ssh2#prebuilt
and thennpm test
from within the resulting node_modules/ssh2 directory. That should be a good test to make sure everything is working as expected.The text was updated successfully, but these errors were encountered: