Fix handling of tools.build:defines for multiconfig CMake (#15921)

[stevenwdv]

Changelog: Bugfix: Fix handling of tools.build:defines for multiconfig CMake.
Docs: Omit

Closes #15921

The fix is not as simple as fixing the quote spacing. The problem is CMake generator expressions, which are a pain to work with. We can't just pass multiple strings to one generator expression, since the expression needs to be in one string argument to add_compile_definitions.
This PR fixes it similarly to how it is done in:

conan/conan/tools/cmake/toolchain/toolchain.py

Lines 122 to 132 in 19b7cf6

	{%- for (conf, value) in values %}
	{% if value is none %}
	set(CONAN_DEF_{{conf}}_{{name}} "{{name}}")
	{% else %}
	set(CONAN_DEF_{{conf}}_{{name}} "{{name}}={{value}}")
	{% endif %}
	{% endfor %}
	add_compile_definitions(
	{%- for (conf, value) in values %}
	$<$<CONFIG:{{conf}}>:${CONAN_DEF_{{conf}}_{{name}}}>
	{%- endfor -%})

But it uses quoted strings instead of variables.

Note that this still is not perfect, since we should really escape characters in the generator expression, such as > as $<ANGLE-R>, which would otherwise close the generator expression, and $ as $<1:$>, but also ; as \; to prevent variables containing it to be split as a list. But this issue is also present with the variable-based approach in toolchain.py. I'll create a separate issue for this See #15926.

• Refer to the issue that supports this Pull Request.
• If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
• I've read the Contributing guide.
• I've followed the PEP8 style guides for Python code.
• I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

[CLAassistant]

All committers have signed the CLA.

[memsharded]

Maybe a failing test first could help, would you like our help to provide one?

[stevenwdv]

I'm not sure what exactly I should do for this, but let me clarify the problem a bit more.

If I have a profile containing two defines for a multiconfig CMake generator like MSVC:

[settings]
arch=x86_64
build_type=Release
compiler=msvc
compiler.cppstd=20
compiler.runtime=dynamic
compiler.runtime_type=Release
compiler.version=192
os=windows
[conf]
tools.build:defines=['_WIN32_WINNT=0x0A00', 'NTDDI_VERSION=0x0A000006']

... and then do a conan install with the CMakeToolchain, it puts the following in conan_toolchain.cmake:

add_compile_definitions($<$<CONFIG:Release>:" _WIN32_WINNT=0x0A00"" NTDDI_VERSION=0x0A000006">)

After a CMake configure, this produces the following .vcxproj:

...
<ClCompile>
  <PreprocessorDefinitions>...;" _WIN32_WINNT=0x0A00"" NTDDI_VERSION=0x0A000006";...</PreprocessorDefinitions>
</ClCompile>
<ResourceCompile>
  <PreprocessorDefinitions>...;\" _WIN32_WINNT=0x0A00\"\" NTDDI_VERSION=0x0A000006\";...</PreprocessorDefinitions>
</ResourceCompile>
...

which MSVC does not interpret as intended.

---

With the new solution, it will output:

add_compile_definitions("$<$<CONFIG:Release>:_WIN32_WINNT=0x0A00>" "$<$<CONFIG:Release>:NTDDI_VERSION=0x0A000006>")

which generates

...
<ClCompile>
  <PreprocessorDefinitions>...;_WIN32_WINNT=0x0A00;NTDDI_VERSION=0x0A000006;...</PreprocessorDefinitions>
</ClCompile>
<ResourceCompile>
  <PreprocessorDefinitions>...;_WIN32_WINNT=0x0A00;NTDDI_VERSION=0x0A000006;...</PreprocessorDefinitions>
</ResourceCompile>
...

[memsharded]

Thanks for the feedback.

I have added some checks in test_cmake_toolchain_cxxflags_multi_config test in 3000b88

The overlook in that test was adding only 1 define, while the changes are not correctly handling more than 1 define.
The test will make the PR fail, at least locally, the test was not passing with the current proposed changes.

[memsharded]

Uhmmm, there might be something there still not right.
I have tested with the previous:

add_compile_definitions($<$<CONFIG:{{config}}>:{% for define in defines %}" {{ define }}"{% endfor %}>)

and the test now passes with 2 values.

We would need a failing test first, to make sure we are covering and correctly fixing it.

[memsharded]

I identified one issue with the test: the validation of the output was not fully correct.
I introduced a more detailed check, validating assert 'DEFINE conan_test_answer=42!' in c.out, one definition per line, and then it started to fail.

I was checking add_compile_definitions() and I agree there is something weird there, it seem CMake was processing it wrong, it might be a bug in CMake. Then I realize that add_compile_definitions() is additive, it can be called many times, and then I changed the template to call it once per define, and it seems to be working now, please check.

[stevenwdv]

@memsharded Thanks for adding the tests. I think the change introduces an issue, though. The value should be quoted: add_compile_definitions("$<$<CONFIG:{{config}}>:{{ define }}>"), maybe add a test with spaces in the define as well to catch this?
Apparently what went wrong with the previous approach is that there were no spaces between the quotes:

add_compile_definitions("$<$<CONFIG:Release>:ABC=123>""$<$<CONFIG:Release>:DEF=456>")

should have been

add_compile_definitions("$<$<CONFIG:Release>:ABC=123>" "$<$<CONFIG:Release>:DEF=456>")

which does work.
That's what I get for not testing and assuming my fix would just work...

[memsharded]

Makes sense, thanks for the investigation and feedback.

I have pushed a fix for the broken test, quoting the defines, I think it should be ok now, please check.
I have kept the 1 define per add_compile_definitions(), it seems more robust and even reads slightly cleaner in my opinion, compared with the quoted strings separated by spaces, I hope it is ok.

[stevenwdv]

@memsharded This does not seem to work. For example:

add_compile_definitions($<$<CONFIG:Debug>:"ABC=123">)
add_compile_definitions($<$<CONFIG:Debug>:"DEF=456">)

generates: -D"ABC=123\" -D"DEF=456\" in build.ninja

Compare to:

add_compile_definitions("$<$<CONFIG:Debug>:ABC=123>")
add_compile_definitions("$<$<CONFIG:Debug>:DEF=456>")

which properly generates: -DABC=123 -DDEF=456

[memsharded]

Omg, we have just released 2.2.2 with this fix... :(

Still, the thing is that the test passes. We would need to add some tests that cover the case and fail. The current tests are working.
I have tried to change the test to use Ninja as you commented, but it is also working (I am not checking the exact command line or -D argument generated, but I can see that the result in the compiled binary got the preprocessor definitions correctly apparently.

[stevenwdv]

@memsharded I'm not sure what we're doing differently, but if I test with the first pair of add_compile_definitionss from my comment above (simply in a CMakeLists.txt) and build with Ninja, I get the following:

/home/swdv/test/test.c:4:9: warning: The value of ABC: 123" -DDEF=456" [-W#pragma-messages]
    4 | #pragma message "The value of ABC: " XSTR(ABC)
      |         ^
/home/swdv/test/test.c:5:9: warning: The value of DEF: DEF [-W#pragma-messages]
    5 | #pragma message "The value of DEF: " XSTR(DEF)

[memsharded]

If you could introduce something like #pragma message in the code of the test, with some checks that make the test fail, that is perfect. Those appear like warnings, but not errors, maybe it is just the pragma message affected by the quotes but not other preprocessor directives?

[stevenwdv]

If you could introduce something like #pragma message in the code of the test, with some checks that make the test fail, that is perfect.

I'll have a look.

Those appear like warnings, but not errors

#pragma message is always displayed as "warning" by the compiler.

maybe it is just the pragma message affected by the quotes but not other preprocessor directives?

Doesn't seem like it:

/home/swdv/test/test.c:7:11: error: expected ';' after top level declarator
    7 | int abc = ABC;
      |           ^
<command line>:1:16: note: expanded from macro 'ABC'
    1 | #define ABC 123" -DDEF=456"
      |                ^
/home/swdv/test/test.c:8:11: error: use of undeclared identifier 'DEF'
    8 | int def = DEF;
      |           ^

[stevenwdv]

@memsharded I don't really understand what's going on. I tried the C code from the test separately with the relevant add_compiler_definitionss, and got this output:

DEFINE answer=answer!
DEFINE conan_test_answer=42" -Dconan_test_other=24"!

Which is in line with the #pragma message test.
So the test should already detect the problem.

I'll check if I'm missing something by actually using Conan with Ninja Multi-Config.

[stevenwdv]

I'll check if I'm missing something by actually using Conan with Ninja Multi-Config.

Yes this outputs the same, as expected.

Maybe it's not broken for MSVC for a weird reason? I'm not on my Windows pc to test atm.

[stevenwdv]

Apparently, this is how it comes out for Visual Studio:

<ClCompile>
    ...
    <PreprocessorDefinitions>%(PreprocessorDefinitions);"_WIN32_WINNT=0x0A00";"NTDDI_VERSION=0x0A000006";CMAKE_INTDIR="Release"</PreprocessorDefinitions>
</ClCompile>
<ResourceCompile>
  <PreprocessorDefinitions>%(PreprocessorDefinitions);WIN32;\"_WIN32_WINNT=0x0A00\";\"NTDDI_VERSION=0x0A000006\";CMAKE_INTDIR=\"Release\"</PreprocessorDefinitions>
</ResourceCompile>

This does seem to work on my pc (the macros are actually defined), although I thought the CI did have problems with it, not sure. In any case, it does not work well with Ninja Multi-Config, as stated above.

[DenizThatMenace]

@memsharded, I can confirm that the current state fails with Ninja Multi-Config.

The correct fix is to just add quotes around the generator-expression instead of putting them inside it.
(CMake generator-expressions should never contain (non-escaped) quotes!)

Therefore, as @stevenwdv already mentioned here the code in question should look like this:

--- a/conan/tools/cmake/toolchain/blocks.py
+++ b/conan/tools/cmake/toolchain/blocks.py
@@ -724,7 +724,7 @@ class ExtraFlagsBlock(Block):
         {% if defines %}
         {% if config %}
         {% for define in defines %}
-        add_compile_definitions($<$<CONFIG:{{config}}>:"{{ define }}">)
+        add_compile_definitions("$<$<CONFIG:{{config}}>:{{ define }}>")
         {% endfor %}
         {% else %}
         add_compile_definitions({% for define in defines %} "{{ define }}"{% endfor %})

---

Note: Of course, (non-escaped) quotes (and some other symbols) inside of define will still make the generator-expression faulty.

edit: Forget the former not. As CMake's documentation for add_compile_definitions clearly says:

CMake will automatically escape the value correctly for the native build system (note that CMake language syntax may require escapes to specify some values).

However, the sentence in parentheses still leaves some possibility for problems.

[stevenwdv]

@DenizThatMenace

However, the sentence in parentheses still leaves some possibility for problems

FYI, #15926 and maybe #15928 should cover this, I think.