Remove the forcing of "/MT"

[fei4xu]

Cinder's current cmake system actually works well on Windows, but the runtime library is fixed to MT which is not the default setting of other cmake projects. When cinder is used together with other cmake based MSW projects, the setting should have a switch to change between MT and MD.

[fei4xu]

this should close #2116

[MikeGitb]

Please no. Cinder should just accept whatever setting was handed down to it. If every library starts to introduce their own cmake option for every compiler flag, this becomes an unmaintainable mess for the user and makes it more difficult to package it.

What was the reason for forcing MT in the first place?

[fei4xu]

This PR is not to force MT, it's actually removing the forcing of MT.
Without this PR, Cinder in MSW with cmake is stuck in MT, no other choice.

You can see my issue of #2116

[fei4xu]

Yes, I agree with you, Cinder should not introduce too many compiler options.
Another possible PR chould be just removing line https://github.com/cinder/Cinder/blame/master/proj/cmake/modules/cinderMakeApp.cmake#L113 to line 123.

[MikeGitb]

This PR is not to force MT, it's actually removingthe forcing of MT.

I got that, but introducing a separate option for this is imho the wrong solution to the problem. As you suggested, just removing the flag is imho the right thing to do, assuming there wasn't a very, very good reason for defaulting to MT in the first place.

[richardeakin]

I think the rational behind forcing MT (static) is that this is how the prepared VS proj files are setup, and we wanted them to be compatible. I'm not sure if that is so important - what is the standard way with CMake on MSVC to set it up for MT?

I think one alternative option would be to introduce some cinder specific setting to disable this, or we could just remove those lines that force the non-default runtime library... though curious what the usual config process would be if we do that.

[fei4xu]

I updated the PR to just remove the forcing code.

In the future, if some application would like to change to "/MT", it's the application's choice to use

# Override the default /MD with /MT
foreach(
	flag_var
	CMAKE_C_FLAGS CMAKE_C_FLAGS_DEBUG CMAKE_C_FLAGS_RELEASE CMAKE_C_FLAGS_MINSIZEREL CMAKE_C_FLAGS_RELWITHDEBINFO
	CMAKE_CXX_FLAGS CMAKE_CXX_FLAGS_DEBUG CMAKE_CXX_FLAGS_RELEASE CMAKE_CXX_FLAGS_MINSIZEREL CMAKE_CXX_FLAGS_RELWITHDEBINFO
)
	if( ${flag_var} MATCHES "/MD" )
		string( REGEX REPLACE "/MD" "/MT" ${flag_var} "${${flag_var}}" )
		set( "${flag_var}" "${${flag_var}}" PARENT_SCOPE )
	endif()
endforeach()

[fei4xu]

What's your thought on this? @MikeGitb @richardeakin

Whatever solution you might decide, I really want to fix the forcing of MT and I can update the PR according to your decision. This is a road blocker for us now.

[richardeakin]

Is there a procedure for switch to /MT from the cmake command line? You have to use a regex string replace for something this common? I would rather think it was a matter of specifying a static versus dynamic linking.

Also noting that the cmake windows docs here are clearly out of date at this time, perhaps we should update them once the decision here has been made.

[MikeGitb]

No, I don't think cmake offers a nice command line way to do this before 3.15 (https://cmake.org/cmake/help/v3.15/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html#variable:CMAKE_MSVC_RUNTIME_LIBRARY).

