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: add support for testsuite #701

Merged
merged 7 commits into from
Feb 9, 2022
Merged

Conversation

eli-schwartz
Copy link
Contributor

As with CMakeLists.txt, this is disabled by default.

  • I have not implemented the examples/ directory because it uses some output regex feature to run the tests, and meson doesn't have any similar feature, so that would probably require some kind of wrapper program that runs the test binary and compares output.
  • I have not enabled doxygen documentation building, because it doesn't seem very useful in the same way that running tests under meson is; also, setting the output directory to be in the builddir requires either a python script to edit Doxyfile, or for Doxyfile to set @VARNAME@ that can be replaced (breaking usage without first running cmake or meson) or for me to stop being lazy and finally implement doxygen in a meson module. :D
  • I have not enabled CUDA tests, because I'm not sure if those are important?
  • Testing CLI11_SINGLE_FILE doesn't seem very useful, because it should behave exactly the same anyway, modulo speed of e.g. deduplicated includes
  • There are some features which I didn't bother to implement, because meson supports it out of the box:
    • sanitizers can be configured with meson using -Db_sanitize=, possible values include address, thread, undefined, memory, address,undefined. AFAIK this functionality is not tested for cmake in CI anyway.
    • a clang-tidy command is automatically set up by meson if the clang-tidy program is installed.
    • code coverage can be configured with meson using -Db_coverage=true

@eli-schwartz
Copy link
Contributor Author

Motivation for the PR: I'd like to add these tests to the https://wrapdb.mesonbuild.com/ release integration for CLI11.

Currently meson is not directly tested in this project's own CI, but tests/mesonTest/ is built using CLI11 as a subproject. This can probably be obsoleted in favor of directly building the main CLI11 project with meson, and configuring with -Dtests=true.

The tests pass locally (with the optional boost dependency installed). Building succeeds with warnings (I did not check the cmake build to see if there are warnings there too):

