feat(libc): windows native module support #490

zeromake · 2024-08-24T04:59:12Z

by #486

add definde QJS_NATIVE_MODULE_SUFFIX, windows is .dll, !windows is .so.

add cmake test

zeromake · 2024-08-25T08:58:47Z

mingw it seems to be different from MSVC……

saghul · 2024-08-26T07:58:43Z

Yep, some if-defing might be required...

zeromake · 2024-08-26T15:41:24Z

@saghul ci is ok

saghul · 2024-08-26T15:49:18Z

Can you make sure the relevant tests also run on Windows?

IIRC they are not compiled in.

zeromake · 2024-08-26T15:58:29Z

@saghul
That's what you're talking about?

quickjs/CMakeLists.txt

Line 295 in 0e5e9c2

if(NOT WIN32)

quickjs-libc.c

bnoordhuis · 2024-08-26T18:14:00Z

quickjs-libc.c

+    init = (JSInitModuleFunc *)(void *)GetProcAddress(hd, "js_init_module");
+    if (!init) {
+        JS_ThrowReferenceError(ctx, "js_init_module '%s' not found: %lu",
+                               module_name, GetLastError());


It'd be nicer to use FormatMessageA() to get a human-readable error message (example)

@bnoordhuis
FormatMessageA error message fetching is a bit complicated, whether to consider a separate method to get it

struct JSContext { char* errmsg; } static int js_error(JSContext *ctx) { if (ctx->errmsg) { LocalFree(ctx->errmsg); ctx->errmsg = NULL; } // FormatMessageA …… }

@bnoordhuis Doing it The Right Way (TM) seems a bit involved: https://github.com/libuv/libuv/blob/0a00e80c3686b93eccb9a801954e86bd7d7fe6ab/src/win/dl.c#L92 perhaps we can simplify here?

saghul · 2024-08-26T20:37:44Z

@saghul

That's what you're talking about?

quickjs/CMakeLists.txt

Line 295 in 0e5e9c2

if(NOT WIN32)

Yep those will do!

zeromake · 2024-08-28T10:00:47Z

https://github.com/quickjs-ng/quickjs/actions/runs/10589030031/job/29348830278 ……

saghul · 2024-08-28T11:13:07Z

How weird, I'll take a look at that one. You can ignore it.

saghul · 2024-09-03T08:27:27Z

I guess what's missing is to run the actual tests, check this one:

quickjs/.github/workflows/ci.yml

Line 137 in 0e5e9c2

- name: test examples

quickjs-libc.c

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

zeromake · 2024-09-04T07:09:32Z

@saghul
job error:

/usr/bin/ld: libqjs.a(quickjs.c.o): relocation R_X86_64_PC32 against symbol `digits36' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: bad value

linux build so file need -fPIC flags?

saghul · 2024-09-04T14:29:27Z

Not sure why that started happening, we need to take a look, but you can ignore that for this PR.

CMakeLists.txt

zeromake · 2024-09-06T14:07:01Z

@saghul
c2391b0

windows shared use .so
fib.c use __declspec(dllexport) js_init_module
ci add examples\test_fib.js

zeromake · 2024-09-07T05:22:31Z

@saghul
except for the wrong text, It should all be done, and you can consider whether to merge or not.

CMakeLists.txt

quickjs-libc.c

zeromake · 2024-09-09T08:16:46Z

@saghul job/29858936901 What is wrong with this

saghul · 2024-09-09T08:37:58Z

Since you modified the JS part of the test, you need to commit the result of make codegen.

chqrlie · 2024-09-09T08:53:28Z

Since you modified the JS part of the test, you need to commit the result of make codegen.

Yes, this is unfortunate. We should try and find a way to generate these files at build time and avoid committing them. This would also reduce bloat in the repository.

saghul · 2024-09-09T08:57:11Z

Yes, this is unfortunate. We should try and find a way to generate these files at build time and avoid committing them. This would also reduce bloat in the repository.

I have the same conundrum on txiki.js, only worse, because I have a lot more generated code.

I gave this a long hard thought and while we could generate it in the release job and then make a "release tarball", I'd rather have the git repo be the source of truth, specially after the xz fiasco.

bnoordhuis · 2024-09-09T09:03:30Z

I concur with that sentiment, fwiw.

saghul · 2024-09-09T09:05:59Z

I concur with that sentiment, fwiw.

Which of the 2? 😅

chqrlie · 2024-09-09T09:17:23Z

I gave this a long hard thought and while we could generate it in the release job and then make a "release tarball", I'd rather have the git repo be the source of truth, specially after the xz fiasco.

I am not sure I understand the connection with the xz fiasco. We shall be very cautious about accepting PRs from, let alone giving commit rights to outsiders.

Generating the byte code files at build time is mainly a problem for cross compiling where it requires building a qjs executable for the host in addition to cross compiling for the selected target. This used to be problematic because the generated files depended on the endianness of the target, which is no longer the case.

bnoordhuis · 2024-09-09T09:26:44Z

Which of the 2? 😅

That git should be the Source Of Truth :)

(I deliberated on whether to mention I was referring to your comment but, since a couple of a minutes had passed, I figured it was unambiguously in response to the last comment.)

saghul · 2024-09-09T09:29:17Z

Let's continue here: #509 not to derail the PR.

saghul · 2024-09-09T10:11:56Z

@zeromake There seems to be a conflict. Can you please rebase with latest master?

…module

zeromake · 2024-09-09T10:13:30Z

@saghul is merged

.github/workflows/ci.yml

quickjs-libc.c

saghul

Great work! @bnoordhuis can you PTAL too?

bnoordhuis

LGTM with some suggestions and questions.

.github/workflows/ci.yml

CMakeLists.txt

examples/fib.c

quickjs-libc.c

…module

saghul · 2024-09-10T07:14:17Z

LINK : fatal error LNK1149: output filename matches input filename 'D:\a\quickjs\quickjs\build\Release\qjs.lib' [D:\a\quickjs\quickjs\build\qjs_exe.vcxproj]

WTF?!

zeromake · 2024-09-10T07:57:22Z

LINK : fatal error LNK1149: output filename matches input filename 'D:\a\quickjs\quickjs\build\Release\qjs.lib' [D:\a\quickjs\quickjs\build\qjs_exe.vcxproj]

WTF?!

i do rollback

+target_link_libraries(fib qjs)
-target_link_libraries(fib ${qjs_libs})

zeromake · 2024-09-10T11:07:59Z

@saghul runs/10788294143 run on 3 hours ago……

saghul · 2024-09-10T11:09:07Z

I think our free credit is running low 😅

saghul · 2024-09-10T13:33:08Z

Hum I see the failure again. I think it might be the JS_EXTERN change. Can you undo that and let's check again?

saghul · 2024-09-10T13:40:26Z

Looking at what we do in libuv:

#ifndef UV_EXTERN
#ifdef _WIN32
  /* Windows - set up dll import/export decorators. */
# if defined(BUILDING_UV_SHARED)
    /* Building shared library. */
#   define UV_EXTERN __declspec(dllexport)
# elif defined(USING_UV_SHARED)
    /* Using shared library. */
#   define UV_EXTERN __declspec(dllimport)
# else
    /* Building static library. */
#   define UV_EXTERN /* nothing */
# endif
#elif __GNUC__ >= 4
# define UV_EXTERN __attribute__((visibility("default")))
#elif defined(__SUNPRO_C) && (__SUNPRO_C >= 0x550) /* Sun Studio >= 8 */
# define UV_EXTERN __global
#else
# define UV_EXTERN /* nothing */
#endif
#endif /* UV_EXTERN */

I think we should undo the JS_EXTERN change and likely do something like the above later on.

saghul · 2024-09-10T20:48:36Z

@bnoordhuis I'll make some follow-up refinements since I'm playing around with the linking libraries in another PR.

saghul · 2024-09-10T20:48:50Z

Nice work @zeromake ! 👏

zeromake added 2 commits August 24, 2024 12:55

feat(libc): windows native module support

29a2588

fix(libc): NATIVE_MODULE_SUFFIX -> QJS_NATIVE_MODULE_SUFFIX

94f3753

zeromake added 2 commits August 26, 2024 19:43

fix: dirent_mbstowcs_s not has

7685526

fix: filename init NULL

c0ef55e

bnoordhuis reviewed Aug 26, 2024

View reviewed changes

fix: codeview change

f87ab72

feat: BUILD_EXAMPLES support msvc

efa5ef6

bnoordhuis reviewed Sep 3, 2024

View reviewed changes

quickjs-libc.c Outdated Show resolved Hide resolved

quickjs-libc.c Outdated Show resolved Hide resolved

quickjs-libc.c Outdated Show resolved Hide resolved

zeromake and others added 3 commits September 4, 2024 07:59

fix: == .

c703ffb

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

fix: uintptr_t conv

76a0588

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

Update quickjs-libc.c

64ae115

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

saghul mentioned this pull request Sep 5, 2024

enable windows binary module import #497

Closed

saghul reviewed Sep 5, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

zeromake added 2 commits September 6, 2024 10:00

fix: only win32 example link qjs

096ada0

feat: support ci examples

c2391b0

saghul reviewed Sep 8, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

quickjs-libc.c Show resolved Hide resolved

feat: add support dynamic import native module

fedc1bc

fix: update codegen

8e465ca

Merge remote-tracking branch 'origin/master' into feature/win-native-…

75837c2

…module

saghul reviewed Sep 9, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

quickjs-libc.c Show resolved Hide resolved

fix: del BUILD_QJS_LIBC=ON

e3b79ba

saghul approved these changes Sep 9, 2024

View reviewed changes

bnoordhuis approved these changes Sep 9, 2024

View reviewed changes

zeromake added 2 commits September 10, 2024 09:41

fix: code view change

a923934

Merge remote-tracking branch 'origin/master' into feature/win-native-…

b5183ae

…module

fix: target_link_libraries use ${qjs_libs}

4727d01

fix: examples use self JS_EXTERN

6094d3d

saghul merged commit 902cc2c into quickjs-ng:master Sep 10, 2024
50 checks passed

feat(libc): windows native module support #490

feat(libc): windows native module support #490

Conversation

zeromake commented Aug 24, 2024 • edited Loading

zeromake commented Aug 25, 2024

saghul commented Aug 26, 2024

zeromake commented Aug 26, 2024

saghul commented Aug 26, 2024

zeromake commented Aug 26, 2024

bnoordhuis Aug 26, 2024

Choose a reason for hiding this comment

zeromake Aug 28, 2024

Choose a reason for hiding this comment

saghul Sep 9, 2024

Choose a reason for hiding this comment

saghul commented Aug 26, 2024

zeromake commented Aug 28, 2024

saghul commented Aug 28, 2024

saghul commented Sep 3, 2024

zeromake commented Sep 4, 2024

saghul commented Sep 4, 2024

zeromake commented Sep 6, 2024

zeromake commented Sep 7, 2024

zeromake commented Sep 9, 2024

saghul commented Sep 9, 2024

chqrlie commented Sep 9, 2024

saghul commented Sep 9, 2024

bnoordhuis commented Sep 9, 2024

saghul commented Sep 9, 2024

chqrlie commented Sep 9, 2024 • edited Loading

bnoordhuis commented Sep 9, 2024

saghul commented Sep 9, 2024

saghul commented Sep 9, 2024

zeromake commented Sep 9, 2024

saghul left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

saghul commented Sep 10, 2024

zeromake commented Sep 10, 2024 • edited Loading

zeromake commented Sep 10, 2024

saghul commented Sep 10, 2024

saghul commented Sep 10, 2024

saghul commented Sep 10, 2024

saghul commented Sep 10, 2024

saghul commented Sep 10, 2024

zeromake commented Aug 24, 2024 •

edited

Loading

chqrlie commented Sep 9, 2024 •

edited

Loading

zeromake commented Sep 10, 2024 •

edited

Loading