Skip to content

CMake/meson fixes, Windows support for shared lib build #1225

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
Open
BalkanMadman wants to merge 6 commits into
base: master
Choose a base branch
from
Open

CMake/meson fixes, Windows support for shared lib build #1225

BalkanMadman wants to merge 6 commits into from
+452 −336

Conversation

@BalkanMadman
Copy link

@BalkanMadman BalkanMadman commented Nov 1, 2025

Hello!

We've been packaging QuickJS-ng in Gentoo and we've found meson to be the most versatile amongst the added build systems. Huge thanks for taking care of QuickJS and implementing modern build systems -- it makes the life of us, packagers, much much easier.

This PR bundles two simple fixes for meson.build

saghul
saghul reviewed Nov 1, 2025
View reviewed changes
@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 4, 2025 •
edited
Loading

As far as I'm aware, the CI failures are not caused by these changes

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 18, 2025

Ping. Any updates?

@saghul
Copy link
Contributor

saghul commented Nov 18, 2025

I've triggered the CI again. IIRC the failures were directly related to this change, but let's see.

@bnoordhuis
Copy link
Contributor

bnoordhuis commented Nov 18, 2025 •
edited
Loading

I'm fine with this change. @saghul?

edit: cross-wired

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 19, 2025

I've been going through CI and stumbled upon this: https://github.com/quickjs-ng/quickjs/actions/runs/19475324220/job/55744866380?pr=1225#step:7:42

meson.build:624: WARNING: Trying to use ['address', 'undefined'] sanitizer on Clang with b_lundef.

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 19, 2025

I can reproduce the failure on my machine too (albeit with clang only).

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 19, 2025

https://github.com/google/sanitizers/wiki/AddressSanitizer#faq:

Q: When I link my shared library with -fsanitize=address, it fails due to some undefined ASan symbols (e.g. asan_init_v4)?

A: Most probably you link with -Wl,-z,defs or -Wl,--no-undefined. These flags don't work with ASan unless you also use -shared-libasan (which is the default mode for GCC, but not for Clang).

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 19, 2025

Meson option b_lundef defaults to true, e.g. -Wl,--no-undefined is added to CFLAGS.

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 19, 2025

Passing -shared-libasan seems to fix the error.

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 19, 2025

Should fix the CI.

@saghul
Copy link
Contributor

saghul commented Nov 19, 2025

Yeah so as I said, the meson CI needs updating now to statically link by default.

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 21, 2025

Would you like to me to do the CI update (including the fix for the failing CI for this PR) in this PR or separately?

@saghul
Copy link
Contributor

saghul commented Nov 21, 2025

Would you like to me to do the CI update (including the fix for the failing CI for this PR) in this PR or separately?

In this PR please, we like to avoid merging failing PRs.

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 22, 2025 •
edited
Loading

Would you like to me to do the CI update (including the fix for the failing CI for this PR) in this PR or separately?

In this PR please, we like to avoid merging failing PRs.

Your release CI builds QuickJS-NG with CMake so this PR shouldn't affect that. The PR already contains a fix for PR CI. My apologies, screwed up the quoting

@BalkanMadman
Copy link
Author

BalkanMadman commented Nov 22, 2025

Apparently, for MSVC CI, we're hitting mesonbuild/meson#13892. Will add a commit forcing MSVC-based builds to use static libs.

@bnoordhuis
Copy link
Contributor

bnoordhuis commented Dec 20, 2025

@BalkanMadman ping, seems this was close to done?

@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 24, 2025

Hello! Sorry for the delay, could you please try running the CI again? The two new commits should fix two issues in building QuickJS-NG shared.

BalkanMadman
BalkanMadman commented Dec 24, 2025
View reviewed changes
@BalkanMadman BalkanMadman force-pushed the meson-fixes branch 2 times, most recently from 5b8d06a to 0cf1f7f Compare December 24, 2025 08:54
@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 24, 2025 •
edited
Loading

Okay, I also need to guard all the dll{export,import} shenanigans if we're building static libqjs🔥🔥🔥

Because obviously just exporting symbols isn't enough and on Windows you need separate exports for DLLs and static libs👍

@saghul
Copy link
Contributor

saghul commented Dec 24, 2025

Not sure I get the unconditional define in quickjs.c. Shouldn't it be set by Meson depending on the build type?

@saghul
Copy link
Contributor

saghul commented Dec 26, 2025

Looks like there are conflicts, can you rebase?

@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 27, 2025 •
edited
Loading

Now, we pass the qjs defines to cutils too.

Please, give the CI one more go.

@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 1, 2026

@saghul another CI run perhaps?

BalkanMadman
BalkanMadman commented Jan 2, 2026
View reviewed changes
@BalkanMadman BalkanMadman force-pushed the meson-fixes branch 3 times, most recently from fc94337 to fc5eee8 Compare January 3, 2026 19:44
@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 3, 2026

Okay, the last CI failure is trivial

@saghul
Copy link
Contributor

saghul commented Jan 4, 2026

This PR has ballooned in size so I'll need to review it again...

@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 4, 2026

I'm going to split 309ba5b into two logical commits, fixing the CI failure. That should make the PR a bit easier to review.

@saghul
Copy link
Contributor

saghul commented Jan 4, 2026

I don't quite like the addition of that header.

Perhaps we can consider not undefining the XXX_EXTERN macro in quickjs.h, so we can use it in quickjs-libc.h.

@bnoordhuis thoughts?

@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 4, 2026

zlib uses a separate header IIRC. If you'd like, we can put the definitions in quickjs.h instead.

@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 4, 2026

What I mean, is that removing the undefs should work as well.

@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 4, 2026

zlib does it like this: https://github.com/madler/zlib/blob/develop/zconf.h#L337

