Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: mention that -MTd is not support with MSVC at the moment #495

Closed
OlivierLDff opened this issue Mar 29, 2023 · 11 comments · Fixed by #496
Closed

doc: mention that -MTd is not support with MSVC at the moment #495

OlivierLDff opened this issue Mar 29, 2023 · 11 comments · Fixed by #496
Labels
📖 documentation Improvements or additions to documentation

Comments

@OlivierLDff
Copy link
Contributor

Hi,
Currently experimenting on windows, with msvc, it was quite annoying not to have the Debug build working out of the box. I investigate a bit and ended up there: rust-lang/cc-rs#717.

So you know the problem, and it seems out of hand for the moment.

It would be really nice to have a propose workaround. What I'm doing is simply adding -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL when doing the configure cmake step. I've also seen people only setting CXXFLAGS="-MTd" as env variable. But this seems even more hacky that simply assuming we are stuck with MT, so there won't be any debugging support/assert stuff in stl. Which is not a big deal when doing qt app anyway.

So 2 ideas here:

  • Mention it in the Readme
  • Create a CMakePresets.json for msvc that just set the variable.

What is your opinion?

@Be-ing
Copy link
Contributor

Be-ing commented Mar 29, 2023

You can also build in release mode.

@OlivierLDff
Copy link
Contributor Author

Yes of course, but I want to integrate cxx-qt in an existing code base, so it is a no go if I can't use Debug mode anymore.

@OlivierLDff
Copy link
Contributor Author

Here an example of CMakePresets.json that would make on boarding easy of windows users:

{
    "version": 5,
    "cmakeMinimumRequired": {
        "major": 3,
        "minor": 24,
        "patch": 0
    },
    "configurePresets": [
        {
            "name": "base-dev",
            "binaryDir": "${sourceDir}/build/${presetName}",
            "hidden": true
        },
        {
            "name": "msvc-17",
            "inherits": "base-dev",
            "displayName": "Visual Studio 2022",
            "generator": "Visual Studio 17 2022",
            "description": "Configure using Visual Studio 17 2022 generator",
            "condition": {
                "type": "equals",
                "lhs": "${hostSystemName}",
                "rhs": "Windows"
            },
            "cacheVariables": {
                "CMAKE_MSVC_RUNTIME_LIBRARY": "MultiThreadedDLL"
            }
        }
    ],
    "buildPresets": [
        {
            "name": "msvc-17-debug",
            "displayName": "Debug",
            "description": "Build in Debug",
            "configuration": "Debug",
            "configurePreset": "msvc-17"
        },
        {
            "name": "msvc-17-minsizerel",
            "displayName": "MinSizeRel",
            "description": "Build in MinSizeRel",
            "configuration": "MinSizeRel",
            "configurePreset": "msvc-17"
        },
        {
            "name": "msvc-17-relwithdebinfo",
            "displayName": "RelWithDebInfo",
            "description": "Build in RelWithDebInfo",
            "configuration": "RelWithDebInfo",
            "configurePreset": "msvc-17"
        },
        {
            "name": "msvc-17-release",
            "displayName": "Release",
            "description": "Build in Release",
            "configuration": "Release",
            "configurePreset": "msvc-17"
        }
    ]
}

@ahayzen-kdab
Copy link
Collaborator

For a CMakePresets.json i would note that this only helps people who clone the repository, so people who are hacking directly on CXX-Qt rather than being a consumer ?

This does not help people who

  • Follow the tutorial create their own project
  • Use crates.io to retrieve CXX-Qt

So ideally an upstream solution would be best that helps everyone, then i don't know if we could set anything in the build script to help things or is that too late ? Then education in the README and/or book would likely help as well.

Adding a CMakePreset may also be of value too, but we should explore or a more general fix can be done first ?

@OlivierLDff
Copy link
Contributor Author

CMakePreset is always cool even for this repo, it provide cool IDE integration (CLion/vscode).

a more general fix can be done first

@Be-ing already done a PR that fix the bug in rust-lang/cc-rs#717

Workaround with warning would be to add .flag("-MTd") to cc::Build in build.rs. But I'm not sure what is the best strategy to convey this information from CMake that we are building in Debug to build.rs.

I guess generator expression then pass an env variable to cargo build is the way. But it is way more work that -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL.

