CMake/meson fixes, Windows support for shared lib build #1225
Conversation
|
As far as I'm aware, the CI failures are not caused by these changes |
|
Ping. Any updates? |
BalkanMadman
force-pushed
the
meson-fixes
branch
from
82ecdd0 to
3aec58b
Compare
|
I've triggered the CI again. IIRC the failures were directly related to this change, but let's see. |
|
I'm fine with this change. @saghul? edit: cross-wired |
|
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 |
|
I can reproduce the failure on my machine too (albeit with clang only). |
|
https://github.com/google/sanitizers/wiki/AddressSanitizer#faq:
|
|
Meson option |
|
Passing |
BalkanMadman
force-pushed
the
meson-fixes
branch
from
3aec58b to
7b63c89
Compare
|
Should fix the CI. |
|
Yeah so as I said, the meson CI needs updating now to statically link by default. |
|
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. |
|
Apparently, for MSVC CI, we're hitting mesonbuild/meson#13892. Will add a commit forcing MSVC-based builds to use static libs. |
|
@BalkanMadman ping, seems this was close to done? |
BalkanMadman
force-pushed
the
meson-fixes
branch
from
7b63c89 to
6cd329f
Compare
|
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
force-pushed
the
meson-fixes
branch
2 times, most recently
from
5b8d06a to
0cf1f7f
Compare
|
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👍 |
|
Not sure I get the unconditional define in quickjs.c. Shouldn't it be set by Meson depending on the build type? |
BalkanMadman
force-pushed
the
meson-fixes
branch
from
0cf1f7f to
d0fe4c0
Compare
|
Looks like there are conflicts, can you rebase? |
BalkanMadman
force-pushed
the
meson-fixes
branch
from
d0fe4c0 to
704fed0
Compare
BalkanMadman
force-pushed
the
meson-fixes
branch
2 times, most recently
from
fc94337 to
fc5eee8
Compare
|
Okay, the last CI failure is trivial |
BalkanMadman
force-pushed
the
meson-fixes
branch
from
fc5eee8 to
1413dfd
Compare
|
This PR has ballooned in size so I'll need to review it again... |
|
I'm going to split 309ba5b into two logical commits, fixing the CI failure. That should make the PR a bit easier to review. |
|
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? |
|
zlib uses a separate header IIRC. If you'd like, we can put the definitions in |
|
What I mean, is that removing the undefs should work as well. |
|
zlib does it like this: https://github.com/madler/zlib/blob/develop/zconf.h#L337 |
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
force-pushed
the
meson-fixes
branch
from
1413dfd to
a27b42e
Compare
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
force-pushed
the
meson-fixes
branch
from
a27b42e to
c34ef94
Compare
|
With how |
|
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 |
saghul
left a comment
saghul
left a comment
There was a problem hiding this comment.
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.
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
force-pushed
the
meson-fixes
branch
2 times, most recently
from
ffe5a50 to
0ec4bae
Compare
|
Removed the separate header, moved the macros decls into |
BalkanMadman
changed the title
BalkanMadman
force-pushed
the
meson-fixes
branch
from
0ec4bae to
8687273
Compare
| #endif /* QUICKJS_NG_PLAT_WIN32 */ | ||
|
|
||
| /* | ||
| * `JS_LIBC_EXTERN` -- helper macro that must be used to mark the extern |
There was a problem hiding this comment.
Is there a problem if we don't use this and simply use JS_EXTERN directly in the libc?
There was a problem hiding this comment.
We can't do that when libc isn't built as a part of shared qjs.dll on Windows.
In this case, in quickjs-libc, we need JS_LIBC_EXTERN to be empty, while JS_EXTERN is __declspec(dllexport) (for qjs) or __declspec(dllimport) (for qjs consumers). Both break libc's builds on Windows.
For all other cases, it doesn't make much sense to duplicate all the logic.
There was a problem hiding this comment.
What I mean, is that we need to have some additional logic for specifically the libc version of JS_EXTERN in any case.
There was a problem hiding this comment.
And, to me, overloading JS_EXTERN for that doesn't seem to be the most ergonomic solution.
There was a problem hiding this comment.
Not sure I fully follow. Both when built as static and shared library the libc is part of the core library build so it should follow the same rules.
There was a problem hiding this comment.
What do you mean? Libc is always built. Either as a static lib or as part of static/shared qjs. If qjs is shared and we're building libc as a static lib, we cant decine JS_LIBC_EXTERN to JS_EXTERN because the latter will be __declspec(dllexport) or __declspec(dllimport) when the former must be empty.
There was a problem hiding this comment.
Currently it's either built as part of the qjs library or other executables, never as a standalone library itself.
There was a problem hiding this comment.
Even if it's built as part of the executables, the issue is the same.
There was a problem hiding this comment.
Sorry I might be a bit obtuse today 😅
Can you walk me through the problem? Maybe show me how the failure looks like.
There was a problem hiding this comment.
Sure, I specifically installed a dev env on Windows to understand why the CI was failing. I'm not at my working machine rn, once I get there, I'll do a walkthrough.
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>
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>
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
force-pushed
the
meson-fixes
branch
from
8687273 to
209e830
Compare
3 participants
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