I tend to put stuff like that into my toolchain files (which is why I don't like it if a library overrides my decision).

EDIT: Again, what is the original motivation for cinder to statically link to the runtime?

[fei4xu]

@MikeGitb: As @richardeakin has mentioned at #2117 (comment), the original motivation to use MT is to make the cmake setting compatible with *.vcproj system. Currently, Cinder has 2 working project systems on MSW, one is vs (.sln, .vcproj) files, the other is the cmake system. The VS files has setting of MT, both the Cinder library and all the samples.

[MikeGitb]

the original motivation to use MT is to make the cmake setting compatible with *.vcproj system

I know but what I wanted to know is, why MT was chosen as the default in the first place (i.e. in the vcproj files).

I mean, is there a cinder specific reason (it doesn't work with MD) or is it "just" what the maintainers/users use in most of their projects and hence set this flag for compatibility reasons?

[fei4xu]

not sure if @chaoticbob can review this?

[paulhoux]

I know it's been a while since this PR was opened, but I was looking into the same problem today and came across this discussion.

@MikeGitb : regarding the choice to statically link Cinder by default: historically, Cinder did not support dynamic linking. It was not until @simongeilfus started experimenting with live C++ coding that the need arose to dynamically link the Cinder library. His work was merged in commit #1770 .

The current master branch's CMake files do not support creating a dynamically linked library. The Visual Studio project files, however, have the Debug_Shared and Release_Shared targets that allow you to successfully build Cinder as a DLL.

Because I myself am starting to use CMake as my build system of choice, I would also like to have a clean way of using CMake to build a dynamic library.

[paulhoux]

As you can see, I have also made a Pull Request (#2133) which addresses a few more issues with building Cinder as a shared library using CMake.

[MikeGitb]

You are probably aware of this, but just to make sure: /MT vs /MD are about linking the crt statically or dynamically and are orthogonal to building cinder as a static or dynamic lib.

[MikeGitb]

@cinder maintainers: Please merge!

[andrewfb]

Sorry to be so negligent in this one. The reason Cinder uses /MT over /MD is to maximize portability, ie to avoid the requirement that end-users have the correct vc_redist package installed. This isn't a problem for developers running their apps on their own machines but can be a complicating element for widely distributed binaries.

A separate but related problem is that 3rd party dependencies must be built with the matching runtime, so in some senses Cinder needs to select a standard for packages like OpenCV or Cairo for which we've traditionally shipped binaries.

Both of these issues are of course solvable for experienced developers, and I agree with the spirit of the comment that Cinder should respect what is asked of it via CMake, though I myself never use CMake on Windows so I can't comment on the mechanism. I think ideally we would default to /MT for maximal compatibility and then it would be possible to override that manually - is something like that possible?

[MikeGitb]

I think ideally we would default to /MT for maximal compatibility and then it would be possible to override that manually - is something like that possible?

I think - as mentioned before - prior to cmake 3.15, that requires an explicit, cinder-specific option. I think @fei4xu's original PR had that before I talked him out of it (sorry for that).

Personally I don't like adding such an option and having a non-standard default for the reasons given at the beginning, but I'm saying this from the perspective of someone adding cinder to an existing cmake -project and/or integrating it into a package manager, not from someone living in the cinder eco-system.

[kino-dome]

I remember having a headache because of this when trying to make Cinder and dlib work together. In my case I had built dlib using their own recommended routine with Cmake and then wanted to use the built lib in conjunction with Cinder in VS Studio (where the defaulting to /MT came to haunt me). I was in a hurry and ended up ditching building dlib in Cmake and just used its source files in my VS project. It wasn't pretty and I don't think this solution fits all situations.

[MikeGitb]

That complication is pretty much, why I don't like it when every lib invents its own way to specify MT or MD. Using the standard mechanism means the user only has to figure it out once for all cmake projects.

Food for though: We are already requiring VS2019 to compile cinder with msvc. If a user has that, he alsmost certainly also uses a cmake version newer than 3.14 (currently VS2019 shipps with cmake 3.17). Cmake 3.15 introduced the CMAKE_MSVC_RUNTIME_LIBRARY mechanism, which in turn allows the user to specify static vs dynamic in a very convenient manner from the command line via e.g. cmake -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded.

Contrary to the raw /MT flag, it is actually possible to check if the user requested a particular value for CMAKE_MSVC_RUNTIME_LIBRARY and if not, set it to whatever default you prefer and is compatible with the precompiled binaries in the cinder repository.

[richardeakin]

Just reviewed this discussion, and it seems like progress on this has been stalemated because of libcinder's history of wanting to link everything statically to redistribution purposes. As I see it, what we need to do is twofold:

1. What is the default option. My current mindset is that we shouldn't hinge our decisions on keeping the two build systems (cmake and handrolled vcproj) compatible with each other, as much as following good practices for build systems. To that effect, will suggest that we make the default match the majority of other projects out there, which is to not add the /MT flag by default.
2. There should still be an option to enable it from CMake, if for nothing else then reverse compatibility reasons. To me, that cmake only has a flag for this in recent versions is a side effect of progress, and if we can't use it then let's just add a flag and move on. We should also document this change and option on the cmake guide.

[MikeGitb]

What is the default option. My current mindset is that we shouldn't hinge our decisions on keeping the two build systems (cmake and handrolled vcproj) compatible with each other, as much as following good practices for build systems.

The argument from andrewfb that kind of convinced me to stay with /MT as a default is that only then will linking with the prebuilt binaries continue to work out of the box (if I understood correctly).

To me, that cmake only has a flag for this in recent versions is a side effect of progress, and if we can't use it then let's just add a flag and move on.

It's not like this is all that important to me, but as the windows build already requires newer toolchains and c++ standard than the builds for linux and mac, I don't think it would be a problem to require a newer version of cmake.