Skip to content

Sync with upstream TryGhost/node-sqlite3 v6.0.1 - #18

Merged
paulfitz merged 72 commits into
gristlabs:grist-mainfrom
gristgouv:sync-upstream-v6.0.1-bis
Jun 11, 2026
Merged

Sync with upstream TryGhost/node-sqlite3 v6.0.1#18
paulfitz merged 72 commits into
gristlabs:grist-mainfrom
gristgouv:sync-upstream-v6.0.1-bis

Conversation

@fflorent

Copy link
Copy Markdown

The problem

node-sqlite3 is out of date. Newer version brings fixes and dependencies upgrade.

The solution

At the contrary of #17, I have synchronized this branch with the upstream branch myself.

Please be aware that I am not very sure about the resolutions of the conflicts.

Merge upstream master (v6.0.1) into grist-main

Summary of the conflict resolution:

  1. For the CI:
  • Continue running exclusively on OS Ubuntu 22
  • Continue building exclusively for Node 22
  • Bump actions where applicable
  • ⚠️ Remove build-alpine and import the build-musl job from upstream
  • Take into account the new scripts in nodejs (cf below)
  1. For the package.json file:
  • Bump version to 6.0.1-grist.1
  • Remove items from the binary section
  • Import the changes upstream
  • Keep the specific scripts introduced by the fork: test:memory,
    build:assert (renamed rebuild:assert), and adapt the scripts to
    the new version of node-gyp
  1. database.cc:
  • Mostly keep upstream changes, I only see introduction of
    the UNUSED(status); macro after invoking napi_create_async_work.
    Just call CREATE_WORK now.
  1. macros.h:
  • CREATE_WORK calls UNUSED(status); to bring back the desired
    behavior introduced previously in the fork
  • remove the DELETE_FIELD macro like in upstream
  1. statement.h:
  • reintroduce WORK_DEFINITION(AllMarshal) and remove the semi-colon
    when invoking macros just like upstream

