-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix MSBuild integration for the host build with CMake 3.26 #88208
Conversation
# in case CMake has decided to use the SDK support. | ||
# We're dogfooding things, so we need to set settings in ways that the product doesn't quite support. | ||
set_target_properties(${targetName} PROPERTIES | ||
DOTNET_TARGET_FRAMEWORK net8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass this value from the invocation script to avoid manual updates in the future? If we could capture this value
Line 6 in 5a24757
<MajorVersion>8</MajorVersion> |
-DCLR_DOTNET_TFM=net%DotnetMajorVersion%.0
, that will do the trick.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually need an installed/available version here since we are disabling implicit framework references. We just need a valid value. I'll add a comment.
@@ -31,6 +31,19 @@ if (CLR_CMAKE_HOST_WIN32) | |||
set_target_properties(${targetName} PROPERTIES COMPILE_OPTIONS "${compileOptions}") | |||
endfunction() | |||
|
|||
function(add_ijw_msbuild_project_properties targetName ijwhost_target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is ijwhost_target needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted a customization layer here as the name ijwhost isn't required to be the same (and the name for the fake implementation in src/tests has changed at times)
/backport to release/7.0 |
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/5537476312 |
@hoyosjs backporting to release/7.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix MSBuild integration for the host build with CMake 3.26
Using index info to reconstruct a base tree...
M eng/native/ijw/IJW.cmake
M src/native/corehost/test/ijw/CMakeLists.txt
M src/tests/Interop/IJW/CopyConstructorMarshaler/CMakeLists.txt
M src/tests/Interop/IJW/IjwNativeDll/CMakeLists.txt
M src/tests/Interop/IJW/NativeVarargs/CMakeLists.txt
Falling back to patching base and 3-way merge...
Auto-merging src/tests/Interop/IJW/NativeVarargs/CMakeLists.txt
CONFLICT (content): Merge conflict in src/tests/Interop/IJW/NativeVarargs/CMakeLists.txt
Auto-merging src/tests/Interop/IJW/IjwNativeDll/CMakeLists.txt
CONFLICT (content): Merge conflict in src/tests/Interop/IJW/IjwNativeDll/CMakeLists.txt
Auto-merging src/tests/Interop/IJW/CopyConstructorMarshaler/CMakeLists.txt
CONFLICT (content): Merge conflict in src/tests/Interop/IJW/CopyConstructorMarshaler/CMakeLists.txt
Auto-merging src/native/corehost/test/ijw/CMakeLists.txt
Auto-merging eng/native/ijw/IJW.cmake
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix MSBuild integration for the host build with CMake 3.26
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@hoyosjs an error occurred while backporting to release/7.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
I'm still hitting this after syncing my local clone to the latest bits in |
Nevermind. I had to build host. Jeremy helped me confirm, it works. |
Fixes #88172