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

Windows build #327

Merged
merged 35 commits into from
Jul 13, 2022
Merged

Windows build #327

merged 35 commits into from
Jul 13, 2022

Conversation

ten0s
Copy link
Contributor

@ten0s ten0s commented Jun 9, 2022

This PR makes it possible to build and run on Windows using Node.js 12, see GitHub workflow for Windows.

At this point the next examples run successfully:

examples/hello-gtk.js
examples/drawing-area.js
examples/builder-auto-connect-signals.js
examples/builder.js
examples/entry.js

Closes #107

@ten0s
Copy link
Contributor Author

ten0s commented Jun 9, 2022

Screenshot_2022-06-09_16-18-21

@romgrk
Copy link
Owner

romgrk commented Jun 14, 2022

Amazing work 😮 I have limited time these days but ping me when it's ready, I'll do a quick review and as long as the tests are passing for the existing OSes I'm ok with merging it.

ten0s added 11 commits June 20, 2022 16:03
Fixes some segfaults on Windows, like
const val = new GObject.Value()
val.init(GObject.TYPE_STRING) <- crash
Fixes some crases on Windows.
Fixes 5 tests
Fixes 1 test on Windows
GType is gsize is 8 bytes
gulong is 8 bytes on Linux and 4 bytes Windows

Fixes some segfaults on Windows
Fixes 3 tests on Windows
Number.MAX_SAFE_INTEGER is 8 bytes, but
vInt is gulong 4 bytes on Windows and 8 bytes on Linux
@ten0s
Copy link
Contributor Author

ten0s commented Jul 8, 2022

@romgrk At this point all the tests pass on Linux and the majority of tests pass on Windows.

The two skipped tests callback.js and signal__non-introspected.js (both Gst) pass locally on my machine, but seems to crash in CI. Since Gst doesn't interest me I'm not going to look into them.

Also I disabled one test inside conversion__array.js introduced in #313, since it crashes somewhere inside node. See comment in code.

MacOS build still fails #326, so no idea if my changes somehow affected it.

@romgrk
Copy link
Owner

romgrk commented Jul 10, 2022

I've started the test suite, let's see what passes.

Overall LGTM, I'm ok merging as it is. Some windows support is better than no windows support.

One thing though, for the cairo.cc changes, those files are actually autogenerated, can you update the corresponding generator?

#define SET_METHOD(tpl, name) Nan::SetPrototypeMethod(tpl, #name, name)
static void AttachMethods(Local<FunctionTemplate> tpl) {
${functions.map(fn =>
(fn.attributes.version ? (() => {
const [major, minor, micro] = fn.attributes.version.split('.')
return '#if ' + [
(major ? 'CAIRO_VERSION_MAJOR >= ' + major : undefined),
(minor ? 'CAIRO_VERSION_MINOR >= ' + minor : undefined),
(micro ? 'CAIRO_VERSION_MICRO >= ' + micro : undefined),
].filter(Boolean).join(' && ') + '\n '
})() : '')
+ `SET_METHOD(tpl, ${getJSName(fn.name)});`

@ten0s
Copy link
Contributor Author

ten0s commented Jul 10, 2022

Will do.

Seems like I found the cause of
a1e0f72#diff-2ecb150c1fd88f2ab8aaf603ccee701e18d113bdb931ffc3aa4eb528243a3a38R1347-R1349

Will change, test and give you know.

On Windows g_free memory allocated with malloc/calloc crashes
@ten0s
Copy link
Contributor Author

ten0s commented Jul 10, 2022

@romgrk Done

Copy link
Owner

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

A few comments, but LGTM. If you're not sure about the macos stuff try to leave it untouched.

README.md Outdated Show resolved Hide resolved
binding.gyp Show resolved Hide resolved
package.json Show resolved Hide resolved
src/function.cc Outdated Show resolved Hide resolved
src/function.cc Outdated
@@ -448,16 +446,17 @@ Local<Value> FunctionCall (
* Third, make the actual ffi_call
*/

void *ffi_args[func->n_total_args];
void **ffi_args = new void*[func->n_total_args]();
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ten0s
Copy link
Contributor Author

ten0s commented Jul 12, 2022

@romgrk Done

@romgrk romgrk merged commit 235b5e2 into romgrk:master Jul 13, 2022
@romgrk
Copy link
Owner

romgrk commented Jul 13, 2022

Good, nice work. Not sure I would have got around to doing the windows support by myself, so thanks a lot :)

@ten0s
Copy link
Contributor Author

ten0s commented Jul 13, 2022

Great! Hopefully the project will start gaining more traction!

@romgrk
Copy link
Owner

romgrk commented Jul 13, 2022

Yeah, my dream as always been to have a clean & simple native alternative to electron. node-qt is probably better placed than this project, but the downside is they use Qt which is ugly by default.

@ten0s ten0s deleted the windows-build branch December 23, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"$(uname)" was unexpected at this time.
2 participants