@BalkanMadman
CMake/meson: Unbundle cutils from libqjs as a static lib
e7d8403 
Functions declared by cutils.h are used by most of the source files in
the project. However, they are not part of libqjs API and, thus, the
functions are not marked as API.

Apart from the fact that cutils do not belong in libqjs, since they do
not constitute a public API and are just helpers, they break shared
qjs.dll builds on Windows where DLLs must explicitly mark their
interfaces.

As cutils.h does not mark its functions with JS_EXTERN or alike, the
Windows linker is unable to resolve references to the cutils functions.

This commit makes cutils a separate static library, which is linked with
the dependent executables/libraries.

Signed-off-by: Zurab Kvachadze <zurabid2016@gmail.com>
@BalkanMadman
meson: Do not build quickjs-libc as static library if -Dlibc=true
71cf78c 
Previously, irrespective of the setting of -Dlibc, quickjs-libc would be
built as a static library. In case -Dlibc=true is set, the static
library is linked in libqjs only. In case -Dlibc=false is set, the
archive is linked to every quickjs-libc consumer.

In the former case (-Dlibc=true), building quickjs-libc as static
library introduces useless indirection, since quickjs-libc is going to
be linked only to libqjs anyway. CMake just adds the libc's sources to
qjs's and calls it a day.

This commit changes meson.build. If -Dlibc=true is set, quickjs-libc's
sources are compiled as a part of libqjs. If not, it compiled as a
static library, as was done before.

In both cases, a direct dependency of quickjs-libc has been changed into
a `dependency` on qjs_libc_dep which conveniently abstracts both
configuration.

So, if quickjs-libc needs to be linked to, just add `dependencies:
qjs_libc_dep`.

Signed-off-by: Zurab Kvachadze <zurabid2016@gmail.com>
@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 4, 2026

With how quickjs-libc is built right now, it feels to me like having separate header is the least fragile solution (quickjs-libc.h includes quickjs.h which can mess the definition of QUICKJS_NG_API/JS_EXTERN up for the symbols in both)
Please tell me if you'd still like me to remove quickjs-ng-conf.h and move the defines into quickjs.h.

@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 4, 2026

Additionally (which can also be done as part of this PR), the cleaner solution with libc on CMake would be to build it as a static lib, the same as it's done in Meson, if -DQJS_BUILD_LIBS=false. That may avoid mistakes and looks cleaner than, for example, linking every dependency of quickjs-libc to its consumer manually:
https://github.com/BalkanMadman/quickjs/blob/c34ef9494031a23903a52d367e66cdcbcb2c3d7a/CMakeLists.txt#L238

saghul
saghul reviewed Jan 5, 2026
View reviewed changes
Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Left some comments, I think this PR has ballooned from the original intent, and it should be refocused.

I'd like to hear Ben's thoughts on whether we add the extra header or not. I'm softly leaning towards the no.

@BalkanMadman
CMake: Build quickjs-libc as a static lib if -DQJS_BUILD_LIBC=false
8143aa9 
Previously, if QJS_BUILD_LIBC was false, quickjs-libc would be built
with every consumer. This made it harder to specify and trace the
dependencies/compiler definitions of quickjs-libc itself.

With this change, if -DQJS_BUILD_LIBC=false is passed to CMake,
quickjs-libc is built as a normal static library, with its own compiler
definitions and dependencies.

This changes CMake to match the Meson behaviour. Additionally, since
CMake does not allow mixing keyword and non-keyword declarations, this
commit adds explicit `PRIVATE` to all `target_link_libraries`
declarations.

Signed-off-by: Zurab Kvachadze <zurabid2016@gmail.com>
@BalkanMadman
Consolidate JS_EXTERN, js_force_inline in quickjs.h, add Windows support
b2e44c6 
Since the macros like JS_EXTERN are used in more than one place, it is
quite reasonable and helpful to declare them in one place. Additionally,
the Windows support with the __declspec tango was missing.

This commit, in addition to reorganising definitions, also plumbs
Windows support for the renamed QJS_EXTERN, QJS_LIBC_EXTERN and
QJS_FORCE_INLINE.

Signed-off-by: Zurab Kvachadze <zurabid2016@gmail.com>
@BalkanMadman
ci/meson: Set -Db_lundef=false when building with sanitisers under Clang
202fbf9 
Clang cannot handle building shared libraries with sanitizers and
-Wl,--no-undefined (set by default unless explicitly disabled with
-Db_lundef=false).

This commit prefixes CI in case shared libraries are built with
sanitisers.

Signed-off-by: Zurab Kvachadze <zurabid2016@gmail.com>
@BalkanMadman
meson: Do not hardcode default_library=static
0ec4bae 
During the build, the default library can be overridden via the
-Ddefault_library=type flag.

Presetting this key in meson.build makes life harder for distributions
which almost always want to build shared libraries. Those requiring
static libraries can always force that via the aforementioned flag.

Signed-off-by: Zurab Kvachadze <zurabid2016@gmail.com>
@BalkanMadman
Copy link
Author

BalkanMadman commented Jan 5, 2026 •
edited
Loading

Removed the separate header, moved the macros decls into quickjs.h. Additionally, added a commit that build quickjs-libc as a static library on CMake if -DQJS_BUILD_LIBC=false is passed. The latter simplifies dependency management for quickjs-libc and makes CMake match Meson's behaviour.

@BalkanMadman BalkanMadman requested a review from saghul January 5, 2026 14:46
@BalkanMadman BalkanMadman changed the title meson.build fixes CMake/meson fixes, Windows support for shared lib build Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@saghul saghul Awaiting requested review from saghul

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BalkanMadman @saghul @bnoordhuis