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

[question] How to make conan 1.59.0 add "architecture": "x64" to CMakePresets.json? #13792

Closed
1 task done
garyo opened this issue Apr 29, 2023 · 24 comments · Fixed by #15215
Closed
1 task done

[question] How to make conan 1.59.0 add "architecture": "x64" to CMakePresets.json? #13792

garyo opened this issue Apr 29, 2023 · 24 comments · Fixed by #15215

Comments

@garyo
Copy link

garyo commented Apr 29, 2023

What is your question?

I'm using conan 1.59 on Windows, with cmake 3.23.0. I find that if I run my build script (https://github.com/AcademySoftwareFoundation/openfx/tree/cmake) twice, it fails the second time with this error:

CMake Error: Error: generator platform:
Does not match the platform used previously: x64
Either remove the CMakeCache.txt file and CMakeFiles directory or choose a different binary directory.

This error, which happens during cmake --preset default, is apparently because the CMakePresets.json that conan generates is missing the configurePresets[0].architecture flag, which should be set to x64. It seems like setting CMAKE_GENERATOR_PLATFORM in conan_toolchain.cmake is not enough to prevent the above error.

How can I make conan create a cmake preset that includes the architecture flag? Or if there's a different way to fix this, that would be great too.

You can repro this with the above open-source github repo. Clone it on Windows with conan 1.59.0 and cmake 3.23.0, check out the cmake branch, then run build-cmake.sh twice (using an msys bash shell).

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded memsharded self-assigned this Apr 29, 2023
@memsharded memsharded added this to the 2.0.5 milestone Apr 29, 2023
@memsharded
Copy link
Member

Thanks very much @garyo for the detailed report.

Certainly, the behavior seems unexpected and wrong.
I am having a look, but IMHO this looks like a CMake bug. If CMake is able to correctly pick the architecture from the toolchain the first pass, there is no reason that calling it again, with exactly same arguments will fail the second one.

I think we should be reporting this to CMake, before trying to do a workaround in Conan, it might be possible that there could be other configuration involved like CMAKE_GENERATOR_TOOLSET which is also set in the toolchain. Those are very toolchain thing, and having them defined in the toolchain is necessary anyway, because Conan should work with CMake 3.15 without presets, so it is not that it can be moved from the toolchain to the presets, and having it duplicated also seems a bit wrong.

Would you like to report it to CMake yourself? (this can be easily reproduced without Conan). Otherwise we could probably do it. Thanks!

@garyo
Copy link
Author

garyo commented Apr 30, 2023

Issue reported to cmake at https://gitlab.kitware.com/cmake/cmake/-/issues/24864 -- hope that helps! I'll report back here if & when I hear from them.

@memsharded
Copy link
Member

Perfect, many thanks @garyo !

@memsharded memsharded removed this from the 2.0.5 milestone May 10, 2023
@rob-avalon
Copy link

I think I'm experiencing the same issue
cmake version 3.25.2
conan version 1.59.0

For now I'm able to work around it by using a CMakePresets.json in the root of my project which includes the conan CMakePresets.json and has a configurePreset which inherits default and adds the architecture : x64 setting

    "include": [
        "./build/generators/CMakePresets.json"
    ],
    "configurePresets": [
        {
            "name": "robs-config",
            "inherits": "default",
            "architecture" : "x64"
        },
    ],

@memsharded
Copy link
Member

Hi @rob-avalon

I'd suggest to upvote or comment on the CMake reported issue by @garyo, to drive a bit more attention over it, to see if they respond.

@datalogics-kam
Copy link

@garyo I also upvoted and commented on the Kitware issue, asking for an analysis.

Their README says to ask on their forums before reporting an issue. Any chance you could make a forum posting asking about it, referring to your test case? Perhaps it would draw the attention of the developers.

This is looking to be a big enough problem that we might have to alter the CMakePresets.json after a conan install in our tooling that surrounds Conan. That being the case, would it make sense to get a workaround into Conan anyway?

P.S. We're probably along way out from being able to adopt Conan 2 (we have to convert a huge SCons project to use CMake) so can any potential Conan fix also be in 1.x?

@memsharded
Copy link
Member

It sounds challenging. The thing is that Conan must support CMake 3.15, without presets at all. And there is nothing that really tell Conan about CMake version when conan install executes and it has to generate things. So it cannot be removed from conan_toolchain.cmake. Adding it too to the CMakePresets sounds at the very least dirty, but I guess it can be also problematic being repeated in the conan_toolchain.cmake.

@garyo
Copy link
Author

garyo commented May 16, 2023

Just as an idea: if conan could add an option for users to add arbitrary info to the generated CMake preset (either as a command-line option or any other way) then it wouldn't affect conan's compatibility with anything; we users would be responsible for adding the particular line when we know it's needed (already knowing the conan and cmake versions). Maybe just merge in a user-provided piece of json? Basically just to save us from having to postprocess the preset.

I'm not on their forum but I'll see what I can do.

@datalogics-kam
Copy link

datalogics-kam commented May 16, 2023

Never mind this....it's supposed to be a cache variable, see below
Hm...Removing CMAKE_GENERATOR_PLATFORM from the toolchain file does fix the problem.

