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

Avoid compiling native dependencies for common build targets #899

Closed
2 tasks done
jaecktec opened this issue Jul 27, 2022 · 29 comments · Fixed by #1098
Closed
2 tasks done

Avoid compiling native dependencies for common build targets #899

jaecktec opened this issue Jul 27, 2022 · 29 comments · Fixed by #1098

Comments

@jaecktec
Copy link

jaecktec commented Jul 27, 2022

Checklist

Before making a feature request, I have:

Feature description

When installing Pact the binaries should be downloaded through an optional platform specific dependency like esbuild

Use case

Due to recent supply chain attacks a lot of developers have npm scripts globally disabled.
Second we wouldn't need to care about proxy settings in the post-install.

@mefellows
Copy link
Member

The latest version of Pact JS (10.x.x) doesn't download anything at runtime, however it needs scripts enabled to compile native code. We previously had optional build dependencies (~4-5 years ago), this might be an option long term but is not something I'll be picking up any time soon.

If it were to be done, it would actually be in the pact-js-core project (https://github.com/pact-foundation/pact-js-core/).

@jaecktec
Copy link
Author

ah ok.
I was also checking if we could use rust + wasm to not have a binary at all.
But I think that's a big topic.

is the compilation of the native code required?

@mefellows
Copy link
Member

is the compilation of the native code required?

Yes, we have raw cpp files that link to the shared library. I believe we could look to precompile them and bundle or ship as optional dependencies. If that's something you'd be interested in contributing, we could definitely discuss further.

I was also checking if we could use rust + wasm to not have a binary at all.

That would be great, however unless I'm mistaken, wasm can't write files to disk or spin up mock servers - so I think it's a non starter.

@jaecktec
Copy link
Author

Wasm could export the matcher and plugin part. This would have the benefit that (in theory) all languages could use it and just the http part would need to be implemented in the respective language.

I can look into the pre compile topic. Issue here is that the pact library is surprisingly difficult to wrap the head around, so it might take some time for me 😅

@mefellows
Copy link
Member

Wasm could export the matcher and plugin part. This would have the benefit that (in theory) all languages could use it and just the http part would need to be implemented in the respective language.

Unfortunately there is a bit more to it than that. For example, there is also a sophisticated plugin framework that installs, discovers and communicates remotely to plugins.

I can look into the pre compile topic. Issue here is that the pact library is surprisingly difficult to wrap the head around, so it might take some time for me 😅

The key bit is in the pact-js-core. We would need to compile a version of the library for each platform we intend to deploy to (windows 64, linux 32/64/arm, mac 64/aarch64 etc.).

@riesel
Copy link

riesel commented Aug 3, 2022

I guess, you could use https://github.com/mapbox/node-pre-gyp to distribute the binaries.

@jaecktec
Copy link
Author

jaecktec commented Aug 4, 2022

but that still means, that we have to have a post install hook

@mefellows
Copy link
Member

I guess, you could use https://github.com/mapbox/node-pre-gyp to distribute the binaries.

That's right. I'd prefer to explore the optional dependencies approach if it could be done, but that won't work on platforms like alpine because the os/architecture combination is not enough to determine the runtime needs (for clarity, we don't officially support Alpine because dynamic linking to glibc is challenging.

@d-koppenhagen
Copy link

Hey, probably it would be good to deliver some pre-build Docker-Images containing the correct built binaries. What do you think about this? In consequence, the Images versions should always be aligned with the version of pact-js, so consumers can align their image and package.json dependency.

@mefellows
Copy link
Member

Is the docker images for building the pact JS versions, or for users to run their Pact JS builds on?

@jaecktec
Copy link
Author

I can't figure out how to get node-gyp to cross-compile the binaries 😖. If it's not possible I could create a github action and run it for every major architecture 🤔

@mefellows
Copy link
Member

Yeah, I think that is probably what is going to be required. I'm almost certain you won't be able to cross-compile all future targets such Alpine, for example. In fact, I'm not sure how you'd even be able to detect what alpine package to install at runtime as the package constraints aren't that granular - so that almost certainly would have to be a post install script.

Alpine aside, it gets tricky though, because you need to only publish the top level package if all of the optional dependencies publish successfully.

I think tools like https://github.com/semantic-release/semantic-release can help with that kind of process, but it would require substantial changes to our release process (I'm not against it, but it's going to be an involved process to setup a test set of packages first to demonstrate the workflow on a test npm package).

@jaecktec
Copy link
Author

is there a difference between alpine and 'standard' linux? esbuild seems to also not make a difference

@mefellows
Copy link
Member

mefellows commented Sep 27, 2022 via email

@mefellows mefellows changed the title Download Pact-Binary through optional dependency Avoid compiling native dependencies for common build targets Oct 27, 2022
@candrews
Copy link

because dynamic linking to glibc is challenging.

Why not static link instead to avoid the problem?

@mefellows
Copy link
Member

I think it's plausible, albeit there is probably a reason we didn't do this earlier (despite the size of the lib being 3x bigger). We do release static artifacts (see https://github.com/pact-foundation/pact-reference/releases/tag/libpact_ffi-v0.3.19), so if somebody wanted to try I'd be more than open to accepting a PR (as noted, it would need to be done here https://github.com/pact-foundation/pact-js-core/).

@andreasmarkussen
Copy link
Contributor

This worked in on Windows 10 and Node 18
on Pact.JS v9.18.1 - and It has been broken in Pact JS v.10.x

I would love to help out, I just need to understand what changed?

@mefellows
Copy link
Member

Thanks @andreasmarkussen, a lot has changed since that version.

TL;DR the current situation:

  1. We distribute a "shared core" containing the majority of important functionality for use across all Pact SDKs as a set of native C (FFI) functions exposed from a shared library, created in https://github.com/pact-foundation/pact-reference/
  2. @pact-foundation/pact-core wraps that shared core using the Node N-API interface. This uses Node Gyp to compile the right target at runtime (i.e. install)
  3. @pact-foundation/pact uses the shared core, exposing the test interface most users will actually use

If we want to avoid compilation at install time, there are several options all with their own set of tradeoffs:

  1. Expose as optional dependencies (see above). This still suffers the issue of requiring install hooks for some architectures, because the os/arch targets node exposes is not enough to detect all environments (e.g. alpine). It probably will work the best for most standard combinations - linux, windows, mac on 64 bit and arm architectures. The other issue with this approach, is that if the optional dependency doesn't install we need a fallback mechanism or to properly detect and error out - hard to do without access to install scripts. It's called an optional dependency for a reason!
  2. Pre-build all of the dependencies and upload to a known location (e.g. GH release artifacts) and use node pre-gyp to fetch them (Avoid compiling native dependencies for common build targets #899 (comment)). Still requires installation hooks
  3. Pre-build all known dependencies and embed them in the package. We actually already distribute all of the necessary shared libraries here, so it would just be a matter of pre-compiling the OS/Arch specific targets and finding a way to load it at runtime (likely at the point we load the module here. We may also be able to use the bindings lib to automatically search for us: Feature request: multiple installed bindings TooTallNate/node-bindings#11)

(3) seems the most promising approach to me and I recommend we start there. The hard part here is finding a way to x-compile the targets and get them into the distribution, possibly spread across a number of GH actions (see this suggestion above)

A reminder - the above would need to happen in the @pact-foundation/pact-core package.

I think we would want to aim for an initial compatibility table as follows;

OS/Arch support

OS AMD 64 arm
Linux (glibc)
Mac
Windows
Linux (alpine) 🔸 🔸

✅ = installs without build hooks, pre-built native library
🔸 = works, however, requires build hooks / runtime compilation or installation
❌ = not supported

NOTE: the core does not release arm artifacts for windows, but there is no reason we couldn't support that if they did.

@mefellows mefellows pinned this issue Mar 9, 2023
@YOU54F
Copy link
Member

YOU54F commented Apr 20, 2023

@jaecktec how do those who use ignore scripts actually work with npm properly as a large amount of packages have pre install and post install scripts.

i don’t assume you are checking the full contents of every top level dependency you have in your project and the pact ffi as a shared lib brings in its own set of dependencies and attack vectors

@jaecktec
Copy link
Author

interestingly pact is the only library that I've noticed actually requiring the post-install in all our products.
But for now I've started to use @lavamoat/allow-scripts to manage the post-install hooks.

@YOU54F
Copy link
Member

YOU54F commented Apr 20, 2023

node pre gyp demo in pact js core

pact-foundation/pact-js-core@master...YOU54F:pact-js-core:master

all the prebuilt node binaries

https://github.com/YOU54F/pact-js-core/releases/tag/v13.16.1

  • generates pact.nodes for multiple oses/arches and node versions
  • doesn’t package the standalone and ffi files into the npm package
  • will fall back to build chain if no github release tarball found but will fail as the shared libs aren’t in the npm package ( can be dealt with in a no dependency pre install script )
  • will install but fail to run if ignore-scripts if set, we could detect at run time, warn the user and then download the pre requisite files or tell them where to get the files for their platform

bonus points - i’ve built pact.node for arm64 windows machines too, your mileage may vary

@YOU54F
Copy link
Member

YOU54F commented Apr 20, 2023

interestingly pact is the only library that I've noticed actually requiring the post-install in all our products.
But for now I've started to use @lavamoat/allow-scripts to manage the post-install hooks.

thanks buddy, that is useful to know! :)

@YOU54F
Copy link
Member

YOU54F commented Apr 20, 2023

if no-one is concerned about the size of the npm package, then pre compiling all the pact.nice binaries and including them in the package would be the simplest as it doesn’t involve any install time fetching or building.

i’m not sure on the versioning node pre gyp provides, for node. it shows node versions 93/111 etc but i think it should be against node api versions which have 1-8

also in doing this exercise it’s makes me definitely think pulling out the ruby standalone into a sep package is a smart idea, as saves having to store the standalone in the release tarball and users can just add it in as an extra import if they want

@mefellows
Copy link
Member

Ah, that's a good point. I believe the node ABI is forwards compatible, so if we built the pact.node dynamic library on the lowest version of Node in our support list, it should work on later ones. I'd need to look that up though.

@YOU54F
Copy link
Member

YOU54F commented Apr 21, 2023

So I was trying to work it out last night

ABI Matrix

https://nodejs.org/api/n-api.html#node-api-version-matrix

Screenshot 2023-04-21 at 12 43 10

https://nodejs.org/en/download/releases

Screenshot 2023-04-21 at 12 42 50

[1]: NODE_MODULE_VERSION refers to the ABI (application binary interface) version number of Node.js, used to determine which versions of Node.js compiled C++ add-on binaries can be loaded in to without needing to be re-compiled. It used to be stored as hex value in earlier versions, but is now represented as an integer.

We can see version in NAPI_VERSION=3 here

https://github.com/YOU54F/pact-js-core/blob/master/native/ffi.cc#L1

anything lower and it fails to build.

Looking at the ABI compat matrix, version 8 supports v12 which is EOL, v14 which is going EOL end of the month, but raises minimum compat up to v15.12.0 (for v15 users)

I'm not sure what the implications are of leaving it at the lowest version it will compile with (which is NAPI_VERSION=3).

I also don't understand why that chart isn't updated, to include v17->v20, especially as that matrix for the node-api version matrix is on the node v20 ref docs

@mefellows
Copy link
Member

Thanks for sharing. We could probably update to 8 if I'm reading that table correctly, given that 14 (our lowest major version officially supported) works with it. I'm not sure what we gain from it, so it might be worth looking into after we first get it all running (or we just test to see what happens).

@YOU54F
Copy link
Member

YOU54F commented Apr 21, 2023

Yeah just best spinning it through a set of docker containers on diff node versions and seeing what happens,

As well as node-pre-gyp, there are also another couple of options, as recommended by node

https://nodejs.org/api/n-api.html#uploading-precompiled-binaries

https://github.com/prebuild/prebuildify/
https://github.com/prebuild/prebuild

These will look up via the detected node/arch/abi version and look for a pre-compiled version that you ship with the package (for multi arches)

Screenshot 2023-04-21 at 13 23 39

npx prebuildify --napi --arch x64+arm64 - this apparently produces a fat binary, that should support both, not tested it yet

npx prebuildify --napi --arch x64

@mefellows
Copy link
Member

mefellows commented Apr 21, 2023 via email

@github-actions
Copy link

👋 Hi! The 'smartbear-supported' label has just been added to this issue, which will create an internal tracking ticket in PactFlow's Jira (PACT-963). We will use this to prioritise and assign a team member to this task. All activity will be public on this ticket. For now, sit tight and we'll update this ticket once we have more information on the next steps.

See our documentation for more information.

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 a pull request may close this issue.

7 participants