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

Added Darwin ARM64 binaries #1594

Merged
merged 1 commit into from Sep 15, 2022
Merged

Added Darwin ARM64 binaries #1594

merged 1 commit into from Sep 15, 2022

Conversation

daniellockyer
Copy link
Member

@daniellockyer daniellockyer commented Apr 27, 2022

Please see this comment:

Hey all! 👋🏻 Whilst I would love to merge this, the generated binaries it produces get caught up with MacOS' quarantine flag, so the it requires you to remove the flag in order for it to be run, which is clearly not going to work from an end-user perspective.

I can't tell if it's something I'm doing locally or whether there is some security issue I've missed.

Any ideas welcome 🙂

@daniellockyer daniellockyer self-assigned this Apr 27, 2022
@daniellockyer daniellockyer force-pushed the arm64-test branch 20 times, most recently from 3bfe330 to e145caa Compare April 28, 2022 08:05
@daniellockyer daniellockyer force-pushed the arm64-test branch 4 times, most recently from 376637a to 832950b Compare May 19, 2022 17:37
@daniellockyer daniellockyer changed the title WIP: added Darwin arm64 binaries Added Darwin ARM64 binaries May 19, 2022
@adrian-arapiles
Copy link

Any update about that? It's seems one test failed because connection issues.
Really appreciated this could be released if it's working.
Thanks.

@Vyazovoy
Copy link

Would be really good to merge this PR and have prebuilt version on M1 instead of building it from source.

@daniellockyer
Copy link
Member Author

Hey all! 👋🏻 Whilst I would love to merge this, the generated binaries it produces get caught up with MacOS' quarantine flag, so the it requires you to remove the flag in order for it to be run, which is clearly not going to work from an end-user perspective.

I can't tell if it's something I'm doing locally or whether there is some security issue I've missed.

Any ideas welcome 🙂

@adrian-arapiles
Copy link

Hey all! 👋🏻 Whilst I would love to merge this, the generated binaries it produces get caught up with MacOS' quarantine flag, so the it requires you to remove the flag in order for it to be run, which is clearly not going to work from an end-user perspective.

I can't tell if it's something I'm doing locally or whether there is some security issue I've missed.

Any ideas welcome 🙂

Hello, thanks in advance for your efforts.
Could you give us the steps to reproduce the issue that you are explaining?
I could try to reproduce or find any clue about the problem.

Cheers :)

@daniellockyer daniellockyer force-pushed the arm64-test branch 2 times, most recently from 3f074ce to 771fe07 Compare September 13, 2022 11:33
@daniellockyer
Copy link
Member Author

Hey @adrian-arapiles 👋🏻 I've just rebased the branch so the binaries are built against the latest and I can explain the problem.

  1. download the binary assets from the recent GHA run: https://github.com/TryGhost/node-sqlite3/actions/runs/3044701485
  2. unzip the archive - unzip prebuilt-binaries.zip
  3. untar the ARM64 NAPI v6 binaries - tar -zxvf v5.0.11/napi-v6-darwin-unknown-arm64.tar.gz
  4. move the prebuilt binary into your node-sqlite3 binding folder: mv ~/Downloads/v5.0.11/napi-v6-darwin-unknown-arm64/node_sqlite3.node lib/binding/napi-v6-darwin-unknown-arm64/
    a. I cleared out the v3 folder just to make sure
  5. run node -e 'require("./")'

I get the following popup:

CleanShot 2022-09-13 at 12 54 03@2x

And the following error:

λ node -e 'require("./")'
node:internal/modules/cjs/loader:1210
  return process.dlopen(module, path.toNamespacedPath(filename));
                 ^

Error: dlopen(/xxxx/node-sqlite3/lib/binding/napi-v6-darwin-unknown-arm64/node_sqlite3.node, 0x0001): tried: '/xxxx/node-sqlite3/lib/binding/napi-v6-darwin-unknown-arm64/node_sqlite3.node' (code signature in <E5FD3505-0605-3412-9A6F-AB4295BC5797> '/xxxx/node-sqlite3/lib/binding/napi-v6-darwin-unknown-arm64/node_sqlite3.node' not valid for use in process: library load disallowed by system policy)
    at Object.Module._extensions..node (node:internal/modules/cjs/loader:1210:18)
    at Module.load (node:internal/modules/cjs/loader:1004:32)
    at Function.Module._load (node:internal/modules/cjs/loader:839:12)
    at Module.require (node:internal/modules/cjs/loader:1028:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/xxxx/node-sqlite3/lib/sqlite3-binding.js:4:17)
    at Module._compile (node:internal/modules/cjs/loader:1126:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1180:10)
    at Module.load (node:internal/modules/cjs/loader:1004:32)
    at Function.Module._load (node:internal/modules/cjs/loader:839:12) {
  code: 'ERR_DLOPEN_FAILED'
}

@gjsjohnmurray
Copy link

@bpasero can you or another member of the VS Code team provide any pointers to @daniellockyer about this? You likely have a pool of experience in dealing with Apple security architecture.

I think VS Code depends on the sqlite3 package, right? Presumably at the moment the Darwin ARM64 binary has to be built on the user's machine, in which case it'd surely be a good thing to get this resolved.

@bpasero
Copy link
Contributor

bpasero commented Sep 15, 2022

@deepak1556 can you chime in how we build SQLite for ARM.