We could also add no_default_flags to cc::Build, and add flag from CMake for every compiler, but then this become a job for every compiler on every platform to replicate the default just to take care of msvc issue.

I guess it should be mentioned in

If this fails for any reason, take a look at the [`examples/qml_minimal`](https://github.com/KDAB/cxx-qt/tree/main/examples/qml_minimal) folder, which contains the complete example code.
a special note about Windows/Msvc.

What about something like:

If you're building qt-cxx on Windows using MSVC generator, you may need to add the `-DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL` flag to the cmake command when building with the `Debug` configuration. This flag is necessary to ensure that the correct C Runtime Library is used. Then you can build using `cmake --build build --config Debug`.

This issue is caused by a bug in the [cc](https://docs.rs/cc/latest/cc/index.html) crate (as described in https://github.com/rust-lang/cc-rs/pull/717), which has not been merged yet. Specifically, the problem is that cc generated code always links to the MultiThreaded runtime, even when building in Debug mode. We hope that this step won't be necessary in the future, once the cc crate fix is merged and released.

@ahayzen-kdab
Copy link
Collaborator

@Be-ing already done a PR that fix the bug in rust-lang/cc-rs#717

Yup :-)

Workaround with warning would be to add .flag("-MTd") to cc::Build in build.rs. But I'm not sure what is the best strategy to convey this information from CMake that we are building in Debug to build.rs.

Hm, does corrosion not already tell cargo to build in debug when CMake is ? I can't remember if this is happening already or not. As ideally i guess it would be in CxxQtBuilder of the build script if windows + debug build then set the flag in cc for now

I guess generator expression then pass an env variable to cargo build is the way. But it is way more work that -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDLL.

We could also add no_default_flags to cc::Build, and add flag from CMake for every compiler, but then this become a job for every compiler on every platform to replicate the default just to take care of msvc issue.

Note that does this issue also occur with non-CMake builds using Cargo-only builds ? As this is why the build script is generally the right place (other than upstream of course) for fixing issues as it solves both CMake and Cargo-only builds.

@ahayzen-kdab ahayzen-kdab added the 📖 documentation Improvements or additions to documentation label Mar 30, 2023
@OlivierLDff
Copy link
Contributor Author

It is not a problem for Cargo only build, because then the whole C++ code in the static library is always linked with -MT. It get a problem when you try to mix CMake built code (in qml-minimal, that would be main.cpp) with cc::Build built code.

That cause issue for example when using stl, as some #define are not the same, resulting in incompatible code for template class.

@ahayzen-kdab
Copy link
Collaborator

Ah fun :-/ I'll let @Be-ing comment on further suggestions as he has been battling the build side of things, but it feels like a minimum there should be documentation somewhere, and then either a template or the examples having something in the CMake to show how to make this work in Windows.

Also, i assume this isn't a generic enough CMake + Rust problem that this change should be in Corrosion ? Eg if you are using Windows MSVC in Debug mode and a CMake Corrosion import of a Rust crate would the issue occur ? Or is it limited to specifically how we are linking things ?

@OlivierLDff
Copy link
Contributor Author

The error occurs with corrosion yep. It is directly caused by the fact that the runtime library to be used is hardcoded as a default value in cc crate. Corrosion could handle the problem, if the cc crate would give some way to overwrite -MT with -MTd.

A solution, not affecting the default could be add a feature crt-debug or something like that there: https://github.com/rust-lang/cc-rs/blob/58e66f5beca3440c04a6a840bfe9e5bf60cf0dc5/src/lib.rs#L1591-L1594. So that corrosion could add in the env variable CARGO_CFG_TARGET_FEATURE the crt-debug.

@ahayzen-kdab
Copy link
Collaborator

Hmm, so while we wait for upstream fixes, is there anything that could be done in Corrosion, why can't it set the same CMake definitions that you are setting in our CMake ? (if there is no cargo feature for now).

As then should we open an issue in Corrosion to allow for this ? Or is the problem still no generic enough ?

@OlivierLDff
Copy link
Contributor Author

It's a "global variable" so I don't think any script should write it.
It can also be set per target, but then corrosion has to find all target that depends on the target to set the property on it. all target might ne be defined at the point of calling corrosion.
Plus corrosion forcing MT would block user that want to link to static crt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants