Skip to content

meson.build fixes #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 4 commits into
base: master
Choose a base branch
from
Open

meson.build fixes #1225

BalkanMadman wants to merge 4 commits into from

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 26, 2025

Oh, yeah, give me a sec. After the rebase, could you please do the CI? I'm not sure whether __declspec(dllimport) is required with DLL consumers though..

@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 26, 2025

Okay, should be good

@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 26, 2025

084788f is not really complete or correct but I want to see whether the method even works.

@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 26, 2025 •
edited
Loading

It seems that to support both static and shared libqjs on Windows, we can avoid specifying __declspec(dllimport) since that works with both configurations.

Could you please restart the CI?

@BalkanMadman BalkanMadman force-pushed the meson-fixes branch 4 times, most recently from d1b42b0 to 9b3883e Compare December 27, 2025 09:11
@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 27, 2025

Okay, let's try it again. Could you please re-run the CI?

@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 27, 2025

Oh wait

@BalkanMadman
Copy link
Author

BalkanMadman commented Dec 27, 2025

Okay, should be good for an another CI run

@BalkanMadman
CMake/meson: Unbundle cutils from libqjs as a static lib
ea96a6d 
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 <[email protected]>
@BalkanMadman
Consolidate helper macros (JS_EXTERN, js_force_inline) in one header
4013acc 
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.

This commit, in addition to moving the macro definitions into the new
header quickjs-ng-util.h, also plumbs Windows support for the newly
introduced QUICKJS_NG_API and QUICKJS_NG_FORCE_INLINE.

The new header is installed with quickjs.h, since the latter depends on
the former.

Signed-off-by: Zurab Kvachadze <[email protected]>
@BalkanMadman
ci/meson: Set -Db_lundef=false when building with sanitisers under Clang
13ad0bd 
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 <[email protected]>
@BalkanMadman
meson.build: Do not hardcode default_library=static
b15601d 
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 <[email protected]>
@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.

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 saghul approved these changes

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