renovateBotand others added 30 commits February 4, 2023 07:36
It seems that an old refactoring (~8 years) made a mistake on the way conditions are expressed in gyp.
the "conditions" key cannot be set 2 times and the conditions should be all set in the "conditions" array.
The impact of this bug is that when sqlite3 is compiled against a custom sqlite3 source tree, the sqlite3 runtime keeps linking with the system sqlite3 instead of linking to the custom compiled sqlite3 library. In my case that lead to a SIGSEGV upon loading an extension.
refs https://www.sqlite.org/releaselog/3_41_1.html
- sha3-256sum: `38ecb6b086c5c1ee1e52b57556745055328ac912929ccade9deaefdd71033ddb`
refs GHSA-jqv5-7xpx-qj74fixeshttps://github.com/TryGhost/Toolbox/issues/491
- when you call `ToString()` on `Napi::Value`, it calls
`napi_coerce_to_string` underneath, which has the ability to run
arbitrary JS code if the passed in value is a crafted object
- both remote code execution or denial-of-service are possible via
this vulnerability
- `toString()` on an Object returns `[object Object]` so instead of
calling the function, we're going to hardcode it to prevent this
issue
Credits: Dave McDaniel of Cisco Talos
…h methods callback rows (TryGhost#1686)
* Change sqlite3.d.ts to add generic type definition support for Statement methods
-For get method callback's row
-For all method callback's rows
-For each method callback's row
* Change sqlite3.d.ts to add generic type definition support for Database methods
-For get method callback's row
-For all method callback's rows
-For each method callback's row
- added myself as a contributor 😀
fixesTryGhost#1690
- the minimum glibc version was inadvertently bumped in the latest
release because `log2` was linked to a higher version
- this adds the downgrade to the gcc preinclude file to resolve this
- in an upcoming commit, we will add a test to prevent these issues from
occurring again
- without this, the syntax is not recognised because we use `sh`
refs https://www.sqlite.org/releaselog/3_42_0.html
- sha3-256sum: `643898e9fcc8f6069bcd47b0e6057221c1ed17bbee57da20d2752c79d91274e8`
…dability
- Implemented RAII to ensure resource safety and exception safety
- Leveraged range-based for loops for cleaner and more efficient iteration
- Addressed and resolved all project-related warnings for enhanced code quality
refs https://www.sqlite.org/releaselog/3_44_2.html
- sha3-256sum: `6c427f0547e2f7babe636b748dd5d5a1f2f31601adadef7e2805e7d1f7171861`
- this doesn't do anything because we just rethrow the error
- Python 3.12 no longer ships with this and GHA CI has updated to 3.12,
which breaks our build scripts
- this should fix that until we can update node-gyp
- merged pretest and test
- removed `pack` as it is not needed
- right now our way to check semver compatibility is to run the tests on
every version
- this is really inefficient as it results in a lot of CI jobs that we
don't need
- this should run a CI job that ensures the minimum version we accept is
Node 10.12.0
- bumped building on Node 16 to Node 18
- removed all matrix executions apart from Node 18
…stall`
fixesTryGhost#1641fixesTryGhost#1721fixesTryGhost#1714fixesTryGhost#1713fixesTryGhost#1700fixesTryGhost#1704
- `@mapbox/node-pre-gyp` is effectively unmaintained [1] as has a few bugs
which our users keep running into
- it seems the prebuilt binary world has moved in favor of prebuild +
it's various other forms
- one option would be to use prebuildify to bundle all binaries into the
package, but that's a step too far removed from the current situation
for now
- instead, we can use prebuild-install to download the binaries, and
`prebuild` to build + upload the binaries
- this means we can remove node-pre-gyp and fix a bunch of issues!
- eventually, we could start providing electron prebuilt binaries too
[1]: mapbox/node-pre-gyp#657
- switched to just providing a list of platform + arch instead of the
full formatted string
renovateBotand others added 9 commits April 7, 2026 06:14
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…0.1-bis
Summary of the conflict resolution:
1. For the CI:
- Continue running exclusively on OS Ubuntu 22
- Continue building exclusively for Node 22
- Bump actions where applicable
- Remove build-alpine and import the build-musl job from upstream
- Take into account the new scripts in nodejs (cf below)
2. For the package.json file:
- Bump version to 6.0.1-grist.1
- Remove items from the binary section
- Import the changes upstream
- Keep the specific scripts introduced by the fork: `test:memory`,
`build:assert` (renamed `rebuild:assert`), and adapt the scripts to
the new version of node-gyp
3. database.cc:
- Mostly keep upstream changes, I only see introduction of
the `UNUSED(status);` macro after invoking napi_create_async_work.
Just call CREATE_WORK now.
4. macros.h:
- CREATE_WORK calls `UNUSED(status);` to bring back the desired
behavior introduced previously in the fork
- remove the `DELETE_FIELD` macro like in upstream
5. statement.h:
- reintroduce `WORK_DEFINITION(AllMarshal)` and remove the semi-colon
when invoking macros just like upstream
@fflorentfflorent changed the title Sync upstream v6.0.1Sync with upstream TryGhost v6.0.1Jun 3, 2026
Still use VARIANT=alpine3.20 in the docker image
@fflorent
fflorentforce-pushed the sync-upstream-v6.0.1-bis branch from f44db33 to 9ddbdd5CompareJune 3, 2026 12:03
@fflorentfflorent moved this to Needs feedback in French administration BoardJun 3, 2026
@fflorentfflorent changed the title Sync with upstream TryGhost v6.0.1Sync with upstream TryGhost/node-sqlite3 v6.0.1Jun 3, 2026

@paulfitzpaulfitz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good. A few questions.

Is this affected by TryGhost#1859 ?

the upstream engines: { "node": ">=20.17.0" } in package.json should probably be kept?

Are the musl-related changes tested?

@fflorent

Copy link
Copy Markdown
Author

Is this affected by TryGhost#1859 ?

Thanks for pointing that!

I cherry-picked this PR, and successfully checked the binary is the one offered by sqlite (this one).

the upstream engines: { "node": ">=20.17.0" } in package.json should probably be kept?

Oups, restored!

Are the musl-related changes tested?

No, I am not sure how we can test this. Do you have in mind running the build-musl job?

@fflorent

Copy link
Copy Markdown
Author

I have run the job:
https://github.com/gristgouv/node-sqlite3/actions/runs/27355622745/job/80829015006

The jobs for Windows fail. Is this an issue?

@paulfitz

Copy link
Copy Markdown
Member

I have run the job: https://github.com/gristgouv/node-sqlite3/actions/runs/27355622745/job/80829015006

The jobs for Windows fail. Is this an issue?

May be a github runner change?

"resolutions": {
"node-gyp": "12.x"
}

may help

@fflorent
fflorentforce-pushed the sync-upstream-v6.0.1-bis branch from 5a933e9 to 725b105CompareJune 11, 2026 15:38
@fflorent

fflorent commented Jun 11, 2026

Copy link
Copy Markdown
Author

@paulfitz Thank you very much! 🙏

I tried to push this commit (5a933e9) to support windows in arm64, run the jobs and it failed. So I rolled back to 725b105. The CI is green: https://github.com/gristgouv/node-sqlite3/actions/runs/27358131751/job/80837931710

@paulfitzpaulfitz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fflorent

@paulfitz
paulfitz merged commit 26c45ba into gristlabs:grist-mainJun 11, 2026
9 of 21 checks passed
@github-project-automationgithub-project-automationBot moved this from Needs feedback to Done in French administration BoardJun 11, 2026
@fflorent
fflorent deleted the sync-upstream-v6.0.1-bis branch June 11, 2026 16:03
paulfitz pushed a commit to gristlabs/grist-core that referenced this pull request Jun 12, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

10 participants

@fflorent@paulfitz@jeromew@daniellockyer@stevescruz@zenon8adams@JohnONolan@lsinger@jonatansberg@MatheusBaldi