CMake build system + msys2 windows binaries CI #32

hartwork · 2022-01-24T22:08:50Z

@gkdr things that are worth noting:

Because this pull requests is not tiny, the individual commits may help tell a story during review.
The message client uses a function getline that MinGW doesn't have out of the box, so that is only compiled on Linux for the moment. Could be fixed but not a blocker in my eyes.
Tests are working fine on Linux but fail on Windows. The build logs show test execution output. I'll need help understanding Windows test failure. Should be fixed but not necessarily a blocker in my eyes.
I have left resolving the .c file includes in the test suite is left for later, maybe after this pull request.

Let me know what you think 🍻

gkdr

fantastic, thank you. definitely a major step towards the automated windows build of lurch. i guess i'm still a bit confused about cmake, so a few questions.

gkdr · 2022-01-26T19:55:17Z

.github/workflows/linux.yml

-        name: coverage_${{ matrix.libsignal }}
-        path: coverage/
+        name: axc_coverage_shared_${{ matrix.BUILD_SHARED_LIBS }}_pthreads_${{ matrix.AXC_WITH_PTHREADS }}
+        path: build/coverage*.html


note to self: i should probably replace this with something like https://github.com/codecov/codecov-action

That will be non-trivial, e.g. because we have four different sets of coverage results right now.
My vote for (1) adding submission rather than replacing artifact upload and (2) doing that after the merge.

yeah, definitely after the merge. and adding sounds good too :D

CMakeLists.txt

gkdr · 2022-01-26T20:11:22Z

README.md


+```
+mkdir build


can cmake do this for me?

~~Creating the build directory? I am not aware it would do that, no.~~

Turns out there is cmake -B build. I'm not sure if I like having ${PWD} being in source dir rather than build dir at the time of that call. ~~Is it about the readme or the CI or both?~~ If you're a big fan of cmake -B build we can do that, instead. What do you think?

I have made the CI use -B build rather than mkdir and cd now.

thanks, that feels a lot better. to be honest, it's about my own workflow. i like to do everything in the project root, some sort of build target that puts its stuff in its own directory so it can easily be ignored/cleaned, and some sort of clean target to get to a clean state. i'm considering putting a makefile in the project root until i remember the commands 😬

i'm considering putting a makefile in the project root until i remember the commands 😬

Please note that unless you are using Ninja locally or name the file GNUmakefile, it will clash with the one generated by CMake. I'd personally recommend less .github/workflows/linux.yml whenever you forget a command. Just an idea.

gkdr · 2022-01-26T20:12:22Z

README.md


+ninja -v all
+ninja -v test  # potentially with CTEST_OUTPUT_ON_FAILURE=1 in the environment


what does this env var change?

When tests fail, with the variable you get the full test run stdout with error details and without it you'd only get a summary like this one:

# ninja test [0/1] Running tests... Test project [..]/axc/build_new Start 1: test_client 1/2 Test #1: test_client ......................***Failed 2.35 sec Start 2: test_store 2/2 Test #2: test_store ....................... Passed 1.29 sec 50% tests passed, 1 tests failed out of 2 Total Test time (real) = 3.65 sec The following tests FAILED: 1 - test_client (Failed) Errors while running CTest Output from these tests are in: [..]/axc/build_new/Testing/Temporary/LastTest.log Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely. FAILED: CMakeFiles/test.util cd [..]/axc/build_new && /usr/bin/ctest --force-new-ctest-process ninja: build stopped: subcommand failed.

PS: CMAKE_CTEST_ARGUMENTS would allow us to pass --output-on-failure to CTest by default I suppose (which would make CTEST_OUTPUT_ON_FAILURE=1 unnecessary), but it needs CMake >=3.17 and we have 3.16 on Ubuntu 20.04 only.

i figured something like that, however i wasn't aware of how much of the output is usually swallowed. almost a bit unsettling how quiet it is now 😬

gkdr · 2022-01-26T20:32:38Z

README.md


+ninja -v all
+ninja -v test  # potentially with CTEST_OUTPUT_ON_FAILURE=1 in the environment
+ninja -v install


can there also be a target like clean? a target of that name exists, but it does not seem to clean everything.

Which files are left over that you'd want ninja -v clean to remove? Probably libaxc.pc. Anything else?
There is ADDITIONAL_CLEAN_FILES as a target property as well as a directory property that we could leverage.

I'm adding set_target_properties(axc PROPERTIES ADDITIONAL_CLEAN_FILES ${CMAKE_CURRENT_BINARY_DIR}/libaxc.pc) now to have target clean remove built file libaxc.pc.

i checked again, apparently it does nothing at all:

ninja: Entering directory `build' [2/2] Cleaning all built files... Cleaning... 0 files.

