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

Fix windows build due to invalid flags #8

Closed 0x00002a closed 3 years ago

0x00002a commented 3 years ago

The windows build fails currently due to the "/Wa,-mbig-obj" compile flags. I'm not very familier with MSVC but I couldn't find either of those flags anywhere and cl.exe doesn't like them. Removing them seems not to cause any issues (so far) but I'm scared I might've messed with some magic incantation :p.

Tectu commented 3 years ago

No need to be scared :p As you might have seen, the library is currently in the "polishing it up for the first release" state. Therefore, I'm not surprised that things like this start to make problems. I am actually happy for this as it indicates that people are starting to use it. I welcome any PR :p

The -Wa -mbig-obj flags are for GCC. With GCC on Windows (via MSYS2) the resulting object files can exceed the 32k limit. These flags are used to tell GCC not to be a bitch about it. Other compilers including MSVC & clang would not need this. In fact, not even GCC as long as it's on a non-Windows platform.

What we should do is not only detect whether the build happen on Windows (using the WIN32 cmake variable) but also check whether it's with GCC (or at least NOT with MSVC).

0x00002a commented 3 years ago

Ah that makes sense. I'll add back the check but only for gcc mingw MSYS2 then. I assume the -O3 should also be NOT:MSVC rather than WIN32?

0x00002a commented 3 years ago

Sorry, accidental push, don't merge its broke

Tectu commented 3 years ago

The -O3 flag is also necessary to get the size down to something that GCC on Windows can handle. It shouldn't be necessary for any other combination either.

0x00002a commented 3 years ago

OK that should do it. I've tested the generate step on gcc/linux and it compiles and links with my setup on windows (MSVC). Haven't tested it with msys since I don't have an environment setup for that but setting WIN32 to True and toggling MINGW gives the correct behaviour (gcc and win32 but not mingw => true)

Tectu commented 3 years ago

I tried this and the build fails with the known "object too large" message. Looking at your code this most likely boils down to the detection of MSYS2. I will have a look at this soon and report back.

0x00002a commented 3 years ago

Just found another issue, some all of the examples have their own duplicates of this. Once we know the issue in the detection logic I'll make it a function and just call it for each one

Tectu commented 3 years ago

I think that 16e3d296c7f67b1b0c5c255fbc347f154298efbe fixes this. Using GCC through MSYS2 is achieved through MinGW which means that MINGW is True.

Can you please confirm that that commit builds fine on MSVC?

0x00002a commented 3 years ago

malloy-objs part builds fine but:

Command line warning D9035: option 'Og' has been deprecated and will be removed in a future release Command line error D8016: '/RTC1' and '/Og' command-line options are incompatible

When building the tests. Removing the -Og fixes that, also need to add #include <iostream> to test_suites/components/uri.cpp and finally need BOOST_DATE_TIME_NO_LIB or we get the link error.

Tectu commented 3 years ago

also need to add #include <iostream> to test_suites/components/uri.cpp

Hmm, I don't see any dependency within that file that would require <iostream>. Is this a transitive issue and we're actually missing that include somewhere else?

and finally need BOOST_DATE_TIME_NO_LIB or we get the link error.

This gets added as a private property to malloy-objs: https://github.com/Tectu/malloy/blob/66eb26a0f5308c77246d9f1e3a5a2f331252b766/lib/malloy/CMakeLists.txt#L47

That should do it, no? The test suite itself shouldn't require that or am I missing something obvious here?

Tectu commented 3 years ago

I just saw this comment in doctest.hpp:2731 where #include <iostream> is located:

// required includes - will go only in one translation unit!

seems relevant.