Maybe it could be conditional on versions that don't have presets?

if(${CMAKE_VERSION} VERSION_LESS "3.19")
  set(CMAKE_GENERATOR_PLATFORM "x64" CACHE STRING "" FORCE)
endif()

Or...maybe it shouldn't be a cache variable? Changing it to this fixes the problem

set(CMAKE_GENERATOR_PLATFORM "x64")

Also, one example in the CMake doc shows a toolchain file setting platform and toolset variables as regular variables, not cache variables.

set(CMAKE_GENERATOR_TOOLSET CE800) # Can be omitted for 8.0
set(CMAKE_GENERATOR_PLATFORM SDK_AM335X_SK_WEC2013_V310)

@datalogics-kam
Copy link

Never mind the above. Brad King replied:

It is supposed to be a cache variable so that cmake . and cmake --build . know how the generator is configured by reading just CMakeCache.txt (and not the CMAKE_TOOLCHAIN_FILE). The cmake-toolchains docs may need to be updated.

@memsharded
Copy link
Member

#11623 (comment) reported by @manuelnp seems the same as this ticket.

@memsharded
Copy link
Member

Even if I still think this shouldn't be the expected behavior from CMake, it is clear that this is biting more users like latest @manuelnp, so I am checking the impact of adding it to the presets. Even if it seems that it can be done, I am still concerned that this might break in unexpected ways or in future CMake versions as the discussion in https://gitlab.kitware.com/cmake/cmake/-/issues/24864 shows.

@Simran-B
Copy link

Same problem here using Conan 2, and I can confirm that CMAKE_GENERATOR_TOOLSET is also affected as @memsharded suspected:

CMake Error: Error: generator toolset:
Does not match the toolset used previously: v142
Either remove the CMakeCache.txt file and CMakeFiles directory or choose a different binary directory.

Unfortunately, setting architecture and toolset on the CMakeToolchain object in the generate() method of a conanfile.py isn't supported in order to have these two settings added to the generated CMakePresets.json. I don't want to manually edit the preset, therefore, the only workaround that "survives" a conan install seems to be setting the CMake -A and -T command line options when configuring:

cmake .. --preset conan-default -A x64 -T v142

I'm not sure if Visual Studio's "Open Folder experience" is affected by the problem but if it is, then I guess you cannot easily set these CLI options. At least in VS 2019, you can't use "Open Folder" anyway because the built-in CMake version doesn't support toolchainFile and include in presets, so using an external CMake is required (and also circumvents some other issues).

@memsharded
Copy link
Member

Unfortunately, setting architecture and toolset on the CMakeToolchain object in the generate() method of a conanfile.py isn't supported in order to have these two settings added to the generated CMakePresets.json

This is what I wanted to try, and try to evaluate if it could be breaking, adding twice the information, both in conan_toolchain.cmake and in the Presets file. Even if ugly, if it doesn't break, we might try to go that route, if this keeps biting users (sorry that this fell during summer, I'll try to reactivate this, assigning for 2.0.12 as look-into)

@memsharded memsharded added this to the 2.0.12 milestone Sep 11, 2023
@PeteAudinate
Copy link

Thanks, that would be great to have a fix for this.

... if this keeps biting users ...

I think this will be biting anyone using CMake presets with Visual Studio, right? FWIW, we've been using the same workaround as @Simran-B.

@czoido czoido modified the milestones: 2.0.12, 2.0.13, 2.0.14 Sep 26, 2023
@czoido czoido modified the milestones: 2.0.14, 2.0.15 Nov 7, 2023
@garyo
Copy link
Author

garyo commented Dec 4, 2023

This is blocking our release of OpenFX -- any suggestions for ways to work around it?

@memsharded
Copy link
Member

This was assigned for checking in 2.0.15. Opening #15215 to see what breaks and start having a deeper look

@memsharded
Copy link
Member

As a quick question: I assume that the change in #15215 is the necessary one to solve this issue, isn't it? (at least to make it work with cmake-presets)

@memsharded
Copy link
Member

#15215 merged, this will be in next 2.0.15 release, feedback welcome!

@garyo
Copy link
Author

garyo commented Dec 29, 2023

Seems to work great! Thanks for doing this.

@memsharded
Copy link
Member

You are welcome @garyo thanks for the feedback!

@jdoubleu
Copy link

Looks like they've recently fixed this in CMake, as well: https://gitlab.kitware.com/cmake/cmake/-/commit/0654051dde88dfb8e374ac7e53bfc46c574b0169.

I'm not sure in which CMake version this will land, but it must be newer than 3.27.8.

@memsharded
Copy link
Member

Thanks @jdoubleu for sharing it.
A lot of Conan users might not be able to use 3.27, so probably the best is to keep the current fix in #15215, I think that I actually tried it with latest 3.28 and it was still working, so most likely we are good.

@gzfortech
Copy link

gzfortech commented Jan 22, 2024

I'm also using your fix, @memsharded, with conan2 plus the latest CMake 3.28.1, and it is working for me. Thanks! Better to let this in for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment