fix: missing SQLite builtin support in Node.js #47706
Conversation
|
The sqlite test suites in upstream under |
|
@deepak1556 it's because they all fell into https://github.com/nodejs/node/blob/f8809cef63540492a1e589a55cc6d84cac6245df/test/common/index.js#L687-L691 from the same PR that caused this - the sqlite tests thought sqlite was disabled on purpose. |
| sources += gypi_values.node_crypto_sources | ||
| } | ||
| + if (node_use_sqlite) { | ||
| + sources += gypi_values.node_sqlite_sources |
There was a problem hiding this comment.
Should we use chromes sqlite instead? Avoid linking two different sqlites
There was a problem hiding this comment.
This is just the core sources not the sqlite dependency as far as I see https://github.com/nodejs/node/blob/3567fa7d70fee8831ab2891e6940ca63be568b08/node.gyp#L415-L420
There was a problem hiding this comment.
Ah gotcha, I guess question still stands though, we should route the sqlite dep to chrome's instead of nodes (same as boringssl and v8)
There was a problem hiding this comment.
Oh I was under the impression we already did but seems not, agree the deps should be reused from chrome would also help get rid of https://github.com/electron/electron/blob/main/patches/sqlite/fix_rename_sqlite_win32_exports_to_avoid_conflicts_with_node_js.patch
There was a problem hiding this comment.
@MarshallOfSound @deepak1556 i tried this initially but there were a serious spate of errors i couldn't figure a way around at the time (i think chrome modifies some sqlite behavior iirc) - i can try again now but i don't think that should block this
There was a problem hiding this comment.
Yeah follow-up sounds good to me since we have built in this mode for a while
There was a problem hiding this comment.
Should we use chromes sqlite instead? Avoid linking two different sqlites
Another thing to take into consideration is the SQLite extension support of Node and Chromium.
For example, Node.js has support for the Session Extension, having its own API (nodejs/node#54181), and has also enabled all compile-time flags that enable the rest of the extensions that do not require any other code modifications (nodejs/node#57621).
|
Thanks @codebytere , that makes sense. |
codebytere
force-pushed
the
fix-sqlite
branch
2 times, most recently
from
0df524d to
315ea70
Compare
codebytere
added
semver/patch
|
Release Notes Persisted
|
|
I have automatically backported this PR to "36-x-y", please check out #47755 |
|
I have automatically backported this PR to "37-x-y", please check out #47756 |
trop
bot
removed
the
target/36-x-y
|
I have automatically backported this PR to "38-x-y", please check out #47757 |
trop
bot
added
in-flight/37-x-y
in-flight/38-x-y
merged/36-x-y
Description of Change
Closes #47671
Refs nodejs/node#58122
Fixes an issue where
require('node:sqlite')didn't work - this happened in #47555 via nodejs/node#58122, which added the option to build without sqlite. That didnt get correctly copied to the GN build config.See upstream at nodejs/node#59017
Checklist
npm testpassesRelease Notes
Notes: Fixed an issue where
require('node:sqlite')didn't work.