MSVC_RUNTIME_LIBRARY target property should not be set explicitly

[jdumas]

Hi there. I'm running into problems with the following statement: https://github.com/AcademySoftwareFoundation/openvdb/blob/73de06e50852f6bf70d1523558bb1f9c929fedf5/openvdb/openvdb/CMakeLists.txt#L516

Choosing to link against static or dynamic MSVC runtime has nothing to do with whether to link against a static or dynamic version of OpenVDB. The statement above is problematic in situations where other third-party libraries have been defined beforehand, and initialized to compile with /MD, but then we pull OpenVDB, which imposes /MT, and final linking fails. CMake configuration should still work when one leaves a default, unspecified value for CMAKE_MSVC_RUNTIME_LIBRARY, so I would suggest to remove this line, or make it a controllable CMake option.

[Idclip]

Choosing to link against static or dynamic MSVC runtime has nothing to do with whether to link against a static or dynamic version of OpenVDB.

I only half agree with this. In essence, yes you are correct, this is a choice that needs to be made for the entire software stack and you can theoretically link static builds with MD and vice versa. But doing the latter (dyn against static) is a really bad idea. In general CRT linking should be consistent unless the different program interfaces have been designed to allow interoperability between the different types. The idea behind this auto behaviour is to provide some semblance of consistency with CRT selection for static vs dynamic builds, something that the predominant of windows workflows seem to advise.

The statement above is problematic in situations where other third-party libraries have been defined beforehand, and initialized to compile with /MD, but then we pull OpenVDB, which imposes /MT, and final linking fails. CMake configuration should still work when one leaves a default, unspecified value for CMAKE_MSVC_RUNTIME_LIBRARY, so I would suggest to remove this line, or make it a controllable CMake option.

Your suggestion here breaks someones workflow where they have compiled all dependencies statically against the static CRT. There's no easy way to detect either and, from my understanding, it seems that maintaining consistency based on the lib type is a more desirable default.

or make it a controllable CMake option.

You should be able to pass in a value for CMAKE_MSVC_RUNTIME_LIBRARY to control this behaviour. i.e, for static->dyn CRT builds:

cmake -DCMAKE_MSVC_RUNTIME_LIBRARY="MultiThreaded$<$<CONFIG:Debug>:Debug>" -D OPENVDB_CORE_SHARED=ON -D OPENVDB_CORE_STATIC=OFF

[jdumas]

But doing the latter (dyn against static) is a really bad idea.

True, but this line is preventing to do the opposite (static lib against dynamic CRT, which is fine).

Your suggestion here breaks someones workflow where they have compiled all dependencies statically against the static CRT.

If someone would compile against the static CRT, they would already be specifying the value for CMAKE_MSVC_RUNTIME_LIBRARY. Leaving it blank by default should use the dynamic CRT, as is CMake's and MSVC's default behavior.

[Idclip]

If someone would compile against the static CRT, they would already be specifying the value for CMAKE_MSVC_RUNTIME_LIBRARY. Leaving it blank by default should use the dynamic CRT, as is CMake's and MSVC's default behavior.

I see what you mean. This then comes down to whether or not we believe the CMake defaults are in actuality not the best defaults for static libraries and whether this outweighs the benefit of providing a more standard (from the perspective of CMake) build workflow. I remember looking into this and deciding that linking against the static CRT made more sense for static libs but this was a while ago. I'm reticent to change this again but I'll try and revisit the sources I found. If every other dep is going to build with /MD by default regardless of the lib type, perhaps you're right and it's just easier all round if this is changed.

[jdumas]

If you do not like the CMake defaults, you should take it up to the CMake developers and push for change (maybe via a new policy in the next CMake version). Changing CMake default's behavior in an opinionated fashion just makes life more complicated by creating incompatibilities.

[Idclip]

This opinionated fashion was based on a read of the MSVC documentation and other investigation into Windows workflows from a non-Windows expert. CMake is not the only build system in existence. We should be choosing the most correct configuration for the platform irrespective of the build tool.

[jdumas]

Sure, but that being said:

1. I think the CMake developers are quite open to feedbacks, so if the CMake default doesn't make sense, it's possible to change it upstream.
2. If you want to diverge from CMake default options, I would suggest doing so only when building OpenVDB as a top-level project, and leaving CMake default unchanged when building it as a subproject. You can use the following snippet of code at the root CMakeLists.txt to detect if OpenVDB is build as a top-level project or not:

   # Detects whether this is a top-level project
   get_directory_property(HAS_PARENT PARENT_DIRECTORY)
   if(HAS_PARENT)
       set(OPENVDB_TOPLEVEL_PROJECT OFF)
   else()
       set(OPENVDB_TOPLEVEL_PROJECT ON)
   endif()
   

3. It's also fine to have different defaults in other build systems (e.g. Bazel), as long as we stay consistent within the same build-system, to maximize compatibility with other projects.

Anyway, let me know what you decide, and whether there's anything I can do to help. I'm also not a Windows expect, but my understanding is that many pre-compiled libs use /MD anyway, so there's usually little advantage in forcing /MT unless you have a very specific shipping scenario (but I could be wrong!).

[jdumas]

@Idclip any update on this issue from last year?