Closed willwray closed 1 year ago
I've resolved the review comments so far.
To move from Draft to full PR I should add a test that meson.build is working.
The least invasive way to add a test might by to use a github action;
a .github/workflows/meson.yml
, using the same platform list as the cmake.yml
.
Then, a CI failure will flag up that attention is needed.
Merging #71 (5daf1e3) into main (b14ee2b) will not change coverage. The diff coverage is
n/a
.
Hi @willwray. The CI does not seem to run on this repo for this PR, only on yours, hence the results don't get published here. I think this is because I incorrectly configured the workflow files to run with
on: push
instead of
on: pull_request
They are equivalent for me when I create a PR, but not for PRs coming from forks like this one. Could you please try to change your new workflow file and the original cmake.yml
to use on: pull_request
instead? I think that should fix it.
Also, I noticed you removed the draft tag; is the PR is ready for review?
ok, I've added pull_request
and workflow_dispatch
triggers to both cmake.yml
and meson.yml
:
on:
workflow_dispatch:
push:
pull_request:
That should trigger as before for you as well as on contributor pull requests.
workflow_dispatch
creates a manual launch button in the UI. It's useful in case CI fails due to flaky runner or timeouts, and in future if you decide to constrain the triggers to run only on specific types of change, file types or paths.
The WebAssembly targets in cmake.yml
may need some attention.
They generate lots of deprecation warnings for me, and all but one fail (on whatever standard github runners are allocated).
https://github.com/willwray/snitch/actions/runs/4211022818
I tried updating mymindstorm/setup-emsdk@v11
-> v12
in the first meson.yml
commit here - that cleared up some warnings.
Then, I still couldn't get the wasm target working with meson (I think that a toolchain file may help). So I cut the target from meson.yml
.
Nice about workflow_dispatch
, I didn't know about it! About push
, I would remove it, since it creates duplicate builds when I create a PR (one for the PR, one for the commit, at least that's what I remember).
For wasm, setup-emsdk
has a known bug with cache collision; it will often fail a build when more than one wasm build runs. This is normally fixed by re-running.
I can't comment on the meson-specific issue though.
Ok, removed on: push
leaving workflow_dispatch
.
I also bumped mymindstorm/setup-emsdk@v11
-> v12
in cmake.yml
.
I resolved the comments about target_compile_definitions
by removing the equivalent definitions from the meson.build
, leaving them in the main CMakeLists.txt
.
If you've decided to remove them on cmake side then I could add a commit to do that here. What do you think?
Yes I think we can remove them from the cmake script too, if you don't mind doing it here!
Great stuff! I reckon this is ready for review now.
I'm using the meson build support in two projects, applying the PR changes as a patch via a meson wrap dependency. It's all working as expected for me. I also tested that install and uninstall works correctly on linux.
The two options - create_library
and create_header_only
- used as switches in meson.build
have the expected effect. The other options are forwarded to generate the snitch_config.hpp
file, same as the cmake build.
The meson.build
scripts had useful review by meson devs - thanks!
The meson.yml
actions build the static library, then run the header-only test on Linux gcc/clang stdlibc++, msvc and mac clang-libc++ platforms. This tests that the make_snitch_all.py
script works correctly. I decided not to add a test target for the static library as it should already be sufficiently tested on the cmake side - the meson CI acts mostly as a sanity check across the common platforms.
Shall I add some documentation or something for release notes? A short section in the readme?
The readme is the only documentation at present; adding a section or note in there to mention meson support would be great. Perhaps just after the section ## Example build configurations with CMake
, we could add a ## Example build configurations with meson
, mirroring what is done with cmake?
I don't want to do a full tutorial on build system here, but it helps newcomers and less experienced devs to have some basic build system integration suggestions to get them started.
I use a SublimeText package (MarkdownTOC) to automatically maintain the table of contents in the readme, but if you don't use Sublime and/or don't use this package, you can absolutely edit the TOC by hand. Here's what MarkdownTOC generates for me if I add the sections suggested above:
- [Example build configurations with meson](#example-build-configurations-with-meson)
- [Using _snitch_ as a regular library](#using-snitch-as-a-regular-library-1)
- [Using _snitch_ as a header-only library](#using-snitch-as-a-header-only-library-1)
IMHO a workflow_dispatch
isn't very useful here... if a CI job flakes out and you want to manually run it, then you can just manually "rerun failed jobs" when viewing the results page for that run.
What workflow dispatch is really useful for, however, is when your workflows can do different things depending on what the date is that you run it. For example, if instead of just building the current source code commit, it tries to integrate it with the current git master of another project. We do this in Meson -- most CI jobs only run on commits, but the website builder runs on both commits and on workflow dispatches, because the WrapDB projects page is generated from a second repo. So whenever WrapDB is updated, it pokes the API and triggers a website builder re-run for the meson repo -- which then downloads the WrapDB repository as part of the build, and therefore changes the website even when no new commits landed in the Meson repo.
Also I still recommend running CI on pushes, because the CI for a pull request will only test that the PR at the time of creation works. If you merge two PRs, one after the other, and both work in isolation but combined cause the tests to fail, you get zero warning.
The best way to solve this is to require PRs to be rebased, and rerun the CI before merging. You can do this with Gitlab's setting for the "semi-linear history" merge method, but GitHub has never added such an option -- I just checked again.
At least if you use:
on:
push:
branches:
- main
You get immediately alerted when it fails, and can go back and fix it. But at the same time, you don't run CI repeatedly for the same commit when using branches as the repository owner.
I haven't looked at the UI for workflow_dispatch
yet, but I was hoping it would be a bit more accessible than the action "re-run failed jobs" button. Could it be also that the workflow_dispatch
UI is accessible to non-members of the repo?
About push
, thanks for pointing out the problem. I noticed GitHub has a setting requiring a PR branch to be up to date with the target branch before merging. I just turned that on. That should do it?
Which setting are you referring to?
Perhaps Managing a branch protection rule point 7, second bullet
Does this setting address the reason for reinstating the push
trigger?
I'd agree that workflow_dispatch
is only marginally useful and may not pay its way.
While it does no direct harm it adds mental overhead to review a feature added 'in case'.
Thinking about it... (it happened to get a mention in a cppcon GitHub actions talk just released)
While we're considering how best to trigger CI actions...
You also bring up the chicken and egg decision of WrapDB. In documenting meson support, it'd be good to recommend
> meson wrap install snitch
as the way to get a client subproject/snitch.wrap
file.
Should I attempt a PR to add the wrap file to the WrapDB repo?
Perhaps Managing a branch protection rule point 7, second bullet
Oh, that. You can only do that as a side effect of making it impossible for the project owner to dismiss failing CI and merge anyways, which is sometimes necessary if you know the CI itself has a bug, but the bug is unrelated to the current PR and all the other tests pass.
It's a big pity that setting isn't its own setting, but I personally lost all temptation to use it as a result.
Should I attempt a PR to add the wrap file to the WrapDB repo?
Sure, either option 2 or 3 sounds good.
pasting the added README.md markdown section here for ease of review
First, meson build needs a subprojects
directory in the project source root for dependencies, then:
> meson wrap install snitch
downloads a reviewed wrap-file from WrapDB to subprojects/snitch.wrap
Here's an equivalent wrap-git specification, for illustration:
[wrap-git]
directory = snitch
url = https://github.com/cschreib/snitch.git
revision = v1.1.0
depth = 1
[provide]
snitch = snitch_dep
The provided snitch_dep
dependency is then retrieved and used in a meson.build
script, e.g.:
snitch_dep = dependency('snitch')
test('mytest', executable('test','test.cpp',dependencies:snitch_dep) )
Or use snitch_dep = subproject('snitch').get_variable('snitch_dep')
in meson < v0.54.
With its wrap
dependency system, meson uses a snitch install, if found, or downloads snitch as a fallback.
Then, configure the project in a given build directory, build and test:
> meson setup builddir
> meson compile -C builddir
> meson test -C builddir
To avoid unneeded build steps, configure with one of these options:
-Dsnitch:create_library=false
to disable the library build-Dsnitch:create_header_only=false
to disable "snitch_all.hpp
" generationIt's a bit longer than intended - I could cut the sample 'illustrative' wrap file.
Oh, that. You can only do that as a side effect of making it impossible for the project owner to dismiss failing CI and merge anyways, which is sometimes necessary if you know the CI itself has a bug, but the bug is unrelated to the current PR and all the other tests pass.
It's a big pity that setting isn't its own setting, but I personally lost all temptation to use it as a result.
I agree it is odd that the two settings are connected in this way. But thankfully, you need to select which CI checks must always pass before a PR can be merged; and the list can be empty... That's what I have now, but I have yet to see if this works as intended. I guess we'll give it a go for the next few PRs, and see what happens.
@willwray: Thank you for the updated readme! I submitted a few suggested changes; if you don't mind reviewing them. There are a couple other places in the readme that are CMake-specific. Would you mind having a look and tweaking these to include instructions for meson as well?
### Detailed comparison with _Catch2_
: "This may be relevant for very small projects, or projects that do not use CMake"; the last bit could be replaced by "do not use one of the supported build systems".### Using your own main function
: mentions how to configure CMake to prevent snitch from defining main()
.### Header-only build
: mentions CMake targets to select between normal and header-only snitch.I think the example wrap-file is useful, if people want more control over the version and other stuff? At any rate, I don't mind a lengthy readme; there's a table of content so people can quickly get to the information they need.
Oh, and the table of contents is missing your new section, here's the updated one
<!-- MarkdownTOC autolink="true" -->
- [Features and limitations](#features-and-limitations)
- [Example](#example)
- [Example build configurations with CMake](#example-build-configurations-with-cmake)
- [Using _snitch_ as a regular library](#using-snitch-as-a-regular-library)
- [Using _snitch_ as a header-only library](#using-snitch-as-a-header-only-library)
- [Example build configuration with meson](#example-build-configuration-with-meson)
- [Benchmark](#benchmark)
- [Documentation](#documentation)
- [Detailed comparison with _Catch2_](#detailed-comparison-with-catch2)
- [Test case macros](#test-case-macros)
- [Standalone test cases](#standalone-test-cases)
- [Test cases with fixtures](#test-cases-with-fixtures)
- [Test check macros](#test-check-macros)
- [Tags](#tags)
- [Matchers](#matchers)
- [Sections](#sections)
- [Captures](#captures)
- [Custom string serialization](#custom-string-serialization)
- [Reporters](#reporters)
- [Default main function](#default-main-function)
- [Using your own main function](#using-your-own-main-function)
- [Exceptions](#exceptions)
- [Header-only build](#header-only-build)
- [`clang-format` support](#clang-format-support)
- [Why _snitch_?](#why-_snitch_)
<!-- /MarkdownTOC -->
Oh, and the table of contents is missing your new section
Oops; done.
I think the example wrap-file is useful, if people want more control ...
I changed my mind on this. Most client projects should use WrapDB. Editing a wrap file should be an 'expert' feature. And the wrap file is optional - I've added an explanation of a more manual setup.
There are a couple other places in the readme that are CMake-specific. Would you mind having a look and tweaking these to include instructions for meson as well?
I added a separate commit to incorporate meson in those places.
See what you think.
@eli-schwartz - do you have thoughts on the added meson README material?
First, meson build needs a subprojects
directory in your project source root for dependencies. Create this directory if it does not exist, then, from within your project source root, run:
> meson wrap install snitch
This downloads a wrap file, snitch.wrap
, from WrapDB to the subprojects
directory. A [provide]
section declares snitch = snitch_dep
, and that guides meson's wrap
dependency system to use a snitch install, if found, or to download snitch as a fallback.
The provided snitch_dep
dependency is retrieved and used in a meson.build
script, e.g.:
snitch_dep = dependency('snitch')
test('mytest', executable('test','test.cpp',dependencies:snitch_dep) )
Alternatively, you can git clone
snitch directly to subprojects/snitch
. A wrap file is then optional. You can retrieve the dependency directly (as is necessary in meson < v0.54):
snitch_dep = subproject('snitch').get_variable('snitch_dep')
If you use snitch only as a header-only library then you can disable the library build by configuring with:
-D snitch:create_library=false
Otherwise, if you use snitch only as a regular library, then you can configure with:
-D snitch:create_header_only=false
And this disables the build step that generates the single-header file "snitch_all.hpp
".
Just one nitpick left; otherwise looks good to me!
I'm happy to merge this, unless you want to wait for more feedback?
It's ready to merge, I reckon.
After a merge, it looks like a release will be needed in order to submit a snitch wrap to WrapDB. It needn't be a 'full' release, just a point release (with commit hash) or beta release will do to test that out.
I plan to pick this up again next week.
Works for me! I'm close to finish #60 (with constexpr expressions decomposition as a bonus, I am surprised but it seems to works...), and together with the accumulated big fixes, this will be a good time for a point release.
Thank you for your contribution :)
This PR adds native meson build support for clients (not for snitch development).
Meson build is already supported for client projects via meson's cmake module. Assuming that the client project has a
snitch_cmake
meson option:(unfortunately this client logic cannot be provided by snitch meson build).
This PR enables meson build without a CMake install available.
Note, in comparison, that doctest has seven lines of meson support:
```python project('doctest', ['cpp'], version: '2.4.9') doctest_dep = declare_dependency( include_directories: include_directories('doctest')) if meson.version().version_compare('>=0.54.0') meson.override_dependency('doctest', doctest_dep) endif ``` doctest also has an update script to edit the version number. A neater way to automate version number updates for snitch might be to place the version number in a tiny file, to be read by both CMake and meson setup. Otherwise, the version has to be updated in both CMakeLists.txt and meson.build files - a bit of maintenance burden easily forgotten. ------The main bloat in this PR is in support of build options.
This PR copies the configuration options of the CMake build, replicating configure_file data as meson_options variables. This calls for repetitive code to copy to and from variables; a maintenance burden when options are added or if the default values are changed, as they need to be edited in at least three places; CMakeList.txt, meson_options.txt, meson.build.
The only options used in
meson.build
arecreate_library
andcreate_header_only
(and the other options could easily be cut down or removed).This PR adds three new entries in the snitch source root;
meson.build
file, itsmeson_options.txt
file and asnitch
subdirectory (apparently needed in order to generatesnitch_config.hpp
in the equivalent build/snitch directory).The
snitch_all.hpp
single file header generation is straightforward using the existingmake_snitch_all.py
script, invoked by acustom_target
via meson's Python module.