silverqx / TinyORM

Modern C++ ORM library
https://www.tinyorm.org
MIT License
242 stars 25 forks source link

Aggressive "warnings" flags exported by the TinyOrm::TinyOrm target #18

Closed SchaichAlonso closed 1 year ago

SchaichAlonso commented 1 year ago

Hi

Currently, the TinyOrm::TinyOrm target is populated with properties that are transcended to consuming targets by code in cmake/CommonModules/TinyCommon.cmake.

Most of those properties seem to be "cosmetic" i.e. not affecting the ABI. Transcending -Werror to consumers, however, is almost guaranteed to break things, as is the transcendance of the QT API Macros.

I take it that these compiler options are supposed to emit diagnostics when TinyOrm is being compiled. However, the INTERFACE mode of cmake's target properties causes these flags to be propagated to all consumers of the TinyOrm::TinyOrm target instead of being used by TinyOrm::TinyOrm itself ( https://cmake.org/cmake/help/v3.27/manual/cmake-buildsystem.7.html#transitive-usage-requirements ). Most of those flags should likely be PRIVATE, i.e. not transcend to consumers at all, with the exception of the c++20 standard requirement, which should be PUBLIC as tiny uses c++20 itself and also passes stl datatypes / exceptions through the DLL interface, i.e. requires consumers also to use c++20 .

To excemplify the problem

main.cpp

int main() {
    return 7.5;
}

doesn't even use TinyOrm, however associating it to TinyOrm via

CMakeLists.txt

cmake_minimum_required(VERSION 3.24)

project(MyProject CXX)

find_package(TinyOrm CONFIG REQUIRED)

add_executable(MyTestApplication)
target_sources(MyTestApplication PRIVATE main.cpp)
target_link_libraries(MyTestApplication PRIVATE TinyOrm::TinyOrm)

will cause TinyOrm::TinyOrm's -Werror transcedence to break the compilation as the diagnostic about 7.5 not being an int in the return statement is turned into an error:

[ 50%] Building CXX object CMakeFiles/MyTestApplication.dir/main.cpp.o
/tmp/main.cpp:2:16: fatal error: implicit conversion from 'double' to 'int' changes value from 7.5 to 7 [-Wliteral-conversion]
        return 7.5;
        ~~~~~~ ^~~
1 error generated.
silverqx commented 1 year ago

I think you are right, we can't dictate to users who want to use the TinyORM library to use so restrictive warnings. The truth is that I set this compiler warning options as strictly as possible and they are brutal (Wextra, Weffc++, Werror, pedantic, pedantic-errors, ...).

You posted this issue at the ideal time because I have currently revisited the whole qmake build, I have prepared a big patch and the next thing I wanted to do is to revisit the CMake build. So we can solve it, BUT it will not be asap because this qmake patch is really big and will take some time to correctly test everything.

And unfortunately the MySQL 8.0.24 and also 8.1 is out and I have a problem with TLS v1.2 and v1.3 connections from the libmaria library (libmaria is used in MSYS2 ucrt64) so I have to solve it even before that qmake patch.

silverqx commented 1 year ago

Now to the described problem itself, I don't exactly remember what is leaking to the users' environment in CMake builds, so I can't argue if something is bad or not, I did a fast look-up now and it's not a simple thing that can be solved in few minutes. I need to revisit it all.

But the problem you have described is not a good thing from the consumer's perspective and has to be fixed.

The fix will not be in the next release but will be in the release after.

Thank you for reporting this. đź‘Ś

silverqx commented 1 year ago

Well, this will be a hard one, fortunately, I have a solution. I have already looked at it and everything is propagated to the user, I'm gonna make it all private but still will be possible to enable this interface library using the CMake option eg. by INTERFACE_LIBRARY or PROPAGATE_STRICNESS or something like that.

silverqx commented 1 year ago

Fixed in eac8957.

It was easier than I thought, I thought I had to do something else.