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.

Sign up for GitHub

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

Jump to bottom

Support build for electron v20 #870

Merged
merged 7 commits into from
Nov 21, 2022
Merged

Support build for electron v20 #870

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
81aabad
support prebuild for electron v20
neoxpert Aug 26, 2022
d3cf4cc
Remove unsupported versions of Node and Electron
mceachen Sep 9, 2022
361be1c
enforce the use of node-gyp 8.4.1
neoxpert Sep 9, 2022
a0227e3
conditional compile with CreationContext for NodeJs < 16
neoxpert Sep 10, 2022
7999f41
use c++17 to fix Mac M1 and Linux x64 builds
neoxpert Oct 13, 2022
926f618
Merge branch 'WiseLibs:master' into fix_electron20_build
neoxpert Nov 7, 2022
8cde058
add c++17 flag for MSVC
neoxpert Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ jobs:
node-version: 16
- run: npm install --ignore-scripts
- run: npx --no-install prebuild -r node -t 10.20.0 -t 12.0.0 -t 14.0.0 -t 16.0.0 -t 18.0.0 --include-regex 'better_sqlite3.node$' -u ${{ secrets.GITHUB_TOKEN }}
- run: npx --no-install prebuild -r electron -t 10.0.0 -t 11.0.0 -t 12.0.0 -t 13.0.0 -t 14.0.0 -t 15.0.0 -t 16.0.0 -t 17.0.0 -t 18.0.0 -t 19.0.0 --include-regex 'better_sqlite3.node$' -u ${{ secrets.GITHUB_TOKEN }}
- run: npx --no-install prebuild -r electron -t 10.0.0 -t 11.0.0 -t 12.0.0 -t 13.0.0 -t 14.0.0 -t 15.0.0 -t 16.0.0 -t 17.0.0 -t 18.0.0 -t 19.0.0 -t 20.0.0 --include-regex 'better_sqlite3.node$' -u ${{ secrets.GITHUB_TOKEN }}
- if: matrix.os == 'windows-2019'
run: npx --no-install prebuild -r electron -t 10.0.0 -t 11.0.0 -t 12.0.0 -t 13.0.0 -t 14.0.0 -t 15.0.0 -t 16.0.0 -t 17.0.0 -t 18.0.0 -t 19.0.0 --include-regex 'better_sqlite3.node$' --arch ia32 -u ${{ secrets.GITHUB_TOKEN }}
run: npx --no-install prebuild -r electron -t 10.0.0 -t 11.0.0 -t 12.0.0 -t 13.0.0 -t 14.0.0 -t 15.0.0 -t 16.0.0 -t 17.0.0 -t 18.0.0 -t 19.0.0 -t 20.0.0 --include-regex 'better_sqlite3.node$' --arch ia32 -u ${{ secrets.GITHUB_TOKEN }}

prebuild-alpine:
name: Prebuild on alpine
Expand Down
6 changes: 6 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,17 @@
"cli-color": "^2.0.2",
"fs-extra": "^10.1.0",
"mocha": "^8.3.2",
"node-gyp": "9.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is required? better-sqlite3 never had node-gyp in its dependencies and from the native modules I know this is not common. Can you link to some docs that recommend doing it? I think this will cause more trouble down the line and it's technically not a dependency but part of the build tooling you need (just like Python, make or gcc). If anything it would be a optionalDependencies or peerDependencies.

I'm just trying to improve the chances of getting this merged anytime soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here was, that prebuild is still using node-gyp 6.0.1. If you try to build for electron 20, using this old version, the build would fail, stating that node-gyp below version 8.4.1 is not supported anymore. So the explicit use of at least node-gyp 8.4.1 is required.

In addition to that I only had msvc 2022 available on the machine I was testing on, which requires node-gyp > 9.0.0 as otherwise no compiler is found.

Copy link
Contributor

