Feat: Prebuildify pact.node for all FFI supported platform/architectures #446
Conversation
.gitignore
Outdated
| @@ -58,7 +58,9 @@ standalone/* | |||
| !standalone/*.ts | |||
| standalone/*.d.ts | |||
| ffi/* | |||
| !src/ffi | |||
There was a problem hiding this comment.
This doesn't seem right. If the intention is to stop the line above matching in /src/ think it's better to change the line above it to:
/ffi/*
There was a problem hiding this comment.
this actually works as intended, however cirrus cli wasn’t honouring the git ignore excludes so when it mounted my workspace it was sans src/ffi
will remove
There was a problem hiding this comment.
Either way, I think the ffi/* was definitely meant to be /ffi/*
|
just a note to the reader i still need to get the generated prebuilds from each of the os arch runs and consolidate them so they can be packaged up for a release. the github action run will generate the x64 bindings for macos windows and linux. ( via seperate runs ), these are then uploaded as artefacts and downloaded in another job to show the prebuilds working cirrrus is used to generate the bindings for arm macos and linux. we can download workflow artefacts from either runs as they upload the packages on each ci run. just needs a bit of thought as to how we pull them together for packaging time. |
main gist of the node gyp changes
i’m actually wondering if we need node-gyp or node-addon-api in our dependencies, i think the only reason we would use node gyp is to do a platform look up to pick the right binary ( which we could do easily ) node-addon-api appears to be needed when building the pact bindings, but again not sure if it’s a run time dep. will do some investigation :) ( read that as remove things till it breaks 😂 ) |
Node Gyp is an install time dependency. It only gets used in the So we could try removing it for the general users (obviously it is needed for other steps in the build process0.
💯 |
|
Hey hey So just an update on this I've got a decent workflow worked out for packaging and releasing, The diff is here master...YOU54F:pact-js-core:master Published to npm on my fork, you can see the prebuilds and pact.node here in the code tab https://www.npmjs.com/package/@you54f/pact-core?activeTab=code One irksome thing I found, is that release versions take that on the commit message only, so if you have a fix, after a feat, you lose the feat version bump. You don't get warned on it, just warned to ensure you have a fix/feat. That seems odd, it's bitten me in the past. I would expect to see it computed based on the last release and the delta of commit conventions that go into a changelog. I added some additional path looks and ways for users to help themselves if they get stuck. I'll write more detail up this week but just thought I'd drop in where I was up to Build and test workflow https://github.com/YOU54F/pact-js-core/actions/runs/5099788056 publish workflow https://github.com/YOU54F/pact-js-core/actions/runs/5099802203 Releases published to GH https://github.com/YOU54F/pact-js-core/releases/tag/v13.19.0 |
This shouldn’t happen. How are you generating releases? They should include all commits (fix, feat, fix!, feat!) since the last tag |
|
Ahh its because I was testing the flow out with dry run, so you dont have committed tags. so
I assume if after + feat, a commit and tag took place, I would get a bump from 13.14.0 -> 13.14.1 It's actually a random by product of the fact I have duplicate version numbers on npm, of the version I was bumping too with a feat was taken, because of previous changes that I had self released. I wanted to get to something that was 13.14.x but not 13.14.0 because I also have a 13.15.x and other versions released when changes out in the past |
|
The version numbers bump to the highest of all the things: so: 13.13.8 with 3 commits of fix,fix,fix would release 13.13.9 Any combination that includes at least one feat commit would be 13.14.0 Any breaking changes (feat!, fix!, refactor!, etc) would release 14.0.0. |
|
Just about to force push some new changes, documented below 📝 Summary of ChangesChanges Implemented in this pull request:
|
This change pre-builds the pact node bindings with prebuildify
Supported node versions are node LTS versions 16, 18, 20.
Supported Platforms + Arches are now
* Linux ARM64/X64
* MacOS ARM64/X64
* Windows X64
For the Pact FFI.
This change additional includes the Pact-Ruby-Standalone updated to v2.x
This brings is support for
* Linux/MacOS ARM64
for the pact cli and api
## Requirements
Users are no longer required to
* Install a node-gyp build chain
- Not limited to
- Python
- MSVS for Windows
* Ensure `--ignore-scripts` is not set to true
- This is because pact-js-core and the npm package pact-core will now come
batteries included, which means the ffi and standalone come packaged in the
npm package. This increases size, however all files have to be downloaded currently
even if all are not used, so we might as well do this once for the user, rather than
every user needing to, and possibly struggling due to various build system reasons related
to node-gyp.
.cirrus.yml
Outdated
| install_script: | ||
| - apt update --yes && apt install --yes curl python3 make build-essential g++ unzip zip | ||
| << : *BUILD_TEST_TASK_TEMPLATE | ||
| GITHUB_TOKEN: ENCRYPTED[636f316600928de28b5c36027cc39d9796bc0d0eca2a181368f255ad61540f13bb38cdce09b6428774f315bbf45e0ada] |
There was a problem hiding this comment.
Note - Pact-Foundation Cirrus CI account will need a scoped GITHUB_TOKEN
There was a problem hiding this comment.
What is needed here sorry Saf - do we need to bake a new one with scoped permissions instead of (presumably) wider perms here?
There was a problem hiding this comment.
hiya,
so currently this is using a PAT under my own user scope, which has been attached to my pact js core fork in cirrus ci.
it creates an encrypted secret
https://cirrus-ci.org/guide/writing-tasks/#encrypted-variables
which is usable at the repo level.
the token can only be encrypted by the associated repo and if the user triggering has suitable perms.
this will mean for merging
- will need a scoped PAT creating in pact foundation org, and added to our vault that Beth setup.
- will need the scoped PAT linked in pact-js-core cirrus config ( under pact foundation org not my fork )
token is required perms to
- list releases
- upload assets to releases
|
I tried taking your published package from npm for a spin: A few things:
Log Levels
Is it expected for users to set that? Also the naming is a little confusing relative to the message ("unsupported platform" is the variable which would appear to override the current detected os?). I imagine this should be a
This should be
This should be I'll keep digging to see if I can work out why it's failing... |
|
So far as I can tell from the PR |
src/ffi/index.ts
Outdated
| `Attempting to find pact native module ${loadPathMessage(bindingPath)}` | ||
| ); | ||
| ffiLib = bindingsResolver(bindingPath); | ||
| if (ffiLib) { |
There was a problem hiding this comment.
Ah! I think this is why it's failing - should this be:
| if (ffiLib) { | |
| if (!ffiLib) { |
At the moment, it seems to be finding the package and then throwing an error!
src/ffi/index.ts
Outdated
| logger.warn(`Failed to load native module from ${bindingPath}: ${error}`); | ||
| } | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
This section isn't currently being caught, at least in the case that the bindings library can't find the node lib. The reason is that on line 90, the error is caught there and thus is swallowed, both preventing the subsequent log from logging, and a non-zero exit status.
src/ffi/index.ts
Outdated
| logger.info(detectedMessage); | ||
| if ( | ||
| !supportedPlatforms.includes(platform) && | ||
| process.env['UNSUPPORTED_PLATFORM'] !== platform |
There was a problem hiding this comment.
I can't see where UNSUPPORTED_PLATFORM is actually used, outside of notices?
There was a problem hiding this comment.
It was, but removed, I tried switching to natively looking up the bindings rather than using another package, which would give us finer grained control over the platform/arch combos, but you have to deal with modules with being loaded sync or async and it was a mission, sooo, they can just raise an issue and we can build ffi bindings in pact-reference if possible :)
test/consumer.integration.spec.ts
Outdated
| @@ -39,7 +44,7 @@ describe('FFI integration test for the HTTP Consumer API', () => { | |||
| value, | |||
| }); | |||
|
|
|||
| describe('with JSON data', () => { | |||
| describe.skip('with JSON data', () => { | |||
There was a problem hiding this comment.
Accident? I'd hope the JSON one works 😬
|
OK! I got a bit further now and I think it's really close - looks like just a single line was responsible for the package not installing on my machine. Had a few additional minor comments also for looking at after you return from your well-deserved break! |
yep will remove, I wanted to be able to allow users to set their own platform and arch but can't whilst using |
Appreciate the eyes dude! |
Yep 110% have removed |
|
Right this should with any luck be good to go, I've simplified the logic a bit and allowed for
I've also now added a scoped pat token, against pact-foundation org, scoped to pact-js-core, into cirrus-ci :) it's valid for a year, and I will add it to our vault |
|
So I'm going to merge this, but I'm not going to squash the commits. There are 3 |
|
The release is failing because it thinks the release notes are empty. I've put some debugging in a branch, and can see a discrepancy between CI and local (which looks correct): CI Local Continuing to investigate. |
|
Thanks to @TimothyJones, spotting the need for From https://github.com/actions/checkout
|
|
There seems to be an issue with the release (tested with Pact JS) so I have deprecated the package (this should mean it won't be pulled in as a transitive dependency). I'll do some more testing locally, hopefully it's something simple (the prebuilds are definitely in the released package) |
|
Woohoo, Right two issues
simple test of the standalone and ffi const pact = require('@pact-foundation/pact-core')
pact.createServer()
.delete()
.then(function() {
console.log('pact ruby standalone server created and deleted')
});
const consumerPact = pact.makeConsumerPact(
'foo-consumer',
'bar-provider',
4 ,// v3 spec,
'TRACE'
);
const interaction = consumerPact.newInteraction('some description');
interaction.uponReceiving('a request to get a dog');
interaction.given('fido exists');
interaction.withRequest('GET', '/dogs/1234');
interaction.withResponseBody(
JSON.stringify({
name: 'fido',
age: 23,
alive: true,
}),
'application/json'
);
interaction.withStatus(200);
port = consumerPact.createMockServer('127.0.0.1');
// port = consumerPact.createMockServer('localhost'); // Error in native callback - JSON.parse(ffi.pactffiMockServerMismatches(port))
console.log(port)
consumerPact.mockServerMatchedSuccessfully(port)
const mismatches = consumerPact.mockServerMismatches(port);
console.log(mismatches)
You can see the lookup path is [11:46:08.471] DEBUG (10503): [email protected]: Attempting to find pact native module : loading native module from:
- /Users/yousaf.nabi/dev/fooscratch/prebuilds/darwin-arm64
source: pact-js-core binding lookup
- You can override via PACT_PREBUILD_LOCATION
[11:46:08.473] DEBUG (10503): [email protected]: Supported platforms are:
- darwin-arm64
- darwin-x64
- linux-arm64
- linux-x64
- win32-x64
We detected your platform as:
- darwin-arm64
[11:46:08.474] ERROR (10503): [email protected]: Failed to initialise native module for darwin-arm64: Error: No native build was found for platform=darwin arch=arm64 runtime=node abi=93 uv=1 armv=8 libc=glibc node=16.20.0
loaded from: /Users/yousaf.nabi/dev/fooscratch
[11:46:08.474] DEBUG (10503): [email protected]: {supportedPlatformsMessage}
We detected your platform as:
- darwin-arm64
We looked for a supported build in this location /Users/yousaf.nabi/dev/fooscratch/prebuilds/darwin-arm64
Tip: check there is a prebuild for darwin-arm64
check the prebuild exists at the path: /Users/yousaf.nabi/dev/fooscratch/prebuilds/darwin-arm64
Wrong Path?: set the load path with PACT_PREBUILD_LOCATION ensuring that $PACT_NODE_NAPI_LOCATION/prebuilds/darwin-arm64 exists
- Note: You dont need to include the prebuilds/darwin-arm64 part of the path, just the parent directory
- Let us know: We can add more supported path lookups easily, chat to us on slack or raise an issue on github
[11:46:08.474] INFO (10503): [email protected]: Removing all Pact servers.
/Users/yousaf.nabi/dev/fooscratch/node_modules/@pact-foundation/pact-core/src/ffi/index.js:94
throw new Error(`Failed to load native module, try setting LOG_LEVEL=debug for more info \n. ${error}`);
^
Error: Failed to load native module, try setting LOG_LEVEL=debug for more info
. TypeError: Cannot read properties of undefined (reading 'pactffiInitWithLogLevel')
2023-07-06T10:41:20.317521Z DEBUG ThreadId(01) pactffi_create_mock_server_for_pact{pact=PactHandle { pact_ref: 1 } addr_str=0x16f8e5958 tls=false}: pact_mock_server::mock_server: Started mock server on 127.0.0.1:51574
2023-07-06T10:41:20.317539Z TRACE ThreadId(01) pactffi_create_mock_server_for_pact{pact=PactHandle { pact_ref: 1 } addr_str=0x16f8e5958 tls=false}: pact_ffi::mock_server::handles: with_pact after - ref = 1, inner = RefCell { value: PactHandleInner { pact: V4Pact { consumer: Consumer { name: "foo-consumer" }, provider: Provider { name: "bar-provider" }, interactions: [SynchronousHttp { id: None, key: None, description: "a request to get a dog", provider_states: [ProviderState { name: "fido exists", params: {} }], request: HttpRequest { method: "GET", path: "/dogs/1234", query: None, headers: None, body: Missing, matching_rules: MatchingRules { rules: {PATH: MatchingRuleCategory { name: PATH, rules: {} }} }, generators: Generators { categories: {} } }, response: HttpResponse { status: 200, headers: Some({"Content-Type": ["application/json"]}), body: Present(b"{\"age\":23,\"alive\":true,\"name\":\"fido\"}", Some(ContentType { main_type: "application", sub_type: "json", attributes: {}, suffix: None }), None), matching_rules: MatchingRules { rules: {BODY: MatchingRuleCategory { name: BODY, rules: {} }} }, generators: Generators { categories: {} } }, comments: {}, pending: false, plugin_config: {}, interaction_markup: InteractionMarkup { markup: "", markup_type: "" }, transport: None }], metadata: {"pactRust": Object {"ffi": String("0.4.0")}}, plugin_data: [] }, mock_server_started: true, specification_version: V3 } }
51574
[
{
method: 'GET',
path: '/dogs/1234',
request: { method: 'GET', path: '/dogs/1234' },
type: 'missing-request'
}
]
[11:41:20.320] INFO (10256): [email protected]: Deleting Pact Server with options:
{"dir":"/Users/yousaf.nabi/dev/fooscratch","pactFileWriteMode":"overwrite","ssl":false,"cors":false,"host":"localhost"}
pact ruby standalone server created and deleted |
|
found the source of the issue Lines 57 to 61 in 39e1558 an extra |
🗣 Context
Would like Pact to run batteries included for all major platforms and architectures
💬 Narrative
When run
npm install @pact-foundation/pact --ignore-scriptson any major platform/architecture, and I am on a supported version of node, I should be able to install Pact🏗 Design
✅ Acceptance Criteria
GIVEN I have node version
16,18or20installedAND I am on one of the following platforms
WHEN I run
npm install @pact-foundation/pact --ignore-scriptsTHEN I should be able to use
pact✅ = installs without build hooks, pre-built native library
❌ = not supported
🚫 Out of Scope
.sofile building - will come later.dllfile building - will come later📝 Summary of Changes
Changes proposed in this pull request:
npx prebuildify --napito generate bindings for each targeted platform/arch combinationnapibuild against supported LTS versions of nodeRuby arm64 runtimes
node-addon-api compatability
ABI Matrix
We can see version in
NAPI_VERSION=3herehttps://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
This PR seeks to address those concerns, by allowing maintainers to easily add or remove different versions of node for testing, and ensuring the latest LTS versions are tested and covered.
A combination of
nvmandnvsis used to manage node versions in.cirrus.yml, asnvsoffers a nicer interface for gettingx64builds which you are on arm hardware (testing rosetta on MacOS)🔨 How To Test
💻 Local Machine
prebuildify.bash script/download-libs.shnpm ci --ignore-scriptsnode_moduleswe only neednode-addon-apifor nownpx prebuildify --napinode-gyp-buildand outputs a platform specific pact.node in theprebuildsdirectoryrm -rf build ffiprebuildsnpm run buildnpm run test🐱💻 Local Machine - GitHub Actions
Linux workflows
act --container-architecture linux/amd64 -W .github/workflows/build-and-test.yml --artifact-server-path pkgs🐱💻 Local Machine - Cirrus CLI
Linux workflows
cirrus run --output github-actions "linux_amd64" --artifacts-dir pkgsMacOS workflows
cirrus run --output github-actions "macos_arm_prebuilder" --artifacts-dir pkgscirrus run --output github-actions "macos_x64_prebuilder" --artifacts-dir pkg