-
Notifications
You must be signed in to change notification settings - Fork 216
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.
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
base: master
Are you sure you want to change the base?
meson.build fixes #1225
Conversation
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>
| default_options: [ | ||
| 'c_std=gnu11,c11', | ||
| 'warning_level=3', | ||
| 'default_library=static', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the override work even if this is the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. However, the most use-cases I can think of (distributions, individuals) require shared libraries so hardcoding default_library=static seems like a bug to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that the default for meson is default_library=shared, I don't really see a reason to override it in meson.build. Does your CI require it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's likely why it failed.
In addition our CMake setup defaults to static builds, so Meson should align to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be okay with using meson defaults. cmake's default is more an accident of history that we inherit, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to build static libraries by default (in either CMake or meson)
In cmake it's controlled by a flag (-BUILD_SHARED_LIBS=ON or off), it's just that cmake defaults to static if not otherwise specified. Our CMakeListst.txt is agnostic as to whether it should produce static or shared libraries.
(Long-winded way of saying that whatever the cmake build does is not under discussion, it just is.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. The question is then does it make sense to build static libraries by default (in either CMake or meson)? I'd imagine most consumers of quickjs need shared libqjs.so rather than static libqjs.a.
I'd argue it's the opposite. Every tool I've seen out there installing JS engines like jsvu makes static builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, as far as I understand, tools installing JS engines mostly care about engine executables, not the provided lib. Obviously, the type of libqjs doesn't have influence on the executables.
For the consumers of the actual QuickJS-NG API, i.e. libqjs, it'd most likely be shared not static libraries they'd need.
In any case, that's not the hill to die on. I'm just arguing that, in my opinion, the consumers of your library API would prefer a shared lib, as with most other APIs out there.
The choice is up to you anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry my bad. As long as building the executable statically links the library by default I don't think I care much about what the default for building a library is.
We haven't called a 1.0 yet so I think we have plenty of wiggle room to try things out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the shared library is built, then the executable is linked dynamically to it via executable(..., dependencies: qjs_dep).
For CI, the static (or shared) lib build can always be forced via the cmdline switch -Ddefault_library=(static|shared)
|
As far as I'm aware, the CI failures are not caused by these changes |
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