@deepak1556
Copy link

I think this only an issue if you are downloading non-notarized executable files from the browser which end up adding the com.apple.quarantine attribute. Have you tried downloading the file with other clients like curl or Node.js ? I think it should be fine when prebuilds are downloaded during npm step. For example,

  1. Get the request URL for the above artifact

Screenshot 2022-09-15 at 8 35 26 PM

  1. curl -X GET "<REPLACE WITH URL>" --output node_sqlite3.zip
  2. xattr node_sqlite3.zip will confirm that quarantine bit is not set
> node
Welcome to Node.js v16.14.2.
Type ".help" for more information.
> require("./v5.0.11/napi-v6-darwin-unknown-x64/node_sqlite3.node")
{
  Database: [Function: Database],
  Statement: [Function: Statement],
  Backup: [Function: Backup],
  OPEN_READONLY: 1,
  OPEN_READWRITE: 2,
  OPEN_CREATE: 4,
  OPEN_FULLMUTEX: 65536,
  OPEN_URI: 64,
  OPEN_SHAREDCACHE: 131072,
  OPEN_PRIVATECACHE: 262144,
  VERSION: '3.39.3',
  SOURCE_ID: '2022-09-05 11:02:23 4635f4a69c8c2a8df242b384a992aea71224e39a2ccab42d8c0b0602f1e826e8',
  VERSION_NUMBER: 3039003,
  OK: 0,
  ERROR: 1,
  INTERNAL: 2,
  PERM: 3,
  ABORT: 4,
  BUSY: 5,
  LOCKED: 6,
  NOMEM: 7,
  READONLY: 8,
  INTERRUPT: 9,
  IOERR: 10,
  CORRUPT: 11,
  NOTFOUND: 12,
  FULL: 13,
  CANTOPEN: 14,
  PROTOCOL: 15,
  EMPTY: 16,
  SCHEMA: 17,
  TOOBIG: 18,
  CONSTRAINT: 19,
  MISMATCH: 20,
  MISUSE: 21,
  NOLFS: 22,
  AUTH: 23,
  FORMAT: 24,
  RANGE: 25,
  NOTADB: 26,
  LIMIT_LENGTH: 0,
  LIMIT_SQL_LENGTH: 1,
  LIMIT_COLUMN: 2,
  LIMIT_EXPR_DEPTH: 3,
  LIMIT_COMPOUND_SELECT: 4,
  LIMIT_VDBE_OP: 5,
  LIMIT_FUNCTION_ARG: 6,
  LIMIT_ATTACHED: 7,
  LIMIT_LIKE_PATTERN_LENGTH: 8,
  LIMIT_VARIABLE_NUMBER: 9,
  LIMIT_TRIGGER_DEPTH: 10,
  LIMIT_WORKER_THREADS: 11
}

@deepak1556
Copy link

For VSCode the modules are built from source during application package generation in CI (there is no module build in users device, arm64 modules are cross compiled on x64 CI in our case)and the modules also get code signed along with the final package getting notarized. This is different from what is being accomplished in this PR, so expanding on our build process might not be of much help here.

@daniellockyer
Copy link
Member Author

@deepak1556

downloading non-notarized executable files from the browser which end up adding the com.apple.quarantine attribute.

Oh! Very interesting - it works fine for me using wget. Do you have a reference for why that would be the case with downloading via the browser? I searched all over and never found that

@daniellockyer
Copy link
Member Author

This seems good news though 🙂

- this adds support for supplying Darwin ARM64 prebuilt binaries for
  node-sqlite3
@daniellockyer daniellockyer marked this pull request as ready for review September 15, 2022 12:13
@daniellockyer daniellockyer merged commit ec154ab into master Sep 15, 2022
@daniellockyer daniellockyer deleted the arm64-test branch September 15, 2022 12:21
@daniellockyer
Copy link
Member Author

I've merged this and will do some further testing before releasing - thanks all 🙂

@gjsjohnmurray
Copy link

@daniellockyer please can we also have the binary for Node v14? That is the version currently used by the mtxr/sqltools project which led me here.

@daniellockyer
Copy link
Member Author

@gjsjohnmurray The binaries only vary on the Node-API version they're built against: https://nodejs.org/api/n-api.html#node-api-version-matrix. We support v3 and v6 right now. If you've got an ARM64 build of Node 14, it should work 🙂

atulsmadhugiri added a commit to CommE2E/comm that referenced this pull request Oct 18, 2022
Summary:
`@redux-devtools/cli` is dependent on the `sqlite3` node package (https://github.com/TryGhost/node-sqlite3). There previously weren't aarch64 binaries available, so `sqlite3` had to be built from scratch which made `yarn cleaninstall` take a while.

They recently added prebuilt binaries (TryGhost/node-sqlite3#1594), so we should bump the version to take advantage of this.

On my machine this took `yarn cleaninstall` from ~90 seconds to under a minute (~58 seconds).

Test Plan:
Read through release notes and things should continue to work as expected.

NOTE: This is unrelated to the SQLite client DB we use in the `native` app. This SQLite is solely to power Redux dev tools (which still work as expected).

Reviewers: tomek, marcin, ginsu, rohan, varun, ashoat

Reviewed By: ashoat

Subscribers: abosh

Differential Revision: https://phab.comm.dev/D5385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants