Bug - MSBuild helper should not force compiler flags such as /o

[solvingj]

It seems that the .props file used to inject a few things implicitly into the build sometimes results in conflicts as seen here:

     2>cl : Command line warning D9025: overriding '/O2' with '/Od' [path_to_my_project.vcxproj]
     2>cl : Command line warning D9025: overriding '/Od' with '/O2' [path_to_my_project.vcxproj]
    <..... above pair of message seen dozens of times.>
     2>cl : Command line error D8016: '/RTC1' and '/O2' command-line options are incompatible 

This can happen in very common circumstances, including any projects that have "Runtime checks" enabled, which I believe is the default for DLL projects.

Eventually, would be good to validate whatever flags are being injected.

• I've read the CONTRIBUTING guide.
• I've specified the Conan version, operating system version and any tool that can be relevant.
• I've explained the steps to reproduce the error or the motivation/use case of the question/suggestion.

[lasote]

Help me understand the issue. Who is setting the /O2 flag? is it an explicit flag appended in the conanfile?

Eventually, would be good to validate whatever flags are being injected.

What is the validation you see the build helper could perform?

[solvingj]

The problem in our environment was that a Visual Studio .sln file configuration of Release was mis-mapped to a Visual Studio .vcxproj file of Debug. Since Conan believed it was building for Release, it set typical release level optimization of /O2 which should not have been a problem, but this conflicted with the default compiler flag for Debug builds: /RTC. Sorry for the trouble.

[lasote]

No problem. You think we could introduce some check?

[solvingj]

Not unless you get another report of it. I guess that last question is, why is Conan even injecting O2 via the props file in the first place? I'm willing to believe there's a good reason, but it's completely unexpected. I think most users would assume that the project would use the optimization level in their .vcxproj, or use whatever defaults that it would use on their local machine.

[solvingj]

This just came up again. I spoke with @memsharded . I just setup our biggest project and the -O2 is breaking the build. Here it's causing a very strange issue. He wasn't sure why -O2 is being injected either. Furthermore, here it looks like the helper is meddling in several options: https://github.com/conan-io/conan/blob/release/1.7.4/conans/client/build/compiler_flags.py#L92

Based on the comment, it looks like maybe the defaults were borrowed from Cmake. If only the RuntimeLibrary needs to be injected, then I think that's all we should inject.

[lasote]

This is a bit tricky, the O2 as you mentioned, is set by the default compiler flags according to the build_type=Release. Right? I think this is correct, because the build helpers adjust the needed flags according to the received settings.

Where do the RTC1 flag comes from? If it is a default in some cases (DLL?) Conan could check it and remove the O2. But if the RTC1 is just a project configuration I think the recipe should remove the O2 and any other offending flag:

msbuild = MSBuild(self)
msbuild.build_env.flags.remove("-O2")
msbuild.build()

I am missing something?

[solvingj]

RTC1 was my mistake, this can be ignored. Here I'm only suggesting changing of O2 behavior.

Indeed, the build helpers are adjusting O2 according to the received settings, but I think this behavior is arbitrary and unnecessary. O2 is optimization level, and should only be set in the VCXPROJ, or manually overridden by the user, or allow MSBuild to choose the default value. I'm saying it should never implicitly overridden by Conan.

With that said, maybe there is some reason this must be done, which I am not aware of.

[lasote]

About RTC1, got it. About the /O2 probably you are totally right, but as I understand Release/Debug/RelWithDebInfo/MinSizeRel are abstract concepts that should be mapped to common flags. As I read, "Release" implies some optimization level, so I don't know why do you think this is arbitrary. Please, help me understand why.
Conan, in the MSBuild helper, could offer a better interface to adjust/change the optimization, for sure, but the same way Conan forces the runtime flag (unless the recipe author changes it in the recipe), shouldn't Conan adjust the optimization level?

Thanks

[solvingj]

Ok, now I understand why it was done this way. In that case, I think this is pretty simple to resolve. It looks like Conan derived the idea of setting O2 as the default for Release builds from CMake's behavior. And, since it was found that Conan had to inject a .props file to force the compiler.runtime value into the build, it probably seemed logical and trivial to also force this seemingly related default. However, there are two significant distinctions which I think were overlooked.

First, CMake generates .vcxproj files from scratch, so it does not need to consider optimization levels already embedded into the .vcxproj by the project author. If there's a desired optimization, it's specified by the project author in the CMakeLists.txt file, and the .vcxproj is generated with the corresponding value. Conversely, with Conan and the MSBuild helper, we're always building existing .vcxproj files provided by a project author with existing optimization levels pre-defined. Thus, the default behavior for MSBuild helper should be to respect the project's existing setting, and not forcibly override it based on a commonly-used default.

Second, there's a big difference between the optimization level and the MSVC runtime, which are both injected together by the MSBuild helper. When running Conan on a recipe that uses the Visual Studio compiler, the setting of compiler.runtime is required to be specified (by default, of course it can be removed, but thats beside the point). By default, it will be a critical and well-known factor in defining ABI compatibility. Most importantly, it's explicit and the user has full control over it. For these reasons, the assumed and desired behavior 99% of the time will be for Conan to ensure that the setting passed to Conan will be the setting used by the build. Obviously, optimization is very different. There's no built-in mechanism for the user to specify the value they want, it isn't part of the ABI by default, and is just like all the other un-modeled compiler flags. If Optimization were to become a default Conan sub-setting for the Visual Studio compiler, then it would probably make sense to force it into the build. However, the desired behavior MOST of the time, is to use the optimization level defined in the project.

I hope this provides some helpful perspective and insight. please let me know if you'd like to discuss further.

[memsharded]

Ok, I think it makes sense to remove them, and leave the project defined ones.

[solvingj]

Changing title to reflect current intention.