-
Notifications
You must be signed in to change notification settings - Fork 259
yaml-cpp: clean-up meson build files #2428
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
Conversation
|
What I haven't checked if we can run unbundled gtest, and the tests don't run as part of the CI. |
7a47feb to
be5321b
Compare
be5321b to
583f3c8
Compare
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.
What I haven't checked if we can run unbundled gtest, and the tests don't run as part of the CI.
Can't we just configure this in ci_config.json? It should have always been enabled, as soon as the option was added, imo.
b252b76 to
10b004c
Compare
Tried but:
I don't care enough about the test to take the time to fix the tests - if you don't mind, I would merge as is and leave this open for someone else to fix. The PR already improve the quality of the wrap. |
"Cleaning up" is also not high priority to merge though, since it doesn't really affect things? But besides, I do think that the fact that part of this wrap doesn't successfully compile because it isn't enabled in CI is a good reason to not keep skipping a resolution. |
|
One thing this would solve is that currently this wrap does not work under Ubuntu 22.04 LTS due to the meson version requirement: |
|
@eli-schwartz how about:
|
|
@eli-schwartz another reason to drop the tests: yaml-cpp 0.8 is so old that gtest 1.17 does not work at all. When trying to build from wrapdb ( |
10b004c to
e392a53
Compare
|
@eli-schwartz I managed to fix the tests by switching the default to C++17 |
e392a53 to
8c9877d
Compare
this gets avoided with override_options. |
8c9877d to
81e1028
Compare
Done! |
Do they run in CI? |
f1e748c to
ff8925f
Compare
ff8925f to
cae342a
Compare
Sorry forgot, now they do! |
|
@eli-schwartz can we merge this now? |
This PR:
warning_levelor manually via compile args, but really in a subproject this doesn't make a lot of sense)cpp_stddefault argument, but I don't think this is an issue.