doesn't really matter if its in it's own dir, just wondered if i'm using it wrong.

Maybe it had nothing to do anymore? It does clean things for me once:

# ninja -v clean [1/2] /usr/bin/cmake -DCONFIG= -P CMakeFiles/clean_additional.cmake [2/2] /usr/bin/ninja -t clean Cleaning... 12 files. ^^ # ninja -v clean [1/2] /usr/bin/cmake -DCONFIG= -P CMakeFiles/clean_additional.cmake [2/2] /usr/bin/ninja -t clean Cleaning... 0 files.

gkdr · 2022-01-26T20:39:56Z

README.md


+ninja -v all
+ninja -v test  # potentially with CTEST_OUTPUT_ON_FAILURE=1 in the environment


the test target does not build the test files, i always have to call the all target first. is this intended?

First, let me say that I would love ninja test to depend on a full build. Using add_dependencies(all [..]) does not work because we'd get error non-existent target "all". Same for add_dependencies(test all). I'm out of other ideas right now. What do you think?

PS: Related links:

CMake upstream ticket https://gitlab.kitware.com/cmake/cmake/-/issues/8774

StackOverflow discussion https://stackoverflow.com/questions/733475/cmake-ctest-make-test-doesnt-build-tests

Seems like a time sink of limited value to me, to be honest. If you spot a great solution in there somewhere please let me know.

PPS: I should note that two of the advertised workarounds do not work:

- add_test(NAME ${_target} COMMAND ${_target}) + add_test(NAME run_${_target} COMMAND ${_target}) + add_dependencies(run_${_target} ${_target})

Test property DEPENDS

you're right that it's not terribly important. quick search also just resulted in the workarounds you tried, i'll leave it at that.

gkdr · 2022-01-26T20:41:42Z

README.md


+ninja -v all


ninja seems to swallow the colored output. is there some flag to cmake that can fix that?

Interesting catch. I found this article with both an explanation and a suggested fix. It's default could be made to depend on whether CMake Generator Ninja is used or not. Happy to improve on that aspect, but I'd still vote for merging before improving to untangle dependencies. What do you think?

PS: Going back to make would not fix the issue for those users with Ninja but is also an option, in theory.

PPS: I confirm that -fdiagnostics-color=always brings the color back for GCC with Ninja. A full build is 0.7s with Ninja and 1.6s with GNU make on my machine.

gkdr · 2022-01-26T20:48:03Z

CMakeLists.txt

+    endif()
+
+    if(AXC_WITH_PTHREADS)
+        target_link_libraries(${_target} PRIVATE Threads::Threads)


does this really work on windows?

Yes, msys2 has mingw-w64-winpthreads-git and it's part of group mingw-w64-x86_64-toolchain that we install in CI. Glib2 wouldn't be happy without it.

gkdr · 2022-01-28T18:12:01Z

i think i'll create another release before i merge this, so i can update the lurch submodule with some of the changes up until now without having to redo the build.

hartwork · 2022-01-28T20:04:12Z

i think i'll create another release before i merge this, so i can update the lurch submodule with some of the changes up until now without having to redo the build.

A "last" release with the Makefile sounds good, but does that block the merge? The submodule is set to any commit you want, e.g. either the commit before the merge to dev or a commit on a new branch release-0-3-7. So I don't see the blocker, personally.

gkdr · 2022-01-29T17:22:10Z

setting the submodule is one thing, and setting the tag and making a "github release" out of it to notify people is another. for the latter, i'd like to have the changelog in order, which i think just means setting the right date as a header.

hartwork · 2022-01-29T17:35:36Z

setting the submodule is one thing, and setting the tag and making a "github release" out of it to notify people is another. for the latter, i'd like to have the changelog in order, which i think just means setting the right date as a header.

