Fix unnecessary/restrictive public target flags

[nyoma-diamond]

Resolves #3125, where building dlib using one compiler or system and linking to a consumer using a different one is incompatible.

Key changes:

• Changed some calls to target_compile_options and target_compile_features from PUBLIC to PRIVATE, preventing dlib's compile-time configuration forcing incorrect assumptions on the consumer.
  • active_compile_opts
  • dlib_needed_public_cflags (now named dlib_needed_private_cflags)
  • dlib_needed_public_ldflags (now named dlib_needed_private_ldflags)
• Combined active_compile_opts and active_compile_opts_private (the latter no longer needs to be a separate variable)
• Applying active_compile_opts no longer requires different methodologies depending on if MSVC + CMake < 3.11 are used (simpler approach)

[nyoma-diamond]

hmm the windows-latest check is failing due to not seeing /bigobj when trying to build tests. It seems like the build methodology for tests relies on PUBLIC target compile options, which it arguably shouldn't. Will need to take a look at/fix this

[davisking]

Well, my point is that things need to Just Work for client code that tries to build things. The test program is just one example of a client program that should just work without visual studio specific magic needing to be added to it.

I.e. dlib is meant to be a portable cross-platform library. Clients should not need to add "oh if I'm using $XYZ compiler then I have to do ABC to make this work"

[nyoma-diamond]

Of course. My point is that the current approach actually makes dlib not portable if the initial compilation system used MSVC, but then the client doesn't (e.g., if they're using Clang). In this case, it'll break because of /bigobj being forced on the client even if it shouldn't. This is an edge-case which previously wasn't considered in these checks.

In the case of the failing tests check, the current approach is problematic specifically because it assumed compiler-specific details that were set at build-time (i.e., /bigobj) would also apply to the client system. This isn't necessarily true. To my understanding, this check only passed previously because the test client uses MSVC. As a result, the edge-case where the client has a different compiler (e.g., clang) was never tested (and would have failed previously had it been tested). The windows-latest test is failing now likely because the change I applied fails to either (A) automatically identify necessary compiler details of the client system with the dtest target, or (B) fails to propagate the selected options to other parts of the build process at initial compilation time. I'll investigate this more and find a fix.

I.e. dlib is meant to be a portable cross-platform library. Clients should not need to add "oh if I'm using $XYZ compiler then I have to do ABC to make this work"

@davisking to clarify on this a little bit, are you saying that the client system shouldn't require any compiler-specific checks or just that the client shouldn't need to manually specify those details themselves (e.g., to cmake in CLI)? If the latter, we're on the same page and that's what I'm trying to fix. If the former, I believe that to be a false errand because flags like /bigobj are inherently incompatible across certain systems/compilers, making portability fundamentally unachievable in the case of cross-compiling (flags like /bigobj need to be [un]set for the client's compiler, depending on what it is).

Please correct me if I'm missing something :)

[nyoma-diamond]

Upon investigation of the check failure:
If we're treating the test program like a client (consumer) program, I would argue the problem here isn't that dlib fails to propagate /bigobj as a requirement for building dlib, but that the client (in this case the test program) fails to enable /bigobj for its own operation.

More specifically, I don't believe it should be dlib's responsibility to force the consumer to use /bigobj. dlib alone can build/compile just fine without /bigobj enabled (the files that fail due to missing /bigobj are all example files, not dlib core). If the consumer uses dlib in a way that requires enabling /bigobj, then /bigobj is a requirement of the consuming project, not dlib itself, and should be the consumer's responsibility to enable (not dlib's). To my understanding, this is common practice for a lot of other libraries.

In the case of the failing check, I'd argue /bigobj needs to be set in example/CMakeLists.txt or test/CMakeLists.txt as it's the example project/subdirectory that contains (client) code that requires the option. @davisking what do you think?

EDIT:
I just noticed dnn/core.h requires /bigobj. Thus there's a different problem occurring here that I'm missing. That said, my prior thoughts still stand; we need a way to configure the project so /bigobj is only enabled when necessary rather than forcing it at all times.

As an additional alternative option, I'll also play with generator expressions to see if we can still allow /bigobj and other compiler options to be public, but automatically ignore them if the client/consumer uses a different compiler. This may work, but is also a much more fundamental methodology change that may require additional review to validate beyond what I'm capable of on my own.

[nyoma-diamond]

(Apologies for all the comments + edits; trying to document my thinking publicly and get input if anyone has any 🙂)

[davisking]

I'm just saying that clients of dlib should not have to set /bigobj themselves. Things should just work for them. They shouldn't have to know about this weird /bigobj thing that MSVC demands. Which concretely here means that whatever you do you can't edit the examples/CMakeLists.txt file or dlib/test/CMakeLists.txt file to do whatever it is you want to do, since those are examples of what clients of dlib have in their CMakeLists.txt files.

I.e. people, for like 2 decades now, have been using dlib and making simple CMakeLists.txt files that just work on all platforms without them ever needing to add any kind of "oh but if I try to build on MSVC then add this switch" to their CMakeLists.txt files. That needs to continue to be the case.

So if you can make a PR that doesn't break any existing clients (e.g. the unit tests or example programs for instance) then that would be sweet.