Fix handling of tools.build:defines for (Ninja) multi-config CMake CMake (#15921)

[DenizThatMenace]

Changelog: Bugfix: Fix handling of tools.build:defines for Ninja Multi-Config CMake.
Docs: Omit
Fixes: #15921

The CMake generator-expressions that results from the values given to Conan's tools.build:defines configuration will now properly be put into quotes to form a CMake string. (Note: CMake's generator-expressions should never contain any (non-escaped) quotes.)

• 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]

Thanks for your contribution @DenizThatMenace

It would be necessary first to contribute a test that shows that this change is necessary (red without the fix, green with the fix). Thanks!

[DenizThatMenace]

Thanks for your contribution @DenizThatMenace

It would be necessary first to contribute a test that shows that this change is necessary (red without the fix, green with the fix). Thanks!

Why do you need this, if you are clearly using wrong CMake syntax?
Quotes should be around generator-expressions, not put into them. (See: https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#whitespace-and-quoting)

[memsharded]

Why do you need this, if you are clearly using wrong CMake syntax?

To protect against future breaking of this change. If it is not covered by tests it can be broken at any time by a refactor, an update, other fixes... The only way to move these things forward is to have tests that cover the functionality. We always add tests or new checks for practically all of our PRs.

[DenizThatMenace]

To protect against future breaking of this change.

I understand why it is good to have tests.

But even if there is a test for this specific case, this will not help if someone writes similar code somewhere else in Conan's code base and puts quotes inside of CMake generator-expressions. This is always wrong! (One might get away with it in some situations but it is brittle and might break with another CMake version.)

Sadly, I do not know enough Python or Conan's code base to know how to quickly adjust and run the tests of Conan to extend/fix the tests.
But I know CMake and I am very sure that putting quotes into generator-expressions is wrong.

---

Regarding the current problem in question:
I do not understand, why you seem to be unable to reproduce the error. Have you actually tried to use the generated conan_toolchain.cmake file?
Because, whether I run it on Windows or on Linux, whether with Clang or MSVC, I get the same error.

For example:

The generated conan_toolchain.cmake contains:

Then, the resulting error looks like this:

If I remove mymacro3 from the list of defines, it compiles. If I additionally remove mymacro2 from the list of defines, it fails again.
(The number of non-escaped quotes very likely has something to do with.)

[memsharded]

I understand why it is good to have tests.

It is not only good, it is fundamentally necessary for a project and a codebase like Conan to make sure things are properly covered by tests to avoid continuous regressions and problems.

But even if there is a test for this specific case, this will not help if someone writes similar code somewhere else in Conan's code base and puts quotes inside of CMake generator-expressions.

Whatever someone else does in the codebase elsewhere is irrelevant if a test that captures the failing cases cover it, it will not be a problem, and possible regressions will be captured. We carefully check for test modifications and modifying the behavior of an existing test is an immediate red flag.

Sadly, I do not know enough Python or Conan's code base to know how to quickly adjust and run the tests of Conan to extend/fix the tests.

It is not necessary to know anything about the codebase to do a new test, and the test framework makes reproducing things very straightforward, just by copying and modifying a test like

conan/test/functional/toolchains/cmake/test_cmake_toolchain.py

Line 1831 in dd28339

def test_cmake_toolchain_cxxflags_multi_config():

, look how the tests look like:

def test_cmake_toolchain_cxxflags_multi_config():
    c = TestClient()
    profile_release = textwrap.dedent(r"""
        include(default)
        [conf]
        tools.build:defines=["conan_test_answer=42", "conan_test_other=24"]
        """)
    profile_debug = textwrap.dedent(r"""
        include(default)
        [settings]
        build_type=Debug
        ..
        """)

    conanfile = textwrap.dedent(r'''
        from conan import ConanFile
        from conan.tools.cmake import CMake, CMakeToolchain, cmake_layout

        class Test(ConanFile):
            ...
        ''')

    main = textwrap.dedent(r"""
        #include <iostream>
        ...
        }
        """)

    cmakelists = textwrap.dedent("""
        cmake_minimum_required(VERSION 3.15)
        project(Test CXX)
        add_executable(example src/main.cpp)
        """)

    c.save({"conanfile.py": conanfile,
            "profile_release": profile_release,
            "profile_debug": profile_debug,
            "src/main.cpp": main,
            "CMakeLists.txt": cmakelists}, clean_first=True)
    c.run("install . -pr=./profile_release")
    c.run("install . -pr=./profile_debug")

    with c.chdir("build"):
        c.run_command("cmake .. -DCMAKE_TOOLCHAIN_FILE=generators/conan_toolchain.cmake")
        c.run_command("cmake --build . --config Release")

In any case, don't worry, it is not a problem, we will try to contribute a test when possible. We tried before and we couldn't make it fail in a test, so it might take a while, hopefully we can reproduce in a test at some point.

[DenizThatMenace]

It took me quite some time to figure it out, but I was able to install all test-requirements, run/debug tests and create a new one that can reproduce the error and which fails if my fix is missing.

---

On a side-note: I do not understand how the tests can work with any CMake version.

I had to locally modify my test to always use CMake 3.23.5 and create a symlink from /usr/share/cmake-3.23.5/bin to /usr/bin, where my actual CMake 3.29 executable is located.

To me the discovery of CMake looks totally broken. (But maybe I did something wrong?)

[memsharded]

Good job. I have managed to get the tests passing in the 3 platforms, they were initially failing because of some minor details in Linux/OSX (the cmake version), and some of the typical Windows stuff.

This can be moved forward, thanks for your contribution!

[memsharded]

Regarding the tests and CMake configuration, it works via conftest.py and conftest_user.py (documented in the conftest.py). It can use some improvements and better documentation, it is in our to-do list.