search

Tectu / malloy

A cross-platform C++20 library providing embeddable server & client components for HTTP and WebSocket.
BSD 3-Clause "New" or "Revised" License
66 stars 8 forks source link

[Bug] Building the tests should force building the client and server #61

Closed 0x00002a closed 3 years ago

0x00002a commented 3 years ago

Currently it is possible to tell cmake to build the tests without either the client, server, or (I'm guessing) both. This results in a linking error since the tests link to both the server and client.

Unless we want to split up the tests (which might prove difficult and which I can see no benefits for, upstream packages don't run the tests on build I'd hope), this should not be an allowed configuration. I suggest having it fail rather than (silently or not) toggle the flags automatically.

Tectu commented 3 years ago

I'm unsure as to why this would be the case. The test suite executable malloy-tests links to both malloy-client and malloy-server:

https://github.com/Tectu/malloy/blob/a6dccf81ee24dec60433309f3fd85237eb120c7b/test/CMakeLists.txt#L23

On my side both the client & server get built (along with their malloy-core dependency) when doing a clean build on the malloy-tests target.

0x00002a commented 3 years ago

Yes but this line: https://github.com/Tectu/malloy/blob/a6dccf81ee24dec60433309f3fd85237eb120c7b/lib/malloy/CMakeLists.txt#L5 removes the part that defines malloy-client as a target, meaning its just a library to cmake at that point and cmake allows libraries it can't identify in target_link_libraries

On my side both the client & server get built (along with their malloy-core dependency) when doing a clean build on the malloy-tests target.

By "clean build" do you mean deleted the build directory and recreated it then reran CMake? Cause the cmake config sticks around otherwise and only actually changes stuff explicitly set (so if I invoke it one with -DX=Y then again with -GNinja I get X=Y + ninja generator). This is what caught me out and how I found this bug origionally :p

Tectu commented 3 years ago

I see what you mean. Yeah, this definitely needs some improvements. Currently I think the best option is indeed to just have the malloy-tests cmake target fail if client & server are not both enabled. I'm not sure whether there will be undesired side effects but as you mentioned that shouldn't be a problem for anyone not actively working on malloy.