feat: update install and publish process #131
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
binding.gyp
Outdated
| @@ -1,63 +1,70 @@ | |||
| { | |||
| 'targets': [ | |||
| "targets": [ | |||
There was a problem hiding this comment.
All of the changes in this file were to get Windows to publish. It’s mostly changes from ” to ’.
The only other meaningful changes are noted below.
There was a problem hiding this comment.
Updated. I reverted all the quote changes. The windows build error was coming from somewhere else. I noticed an indentation inconsistency in the file so when I was in there editing I reverted the quotes to the more standard '.
| "<!@(node -p \"require('node-addon-api').include_dir\")", | ||
| "include_dirs": [ | ||
| "blst/bindings", | ||
| "<!@(node -p \"require('node-addon-api').include\")", |
There was a problem hiding this comment.
This is a known issue with how windows builds native modules. Need to use the include export instead of include_dir for it to be fully cross-compatible.
binding.gyp
Outdated
| 'AdditionalOptions': [ '/std:c++17' ], | ||
| "conditions": [ | ||
| [ "OS!='win'", { | ||
| "sources": ["blst/build/assembly.S"], |
There was a problem hiding this comment.
This source is not for windows so refactored a bit to move it to a conditional.
| "node-gyp": "^10.0.1" | ||
| } | ||
| "optionalDependencies": { | ||
| "@jazzer.js/core": "^2.1.0" |
There was a problem hiding this comment.
This lib is not compatible with linux-arm64. I attempted to fix it but just ended up fighting a battle that was not worth it. Its an issue with how the base C lib exports artifacts for CMake in @jazzer/fuzzer. I put up an issue in their repo here CodeIntelligenceTesting/jazzer.js#795
| async function buildBindings(binaryName: string): Promise<string> { | ||
| await exec("npm run clean:gyp", true, {cwd: ROOT_DIR}); | ||
| await exec("npm run build:gyp", true, {cwd: ROOT_DIR}); | ||
| const bindingPath = getBindingsPath(ROOT_DIR); |
There was a problem hiding this comment.
after we use node-gyp do we know the exact location? looks like this getBindingsPath function is more suitable for other places not this one because it involves a search
There was a problem hiding this comment.
Most of the times yes but not always.
Because its possible to npm install --debug which will create a debug build in build/Debug instead of in build/Release. So its necessary to check several places...
| }, | ||
| "main": "./dist/lib/index.js", | ||
| "types": "./dist/lib/index.d.ts", | ||
| "files": [ |
There was a problem hiding this comment.
we also specified this in the publish workflow:
- name: Create Github release with prebuilds
if: steps.tag.outputs.tagcreated == 'yes'
uses: softprops/action-gh-release@v2
with:
files: release/*
tag_name: ${{ steps.tag.outputs.tagname }}
fail_on_unmatched_files: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}do they serve different purposes? looks like releases/* only contains the binding files there
There was a problem hiding this comment.
Yes. The releases are for the github release and the prebuilds get included in the npm bundle. I added information explaining this in CONTRIBUTING.md
matthewkeil
deleted the
mkeil/migrate-rebuild-update-install-and-publish
branch
Description
linux/arm64bindingsnode-fetchand movejazzer.jsto optional dependencies to supportlinux/arm64buildscripts/makeReleaseto move all CI prebuilds toreleasefolder and then selectively move back the bundled version to the npm publishedprebuildfolderrandomBytesNonZero