I'm not following. I'll ask you about it in detail on Monday.

hartwork force-pushed the cmake-linux-msys2 branch 4 times, most recently from 65c4ed1 to 8ef19ae Compare January 25, 2022 02:12

gkdr reviewed Jan 26, 2022

View reviewed changes

hartwork force-pushed the cmake-linux-msys2 branch 3 times, most recently from fd5a841 to f23f542 Compare January 28, 2022 13:46

hartwork requested a review from gkdr January 28, 2022 13:47

hartwork mentioned this pull request Jan 28, 2022

fix test compile warnings #33

Merged

gkdr approved these changes Jan 28, 2022

View reviewed changes

hartwork mentioned this pull request Jan 29, 2022

CMake build system + msys2 windows binaries CI (fixes #6) gkdr/libomemo#38

Merged

tests: Make tests use . rather than test/ for SQLite files

c287e25

hartwork force-pushed the cmake-linux-msys2 branch from f23f542 to 664a54c Compare January 30, 2022 20:57

hartwork added 9 commits January 31, 2022 04:49

Add CMake build system (including a fixed pkg-config template)

ba53eb4

Actions: Migrate from Makefile to CMake and Ninja

281ca9f

Makefile: Drop

4754968

Drop Git submodules

2f83359

Actions: Build for Windows using msys2

cdc837c

README.md: Improve intro section text

8982304

README.md: Improve section on dependencies

5e01311

README.md: CMake-ify

57e22aa

CHANGELOG.md: Document this pull request

1e5cb97

hartwork force-pushed the cmake-linux-msys2 branch from 664a54c to 1e5cb97 Compare January 31, 2022 03:50

gkdr merged commit 31f6922 into gkdr:dev Jan 31, 2022

hartwork deleted the cmake-linux-msys2 branch January 31, 2022 23:01

hartwork mentioned this pull request Sep 2, 2024

CMake build system + msys2 windows binaries CI (fixes #11, fixes #148) gkdr/lurch#177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake build system + msys2 windows binaries CI #32

CMake build system + msys2 windows binaries CI #32

hartwork commented Jan 24, 2022

gkdr left a comment

gkdr Jan 26, 2022

hartwork Jan 26, 2022 •

edited

Loading

gkdr Jan 28, 2022

gkdr Jan 26, 2022

hartwork Jan 26, 2022 •

edited

Loading

hartwork Jan 26, 2022 •

edited

Loading

hartwork Jan 27, 2022

gkdr Jan 28, 2022

hartwork Jan 28, 2022

gkdr Jan 26, 2022

hartwork Jan 26, 2022

hartwork Jan 27, 2022

gkdr Jan 28, 2022

gkdr Jan 26, 2022

hartwork Jan 26, 2022 •

edited

Loading

hartwork Jan 27, 2022

gkdr Jan 28, 2022

hartwork Jan 28, 2022

gkdr Jan 26, 2022

hartwork Jan 26, 2022

hartwork Jan 26, 2022

hartwork Jan 26, 2022 •

edited

Loading

gkdr Jan 28, 2022

gkdr Jan 26, 2022

hartwork Jan 26, 2022

hartwork Jan 26, 2022

hartwork Jan 27, 2022

gkdr Jan 26, 2022

hartwork Jan 26, 2022

gkdr commented Jan 28, 2022

hartwork commented Jan 28, 2022

gkdr commented Jan 29, 2022

hartwork commented Jan 29, 2022


		ninja -v all
		ninja -v test # potentially with CTEST_OUTPUT_ON_FAILURE=1 in the environment


		```
		mkdir build

CMake build system + msys2 windows binaries CI #32

CMake build system + msys2 windows binaries CI #32

Conversation

hartwork commented Jan 24, 2022

gkdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

hartwork Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkdr commented Jan 28, 2022

hartwork commented Jan 28, 2022

gkdr commented Jan 29, 2022

hartwork commented Jan 29, 2022

hartwork Jan 26, 2022 •

edited

Loading

hartwork Jan 26, 2022 •

edited

Loading

hartwork Jan 26, 2022 •

edited

Loading

hartwork Jan 26, 2022 •

edited

Loading

hartwork Jan 26, 2022 •

edited

Loading