@Prinzhorn Prinzhorn Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we can extend the node-gyp docs here https://github.com/WiseLibs/better-sqlite3/blob/master/docs/troubleshooting.md . There have been issues in the past where local node-gyp would override the global one. I think that's why it was left to the user (and in the best case you're getting prebuilds and don't even need to worry about it). I'm not pro or against either, I want whats recommended and is the best option for our users.

Copy link
Contributor Author

@neoxpert neoxpert Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I tried using a global installation of node-gyp on a fresh VM. Without overwriting node-gyp, the local version of prebuild still overwrites the global one ending up with building using node-gyp 6.0.1 which is not viable.

Running npx --no-install prebuild -r electron -t 20.0.0 yields:

prebuild\electron\20.0.0\include\node\node.h(27): fatal error C1189: #error:  "It looks like you are building this native module without using the right config.gypi.  Thi
s normally means that you need to update electron-rebuild (>=3.2.8) or node-gyp (>=8.4.0) if you're building modules directly."

I'll remove the dependency, but how to address this issue? No matter which approach I try, to enforce the use of the global installed node-gyp version, prebuild always falls back to its local 6.1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, maybe we need to include node-gyp prebuild/prebuild#286 and the overrides then. I'm not an expert on this, but if it makes the builds more predictable I'm all for it. And I changed my mind on the "it's technically not a dependency", since the build-release script literally needs node-gyp when no prebuild is found. But it should be the absolute minimum like 8.4.0 to be as compatible as possible. Otherwise if a project contains a second dependency that depends on node-gyp we're increasing the chances of conflicts. But someone else should weigh in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As sqlite3 is using 8.4.1 I would tend to also go with this version for now. But as I am not that deep into this topic yet, I am open for any good advise.

"nodemark": "^0.3.0",
"prebuild": "^11.0.4",
"sqlite": "^4.1.1",
"sqlite3": "^5.0.8"
},
"overrides": {
"prebuild": {
"node-gyp": "$node-gyp"
}
},
"scripts": {
"install": "prebuild-install || npm run build-release",
"build-release": "node-gyp rebuild --release",
Expand Down
5 changes: 2 additions & 3 deletions src/better_sqlite3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,7 @@ void SetPrototypeGetter (v8::Isolate * isolate, v8::Local <v8::External> data, v
0,
data,
v8::AccessControl::DEFAULT,
v8::PropertyAttribute::None,
v8::AccessorSignature::New(isolate, recv)
v8::PropertyAttribute::None
);
}
#line 4 "./src/util/constants.lzz"
Expand Down Expand Up @@ -1949,7 +1948,7 @@ bool Binder::IsPlainObject (v8::Isolate * isolate, v8::Local <v8::Object> obj)
#line 35 "./src/util/binder.lzz"
{
v8::Local<v8::Value> proto = obj->GetPrototype();
v8::Local<v8::Context> ctx = obj->CreationContext();
v8::Local<v8::Context> ctx = obj->GetCreationContext().ToLocalChecked();
ctx->Enter();
v8::Local<v8::Value> baseProto = v8::Object::New(isolate)->GetPrototype();
ctx->Exit();
Expand Down
2 changes: 1 addition & 1 deletion src/util/binder.lzz
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ private:

static bool IsPlainObject(v8::Isolate* isolate, v8::Local<v8::Object> obj) {
v8::Local<v8::Value> proto = obj->GetPrototype();
v8::Local<v8::Context> ctx = obj->CreationContext();
v8::Local<v8::Context> ctx = obj->GetCreationContext().ToLocalChecked();
ctx->Enter();
v8::Local<v8::Value> baseProto = v8::Object::New(isolate)->GetPrototype();
ctx->Exit();
Expand Down
3 changes: 1 addition & 2 deletions src/util/macros.lzz
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ void SetPrototypeGetter(
0,
data,
v8::AccessControl::DEFAULT,
v8::PropertyAttribute::None,
v8::AccessorSignature::New(isolate, recv)
v8::PropertyAttribute::None
);
}