Skip to content
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

Meson build system support #159

Merged
merged 11 commits into from
Feb 12, 2025
Merged

Meson build system support #159

merged 11 commits into from
Feb 12, 2025

Conversation

bgs99
Copy link
Collaborator

@bgs99 bgs99 commented Feb 1, 2025

This adds meson as an alternative build system.

There's only a single configuration option, build_tests, that controls whether the tests are built, enabled by default and used only if the project is not built as a sub-project to mirror the CMake behavior.

When the tests are enabled, pulls googletest either from the system or from source release with WrapDB patches (see https://mesonbuild.com/Wrapdb-projects.html) - if needed, that can be changed by modifying the suprojects/gtest.wrap file.

@bgs99 bgs99 requested a review from bugdea1er February 1, 2025 18:27
@bgs99 bgs99 force-pushed the meson branch 9 times, most recently from 544f5ed to 1f8d903 Compare February 2, 2025 14:05
Comment on lines 13 to 16
#elif defined TMP_BUILDING_DLL || defined TMP_SHARED
#define TMP_EXPORT __attribute__((visibility("default")))
#define TMP_NO_EXPORT __attribute__((visibility("hidden")))
#else
#define TMP_EXPORT
#define TMP_NO_EXPORT
#endif

Choose a reason for hiding this comment

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

Note that on Linux it is never problematic to define the visibility attributes even for static builds.

meson.build Outdated
gnu_symbol_visibility: 'hidden',
version: meson.project_version(),
dependencies: [std_fs_dep],
cpp_shared_args: ['-DTMP_BUILDING_DLL'],

Choose a reason for hiding this comment

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

... which means you can choose to pass this define only if host_machine.system() == 'windows', and therefore allow linux / macOS to share objects for both static/shared.

Comment on lines 1 to 5
gtest_main_dep = dependency('gtest', main: true, required: false)
if not gtest_main_dep.found()
gtest_proj = subproject('gtest')
gtest_main_dep = gtest_proj.get_variable('gtest_main_dep')
endif

Choose a reason for hiding this comment

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

This seems complicated. A subproject fallback will occur automatically if the dependency is required, so you can do:

gtest_main_dep = dependency('gtest_main', 'gtest', main: true)

You need to specify both dependency names with the preferred one (gtest_main) first, so that you get a fallback to it via the wrap file. The main: true is ignored when trying gtest_main, but is used for gtest (so that meson will use its internal lookup method when pkg-config is not available). Long term, meson needs to deprecate that kwarg and consolidate on "gtest_main" because the current state of affairs is confusing...

gtest_main_dep = gtest_proj.get_variable('gtest_main_dep')
endif

unit_test = executable(

Choose a reason for hiding this comment

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

You can add build_by_default: false to avoid building the test binary when not running the tests:

https://mesonbuild.com/Release-notes-for-1-7-0.html#test-targets-no-longer-built-by-default

This may also make it tempting to avoid having a dedicated project option just to control tests.

Choose a reason for hiding this comment

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

The dedicated option is still useful to avoid requiring test-only dependencies when building the project without tests, for example for packaging. One could maybe make the dependency optional and somehow error gracefully when tests are build, but that seems harder and more error-prone than disabling the tests directory from configuring.

I guess this is one of the reasons BUILD_TESTING is built-in in CMake's CTest module
https://cmake.org/cmake/help/latest/module/CTest.html

Excluding it from all though is definitely a good idea, thanks!

Copy link

@eli-schwartz eli-schwartz Feb 4, 2025

Choose a reason for hiding this comment

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

I'd normally say a test option makes sense to avoid test-only dependencies, yeah. But in this case you always use a subproject fallback which is an additional factor?

If you're going to use an option though, the above rewrite into pure dependency() that I suggested above, makes it simple to call into gtest exactly once, and thereby unlock the ability to do this:

gtest_main_dep = dependency(....., required: get_option('tests'), allow_fallback: true)
if not gtest_main_dep.found()
    subdir_done()
endif

The tests option, when using a meson "feature" option, is a tristate. When configuring with:

  • -D tests=enabled testing is mandatory, fallback to subprojects is enabled (allow_fallback is implicitly true by default)
  • -D tests=disabled meson will print "Dependency gtest_main skipped: feature tests is disabled" and the .found() will evaluate as false
  • -D tests=auto testing is optional, and if gtest is installed, or allow_fallback is true + the subproject is fetchable (e.g. the "nodownload" wrap node isn't set), it will be found and used but otherwise it is simply not found and the build gracefully falls back

Excluding it from all though is definitely a good idea, thanks!

Thinking about the cmake comparison here... this may not be commonly expected knowledge (to exclude from all) because a lot of people have run into a cmake quirk here that makes them shy away from this regardless of build system.

Namely, cmake can exclude test programs from all but if you do then tests fail because ctest won't build them. Just fail with "file not found" :D

Meson always builds test executables on demand though, e.g. meson test mytest1 mytest2 will (regardless of the 1.7.0 changes in how all is handled) read the build data and determine which targets are annotated as dependencies of that subset of tests, and explicitly run ninja to ensure they are up to date. You have to pass the --no-rebuild option if you want to skip checking that they are up to date (presumably because you are positive you handled it already in a previous packaging phase).

Copy link
Owner

@bugdea1er bugdea1er left a comment

Choose a reason for hiding this comment

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

It seems the configuration does not set the required C++ standard, I get the following errors on macOS:

ninja: Entering directory `/Users/bugdealer/Developer/tmp/build'
[1/10] Compiling C++ object libtmp.2.dylib.p/src_file.cpp.o
FAILED: libtmp.2.dylib.p/src_file.cpp.o 
c++ -Ilibtmp.2.dylib.p -I. -I.. -Iinclude -I../include -fvisibility=hidden -fdiagnostics-color=always -D_LIBCPP_ENABLE_ASSERTIONS=1 -Wall -Winvalid-pch -O0 -g -DTMP_BUILDING_DLL -MD -MQ libtmp.2.dylib.p/src_file.cpp.o -MF libtmp.2.dylib.p/src_file.cpp.o.d -o libtmp.2.dylib.p/src_file.cpp.o -c ../src/file.cpp
In file included from ../src/file.cpp:1:
../include/tmp/entry:24:23: error: no member named 'filesystem' in namespace 'std'
   24 |   operator const std::filesystem::path&() const noexcept;
      |                  ~~~~~^
...

@bgs99 bgs99 force-pushed the meson branch 8 times, most recently from 1fac9f1 to 6c612ea Compare February 12, 2025 05:38
meson.build Outdated
include_directory = include_directories('include')
cxx = meson.get_compiler('cpp')

std_fs_dep = cxx.find_library('stdc++fs', required: false)
Copy link
Owner

Choose a reason for hiding this comment

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

Can we add libc++ support in the same way?

Choose a reason for hiding this comment

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

That's usually done via -stdlib=libc++ (e.g. add_project_arguments or user-defined via CXXFLAGS / LDFLAGS). It does not suffice to simply compile with GCC's builtin libstdc++ headers and then link to clang's libc++.so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are talking about libc++fs as a libc++'s libstd++fs counterpart.

In the end I have decided to copy the CMake "try to compile and link" approach as otherwise it would still attempt to link with (and include in pkgconfig requirements) both filesystem impls if they are both present.

The relevant issue for CMake is https://gitlab.kitware.com/cmake/cmake/-/issues/17834

I have not seen a similar discussion on meson's github, maybe I should open one myself.

Copy link

@eli-schwartz eli-schwartz Feb 12, 2025

Choose a reason for hiding this comment

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

Oh right, sorry. :o

I think this would probably be fine to add for meson. e.g. dependency('cxx-filesystem') (no clue what the name should be) could call into the detected C++ compiler by name and version and "which stdlib is in use" (we already need to know that in order to handle multi-language targets with a different link_language) and figure out what library is needed (for older compilers!), or if it is supported at all. We do something similar with dependency('threads').

Please feel free to open a ticket.

@bgs99 bgs99 force-pushed the meson branch 2 times, most recently from 7224095 to a72aea5 Compare February 12, 2025 19:23
@bugdea1er bugdea1er merged commit 4d4da5e into main Feb 12, 2025
15 checks passed
@bugdea1er bugdea1er deleted the meson branch February 12, 2025 21:09
@bugdea1er
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants