Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

build: initial support for meson build system #902

Closed
wants to merge 7 commits into from

Conversation

mochaaP
Copy link
Contributor

@mochaaP mochaaP commented Feb 8, 2025

Initial support for meson build system.
I plan to maintain this port continuously alongside cmake.

suppoted features

xrefs

mesonbuild/wrapdb#1865
#761

tested

  • linux gcc
  • mingw gcc (cross)
  • mingw gcc (msys2)
  • mingw clang (msys2)
  • msvc (cross)
  • clang-cl
  • apple clang

meson.build Outdated
qjsc_exe = executable(
'qjsc',
qjsc_srcs,
qjs_libc_srcs,
Copy link

Choose a reason for hiding this comment

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

This will cause all of these sources to be re-compiled for the executable, but then it seems like you link them back in via the qjs_dep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

qjs_libc_srcs == [] if -Dlibc=true, since it's already compiled in the main qjs_lib.
this variable is here to explicitly add quickjs-libc to required targets. is it better to use qjs_libc_lib = static_library(...) and then add link_whole: qjs_libc_lib to qjs_lib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied the qjs_libc_lib thing.

@dcbaker
Copy link

dcbaker commented Feb 8, 2025

I saw your comments on the wrap proposal, and thought I might take a peak at what you where working on.

@saghul
Copy link
Contributor

saghul commented Feb 8, 2025

Prior art: #761

@mochaaP mochaaP force-pushed the mochaa branch 2 times, most recently from 24aba70 to 42cb912 Compare February 8, 2025 20:09
@trim21
Copy link

trim21 commented Feb 8, 2025

I'm the author of mesonbuild/wrapdb#1865 . Then initial motivation is that we do not want to add meson.build in this project. If we are going to support meson build system natively I'll close mesonbuild/wrapdb#1865 because it's unnecessary

@saghul
Copy link
Contributor

saghul commented Feb 8, 2025

I'm the author of mesonbuild/wrapdb#1865 . Then initial motivation is that we do not want to add meson.build in this project. If we are going to support meson build system natively I'll close mesonbuild/wrapdb#1865 because it's unnecessary

Personally I'm still on the fence about adding Meson support here. If wrapdb is there then users already have a good way forward.

@bnoordhuis
Copy link
Contributor

I'm mildly negative on the changes to the Makefile. A meson build file, okay, but only if I don't have to look at it or think about it too much. For testing it, I'd add a CI job.

@dcbaker
Copy link

dcbaker commented Feb 8, 2025

From Meson's perspective we would prefer if meson.build files could live upstream, even with a lower level of support that $main_build_system. WrapDB can still reference projects with upstream meson.build files to make meson wrap install quickjs-ng work seemlessly.

@dcbaker
Copy link

dcbaker commented Feb 8, 2025

LGTM from a meson is correct point of view

@saghul
Copy link
Contributor

saghul commented Feb 9, 2025

@bnoordhuis whats your appetite towards this? I'm somewhere around -0.5

@chqrlie
Copy link
Collaborator

chqrlie commented Feb 9, 2025

I'm mildly negative on the changes to the Makefile. A meson build file, okay, but only if I don't have to look at it or think about it too much. For testing it, I'd add a CI job.

Regarding changes to the Makefile and a choice of a build option, I am nor negative but I would favor an option to build directly from Makefile on selected targets, without CMake :)

@saghul
Copy link
Contributor

saghul commented Feb 9, 2025

@chqrlie Please let's keep this about meson.

@bnoordhuis
Copy link
Contributor

@bnoordhuis whats your appetite towards this? I'm somewhere around -0.5

I'm okay with merging it provided it's close-to-zero effort to maintain. I don't care for it myself but there's no denying meson is pretty popular nowadays.

Having said that, since wrapdb exists, is there a compelling reason to have the build file in this repo?

@dcbaker
Copy link

dcbaker commented Feb 9, 2025

Having them upstream means that your users can use git snapshots and git submodules as subprojects, that’s not possible with meson files hosted in wrapdb

@bnoordhuis
Copy link
Contributor

That's a good reason. @saghul, up to you.

@saghul
Copy link
Contributor

saghul commented Feb 10, 2025

Alright, let's give it a try. I'll give it a deeper look this week. What this PR is missing is a few CI jobs, to make sure this thing works at all :-)

@mochaaP
Copy link
Contributor Author

mochaaP commented Feb 10, 2025

sure, i will add it

@mochaaP
Copy link
Contributor Author

mochaaP commented Feb 10, 2025

should I add this to a separate workflow or just ci.yml?

@saghul
Copy link
Contributor

saghul commented Feb 10, 2025

Same file, new jobs.

@mochaaP mochaaP force-pushed the mochaa branch 2 times, most recently from 3c67859 to d5deb43 Compare February 10, 2025 11:41
@mochaaP
Copy link
Contributor Author

mochaaP commented Feb 10, 2025

clang sanitized builds fail with: runtime error: call to function js_realloc_rt through pointer to incorrect function type 'void *(*)(void *, void *, unsigned long)

@saghul
Copy link
Contributor

saghul commented Feb 10, 2025

In quickjs-libc?

Any way to get more output from the meson runners? They seem to output to a file.

@bnoordhuis
Copy link
Contributor

Sticking to the default limit seems best/most appropriate.

@saghul
Copy link
Contributor

saghul commented Mar 3, 2025

@mochaaP can you please apply the fix above?

@mochaaP
Copy link
Contributor Author

mochaaP commented Mar 4, 2025

sure.

@mochaaP mochaaP force-pushed the mochaa branch 2 times, most recently from 0bd07a5 to f68504b Compare March 4, 2025 03:24
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.

LGTM! Thanks for the perseverance!

@bnoordhuis do you want to take another look?

@mochaaP
Copy link
Contributor Author

mochaaP commented Mar 4, 2025

uhh, and now Win32 builds fails?

@saghul
Copy link
Contributor

saghul commented Mar 4, 2025

What the... I'll take a look tonight :-/

@saghul
Copy link
Contributor

saghul commented Mar 7, 2025

This fixes the Windows build: #954

@saghul
Copy link
Contributor

saghul commented Mar 7, 2025

@mochaaP Can you please rebase?

@saghul
Copy link
Contributor

saghul commented Mar 7, 2025

Aside from the rebase the same stack size flags need to be passed on to the linker.

@mochaaP
Copy link
Contributor Author

mochaaP commented Mar 8, 2025

all checks passed. 🥳
https://github.com/mcha-forks/quickjs/actions/runs/13732211416

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.

Great work! I'll merge and cut the release later tonight!

args: -Dlibc=true
- name: mimalloc
args: -Dcli_mimalloc=auto
- name: jscheck
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oooh i think i accidently forgot some leftovers from testing. cleaning it.

@saghul
Copy link
Contributor

saghul commented Mar 8, 2025

Squashed and merged in #957

@saghul saghul closed this Mar 8, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants