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

Convert CMakeSettings.json to CMakePresets.json for coreclr #89513

Merged
merged 9 commits into from
Aug 7, 2023

Conversation

huoyaoyuan
Copy link
Member

Part of #88688.

Basically, add all variables from cmake_cmd_line.txt.

The presets file is currently only used by IDE, but conversion of command line build for CLR should be straightforward. Currently only supports Windows because I didn't test other platforms.
CMakePresets supports file importing and we can separated the file when fully depending on it.

Corehost is a bit complicated and I'll try to include it later.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 26, 2023
@ghost
Copy link

ghost commented Jul 26, 2023

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Part of #88688.

Basically, add all variables from cmake_cmd_line.txt.

The presets file is currently only used by IDE, but conversion of command line build for CLR should be straightforward. Currently only supports Windows because I didn't test other platforms.
CMakePresets supports file importing and we can separated the file when fully depending on it.

Corehost is a bit complicated and I'll try to include it later.

Author: huoyaoyuan
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contributes to #88688

LGTM!

Copy link
Member Author

@huoyaoyuan huoyaoyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes for future potentials.

"generator": "Ninja",
"binaryDir": "${sourceDir}/../../artifacts/obj/coreclr/${presetName}",
"cacheVariables": {
"CMAKE_INSTALL_PREFIX": "${sourceDir}/../../artifacts/bin/coreclr/${presetName}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The installDir property is available since version 3.

Comment on lines +22 to +23
"CLR_CMAKE_TARGET_OS": "windows",
"CLI_CMAKE_FALLBACK_OS": "win10"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The varaible CMAKE_SYSTEM_VERSION is not set here. It seems to match exact Windows SDK version.

"name": "x86",
"hidden": true,
"architecture": {
"value": "x86",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSBuild uses Win32 while Ninja uses x86 here. May be difficult to implement a -msbuild switch in the future.

{
"name": "base",
"hidden": true,
"generator": "Ninja",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default since version 3.

@huoyaoyuan
Copy link
Member Author

Test failure looks unrelated. The files aren't affecting CLI build.
After getting merged, utilizing CMakeUserPresets.json for experiments would be easier.

@jkoritzinsky jkoritzinsky merged commit c37f6c3 into dotnet:main Aug 7, 2023
@huoyaoyuan huoyaoyuan deleted the cmake-preset branch August 8, 2023 02:27
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants