Compiler flags override consumer flags

[Warpten]

Git commit reference cee27e000166b7aa5fce63f6c8ef25ded2f8bf27 ; master still affected

Describe the bug Using DPP through vcpkg causes compile definitions to leak through to users of the library, overriding user flags.

To Reproduce Steps to reproduce the behavior:

find_package(dpp CONFIG REQUIRED)
set_target_properties(my_target PROPERTIES
  CXX_STANDARD 20
  CXX_EXTENSIONS OFF
  CXX_STANDARD_REQUIRED ON
)

target_link_libraries(my_target
  PRIVATE
    dpp::dpp
)

Then compiling on clang (or gcc, doesn't matter) happens:

/usr/bin/c++
-DBOOST_ALL_NO_LIB -DBOOST_ASIO_NO_DEPRECATED
-DBOOST_BIND_NO_PLACEHOLDERS
-DBOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE
-DBOOST_SYSTEM_USE_UTF8
-DBOOST_THREAD_PROVIDES_FUTURE
-DBOOST_THREAD_PROVIDES_FUTURE_CONTINUATION
-DBOOST_THREAD_PROVIDES_FUTURE_WHEN_ALL_WHEN_ANY
-DDPP_BUILD
-DLIBTACTMON_STATIC_BUILD
-DNO_BUFFERPOOL
-DSPDLOG_COMPILED_LIB
-DSPDLOG_FMT_EXTERNAL
-DTC_HAS_BROKEN_WSTRING_REGEX
-DCMAKE_INTDIR=\"Release\"
-I/home/runner/work/tactmon/tactmon/src/tactmon
-I/home/runner/work/tactmon/tactmon/src/libtactmon
-I/home/runner/work/tactmon/tactmon/dep/libassert
-isystem /home/runner/work/tactmon/tactmon/builds/ninja-multiconfiguration-vcpkg/vcpkg_installed/x64-linux/include
-O3
-DNDEBUG
-std=c++20
-std=c++17
-Wall -Wempty-body -Wno-psabi -Wunknown-pragmas -Wignored-qualifiers -Wimplicit-fallthrough -Wmissing-field-initializers -Wsign-compare -Wtype-limits -Wuninitialized -Wshift-negative-value -pthread -O3 -fPIC
-MD -MT src/tactmon/CMakeFiles/tactmon.dir/Release/Program.cpp.o -MF src/tactmon/CMakeFiles/tactmon.dir/Release/Program.cpp.o.d -o src/tactmon/CMakeFiles/tactmon.dir/Release/Program.cpp.o -c /home/runner/work/tactmon/tactmon/src/tactmon/Program.cpp

Notice every last one of dpp's compile definitions leak through, causing builds to fail on code that uses C++20 specific features such as concepts.

These specific build flags come from the target generated for CMake:

  INTERFACE_COMPILE_OPTIONS "\$<\$<PLATFORM_ID:Windows>:/bigobj>;\$<\$<PLATFORM_ID:Linux>:\$<\$<CONFIG:Debug>:-std=c++17;-Wall;-Wempty-body;-Wno-psabi;-Wunknown-pragmas;-Wignored-qualifiers;-Wimplicit-fallthrough;-Wmissing-field-initializers;-Wsign-compare;-Wtype-limits;-Wuninitialized;-Wshift-negative-value;-pthread;-g;-Og;-fPIC>>;\$<\$<PLATFORM_ID:Linux>:\$<\$<CONFIG:Release>:-std=c++17;-Wall;-Wempty-body;-Wno-psabi;-Wunknown-pragmas;-Wignored-qualifiers;-Wimplicit-fallthrough;-Wmissing-field-initializers;-Wsign-compare;-Wtype-limits;-Wuninitialized;-Wshift-negative-value;-pthread;-O3;-fPIC>>"

And these in turn come from https://github.com/brainboxdotcc/DPP/blob/master/library-vcpkg/CMakeLists.txt#L14

Expected behavior Compiler options for DPP should not leak trough to consumer files. Probably define them as PRIVATE in cmake. I'm not a CMake expert so maybe I'm doing something wrong tho.

System Details:

• OS: GitHub Linux runner
• Discord Client used for testing [mobile, desktop, web]: Irrelevant

Additional context

Happens also on MSVC but is mostly invisible. I'm guessing CL invocation by Visual Studio appends actual flags from config pages to the extra options, and since DPP flags leak through extra options, the order of flags on the command-line is reversed and that issue is just hidden.

[harshfeudal]

Currently, we support C++ 17. You can read more here: https://dpp.dev/md_docpages_01_frequently_asked_questions.html#autotoc_md20

Does D++ require C++20 support?

No, at the current time we do not use any C++20 features. Some C++17 features are used, which are available in all recent compilers.

[Warpten]

The issue is not that DPP requires C++17. The issue is that these flags bleed to consumers of DPP, which means that a compiler then tries to build my code as C++17 while it uses C++20 features, which obviously fails if i ever so happen to use something that was introduced by C++20, such as concepts.

[LagPixelLOL]

happens to me too when compiling on ubuntu 20.04

[Warpten]

Some followup on this:

• I've spent a bit of time rewriting DPP to use vcpkg in mostly all cases when compiling, as well as cleaning up the CMakeLists files. I got CI to work for the most part except for cross-compiling. Haven't looked exactly as to why. https://github.com/Warpten/DPP/tree/feature/cmake-cleanup . GH Actions and CI in general is something I'm not really good at, I'm down for pointers to fix those and any other potential mistakes I may have added.
• I don't exactly remember (I'm working on other things unrelated to Discord at the moment) but I think this also happened to me when building DPP as a submodule to my actual code, rather than being pulled from vcpkg.
• I also made it so that default vcpkg builds don't include coroutine and voice features. This is now exposed through vcpkg features. Potentially a breaking change for existing vcpkg consumers; is this something worth enabling by default? Having no-voice and no-coro vcpkg features feels backwards
• I also added static Windows DPP builds; at the moment this is very subpar as it also statically links to openssl, opus etc leading to a very large static library (so large, in fact, that the linker straight up complains and refuses to link). Ideally, static DPP builds would have CMake interface dependancies on opus, libsodium etc, but for some reason I can't get that to work. Probably my mistake somewhere.
• I also have some questions as to how the current CMakeLists came to be as I see stuff that makes little to no sense and feels just hacked together until it just works (tm).

[braindigitalis]

a few things to note:

1) we dont want dpp to use vcpkg in mostly all cases. vcpkg is something that you should have to explicitly request to use, and not something we want pushing its way into all the builds. In my opinion it is a horrid package manager to auto build releases for, and causes all kinds of hassle outside of windows. 2) coro feature is not complete and should not be offered as an option in vcpkg builds right now... 3) theres is a LOT of cmakelists for a lot of systems and setups, not just vcpkg. We cant take them out, and they arent hacked together, they are the result of 2 years of fixes, adjustments and enhancements. Trimming them out will reintroduce many regressed bugs.

I hope you dont see this as me shooting you down, as it looks like youve put a lot of effort into this. I just wouldnt want the vcpkg stuff to intrude into the rest of the build workflow, as a tiny minority of our userbase use it and most do not like it much. They prefer to install the package using linux package management such as apt, or to include it as a git submodule.

[Warpten]

Well, the work I've done does not use vcpkg unless a CMake toolchain is specified; I have made intermediate dpp::* INTERFACE targets (for zlib, sopium, opus, etc) that are then pulled in as needed into the main target itself. They all use the standard find_package CMake macros.

I understand you're not shooting me down, i'd rather have you tell me what I did wrong so I can correct it as not to break everytrhing.

At the very least, it'd be nice to fix the actual issue at hand; even if I don't use coroutines, I should be able to use C++20 features in consumer code

[Eremiell]

Could you possibly try to open a PR so it runs on the CI and we can all see what it does or doesn't?

Like Brain said, we don't really want the vcpkg to take over the whole process, and you made it sound like that's your goal, so that makes us quite reluctant. But if we're leaking any flags, that should be fixed if possible but importantly without breaking any of the other stuff.

Also note that same or similar issue has been already resolved recently. See #595 and the followup link to Gentoo issue tracker. Could a similar approach fix this?

Also this seems to be a pretty huge set of changes. Could you possibly try to split it into multiple smaller sets of well defined changes, that would be easier for us to analyse and understand rather than a huge monolithic rewrote the whole CMake setup scenario?

[Warpten]

Yeah, I'll squash and organize eventually. As far as CI goes, it runs on the fork - at least the relevant bits; those that use secrets don't.

[Eremiell]

Alright, checked random job on your fork. g++-11 specifically. Seems like it uses vcpkg now. That is explicitly unwanted. Others seem to follow the suit.

Vcpkg should be basically limited to when user explicitly requires that and never used otherwise, esp. not on CI. The previous setup when it kinda used vcpkg seemingly by default has been primarily caused by how vcpkg setup works afaik, and some of us objected that already. No one was able to figure a better way to set it up back then sadly and so it remained the way it was. (Issue of both time/energy and experience with vcpkg specifically.)

It has been explicitly set to off for CI.

Notes on some other parts:

I believe we prefer the default builds to include voice. Reason for that is we got plenty of not very experienced developers interested in Discord development, and it's easier to just ship the feature by default so people who don't know any better get it there and don't have to wonder why it doesn't work. May be negotiable though.

We don't really support static linking any much. For first Discord API often changes abruptly and unpredictably, while we generally try to upkeep stable library API. Then there's the whole thing with people trying to use the library for malware, and they seem to always be unhappy to trail dlls along their creations for some reason, but that actually works for us. zlib also has pretty limited support was static linking based on some analysis made by other contributors previously. But that's more of a supplementary argument.

[Warpten]

I will have to check but I remember setting up CI so that vcpkg should default to system installs and not its own ports. Ideally, I would get rid of vcpkg in CI.

I can't work more on this branch right now if it's even something that is wanted, but I'll take notes and correct accordingly.

[Eremiell]

There's no rush to anything. Patches are generally welcome, but discussing the scope and goals may be beneficial esp. when doing something that has many possible ways to be resolved. You can always join on the Discord, people are generally available and chatty there to answer any questions, mainly during European hours. Or just open an issue though the speed and general attendance may be much lower here.

[github-actions[bot]]

This issue has had no activity and is being marked as stale. If you still wish to continue with this issue please comment to reopen it.