$ ninja -C builddir/ 
ninja: Entering directory `builddir/'
[3/47] Compiling C++ object tests/link_test_2.p/link_test_2.cpp.o
In file included from ../tests/link_test_2.cpp:8:
../include/CLI/Timer.hpp:114:12: warning: ‘CLI::Timer& CLI::Timer::operator/(std::size_t)’ should return by value [-Weffc++]
  114 |     Timer &operator/(std::size_t val) {
      |            ^~~~~~~~
[8/47] Compiling C++ object tests/liblink_test_1.so.p/link_test_1.cpp.o
In file included from ../tests/link_test_1.cpp:8:
../include/CLI/Timer.hpp:114:12: warning: ‘CLI::Timer& CLI::Timer::operator/(std::size_t)’ should return by value [-Weffc++]
  114 |     Timer &operator/(std::size_t val) {
      |            ^~~~~~~~
[39/47] Compiling C++ object tests/TimerTest.p/TimerTest.cpp.o
In file included from ../tests/TimerTest.cpp:7:
../include/CLI/Timer.hpp:114:12: warning: ‘CLI::Timer& CLI::Timer::operator/(std::size_t)’ should return by value [-Weffc++]
  114 |     Timer &operator/(std::size_t val) {
      |            ^~~~~~~~
[47/47] Linking target tests/BoostOptionTypeTest

@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

Great, thanks for working on the Meson code. I've been meaning to do a bit more with Meson. Would it be possible to add a single test to the GHA CI running on Meson? I guess I could try it...

@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

I tried:

$ pipx run meson setup build-meson .
$ pipx run meson configure build-meson -Dtests=true
$ pipx run meson compile build-meson

(removed mistakes and lots of help calls) and I got:

Traceback (most recent call last):
  File "/Users/henryschreiner/.local/pipx/.cache/0bca3ad613ead43/lib/python3.10/site-packages/mesonbuild/mesonmain.py", line 146, in run
    return options.run_func(options)
  File "/Users/henryschreiner/.local/pipx/.cache/0bca3ad613ead43/lib/python3.10/site-packages/mesonbuild/mcompile.py", line 325, in run
    cdata = coredata.load(options.wd)
  File "/Users/henryschreiner/.local/pipx/.cache/0bca3ad613ead43/lib/python3.10/site-packages/mesonbuild/coredata.py", line 1033, in load
    with open(filename, 'rb') as f:
FileNotFoundError: [Errno 2] No such file or directory: '/Users/henryschreiner/git/software/CLI11/meson-private/coredata.dat'

ERROR: Unhandled python exception

    This is a Meson bug and should be reported!

🤦

@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

Oh, maybe the compile command doesn't take a build directory like everything else, and forces me to switch to the build directly manually like an animal. Fine, will try that.

$ cd build-meson
$ pipx run meson compile
[0/1] Regenerating build files.
The Meson build system
Version: 0.61.1
...
../tests/meson.build:57:18: ERROR: Expecting rbracket got comma.
    ['link_test_2', {'link_with': link_test_lib}]
                  ^

tests/meson.build Outdated Show resolved Hide resolved
@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

Okay, now at:

Run-time dependency catch2 found: NO (tried pkgconfig, framework and cmake)

Guessing this doesn't download catch2 like CMake does? Did a brew install catch2 and it's compiling. CMake sets the includes as SYSTEM includes, is there a way to do that with Meson? Not really very interested in seeing Catch2 & Boost warnings.

@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

As with CMakeLists.txt, this is disabled by default.

This is enabled by default if you are building this as the main project. It's only off by default if this is not the main project.

I found include_type: 'system'. CMake correctly adds -isystem /usr/local/include. Meson does not. Looks like this bug: mesonbuild/meson#8755. This is slamming my screen with boost warnings wrapped in catch2 code, making it pretty much impossible to see if it's producing any other warnings.

@eli-schwartz
Copy link
Contributor Author

Oh, maybe the compile command doesn't take a build directory like everything else, and forces me to switch to the build directly manually

Actually, like ninja or make it expects you to specify a directory using -C build-meson/ and any regular positional argument is treated as the name of a target to build, not a build directory to chdir into

Guessing this doesn't download catch2 like CMake does?

That can be made available, just a second...

This is slamming my screen with boost warnings wrapped in catch2 code, making it pretty much impossible to see if it's producing any other warnings.

Huh, I did not get any warnings... this was on Linux with Boost 1.78.0

…code

This would implicitly default to false, so if something bizarre happened
and the command errored out, meson would consider that fine. Now meson
emits a warning about this deprecated legacy behavior, suggests that it
will eventually change, and, most importantly, prevents a warning-free
build.

Suppress the warning by manually specifying the sensible behavior, which
is to fail on errors.
Produced by running `meson wrap install catch2` and checking the results
into git.

No modifications to the build files are expected; this makes use of
https://mesonbuild.com/Wrap-dependency-system-manual.html#provide-section
@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

From the bug report linked, sounds like it's specific to brew + macOS (which is what I'm on). If you can get the download working, I can add the CI (basically have it added).

@eli-schwartz
Copy link
Contributor Author

The catch2 download is now working.

@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

Actually, like ninja or make it expects you to specify a directory using -C build-meson/ and any regular positional argument is treated as the name of a target to build, not a build directory to chdir into

I'd have expected consistency across meson commands, rather than consistency with one (of three) build backends or a program that's not even a build backend for meson. ;) I'd also expect meson compile to be the more verbose but consistent way to build, while I can just use ninja directly if I want to cd around. But just an observation on my first try, not really "wrong". Though the error message could be better, something like "not a valid build directory", not "ERROR: Unhandled python exception, This is a Meson bug and should be reported!".

@eli-schwartz
Copy link
Contributor Author

eli-schwartz commented Feb 9, 2022

The terrible error message was a regression and got fixed by mesonbuild/meson@88f8a8e, it's on the milestone for the next bugfix. :D

(As expected, "This is a Meson bug" deserved reporting, got reported, and got fixed.)

@@ -0,0 +1,78 @@
catch2 = dependency('catch2')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for my understanding, this will download catch only if not found? And any version can be found, but 2.13.7 will be downloaded, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Based on the setting of --wrap-mode=default (you can set it to =forcefallback if you'd prefer to download it no matter what) it will:

  • try to find any version using pkg-config
  • try to find any version using cmake (by running a crafted CMakeLists.txt, seeing if find_package() works, and extracting that info using the trace parser)
  • download the pinned version available as a meson wrap

@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

sanitizers ... AFAIK this functionality is not tested for cmake in CI anyway.

Yeah, we set this up in pybind11, but I don't think it's ever been set up here. I need to move more CI to GHA before that happens, probably (you can see Appveyor is a bottleneck).

@eli-schwartz
Copy link
Contributor Author

Well, there are worse bottlenecks. :) Your PRs to meson, for example, will take roughly 2 hours to complete the slow Azure runs and GHA+Cygwin.

@henryiii
Copy link
Collaborator

henryiii commented Feb 9, 2022

If we are having a battle of bottlenecks, CMake takes nearly 3.5 hours to cross-compile the various PyPI packages on GHA. But this one I can workaround eventually - I haven't been very convincing in getting credits from Travis for the CMake one. :)

Thanks for the contribution! I'll try to look into that warning soon too (especially to see why it's passing our current CI), and I see a bit of CI cleanup I can do (we don't have submodules anymore). But this is good. @phlptp going to assume you are also fine with this, shout out if not!

@henryiii henryiii merged commit e8265f9 into CLIUtils:main Feb 9, 2022
@github-actions github-actions bot added needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Feb 9, 2022
@eli-schwartz eli-schwartz deleted the meson-test branch February 9, 2022 05:27
@henryiii henryiii removed the needs README Needs to be mentioned in the README label Feb 9, 2022
@henryiii henryiii removed the needs changelog Hasn't been added to the changelog yet label Mar 27, 2022
@neheb
Copy link

neheb commented Apr 17, 2022

CLI11-2.2.0/tests/meson.build:71:4: ERROR: Invalid use of addition: can only concatenate list (not "DependencyHolder") to list

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.

3 participants