Add target-specific CMake project references - #17514
Open
jkoritzinsky wants to merge 3 commits into
Open
jkoritzinsky wants to merge 3 commits into
jkoritzinsky wants to merge 3 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are build-blocking issues (unused using with warnings-as-errors, and invalid MSBuild metadata method usage in ProjectReference.targets) that must be corrected before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/Microsoft.DotNet.CMake.Sdk/sdk/ProjectReference.targets — The %(CMakeProjectReference.CMakeTargets.Replace(...)) syntax is not a valid MSBuild metadata… |
|
src/Microsoft.DotNet.CMake.Sdk/src/ExecWithMutex.cs — System.IO is not used in this file, and the repo builds with TreatWarningsAsErrors=true, so… |
 |
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs — When CMakeTargets is specified, this task doesn’t use config.Directories, but it still fails if… |
What changed in this PR
This PR extends the CMake SDK’s project reference model so consumers can build/copy artifacts from specific CMake targets (via the CMake File API) while preventing concurrent native configure/build operations on the same build tree using a cross-process, user-scoped mutex.
Changes:
- Add
CMakeProjectReferencewithCMakeTargetsmetadata and integrate it into restore/build/copy flows. - Update File API artifact discovery to support selecting artifacts by CMake target name(s) in addition to source-directory matching.
- Introduce
ExecWithMutexand use it to serialize CMakeConfigure/CoreCompileexecution per build output directory.
| File | Description |
|---|---|
| src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs | Adds target-based artifact discovery path and updates logging/messages accordingly. |
| src/Microsoft.DotNet.CMake.Sdk/src/ExecWithMutex.cs | New task to run Exec under a named cross-process mutex (CurrentUserOnly on non-NETFRAMEWORK). |
| src/Microsoft.DotNet.CMake.Sdk/sdk/ProjectReference.targets | Adds CMakeProjectReference item type and wires restore/build/copy behavior for target-based references. |
| src/Microsoft.DotNet.CMake.Sdk/README.md | Documents the new CMakeProjectReference usage and behavior. |
| src/Microsoft.DotNet.CMake.Sdk/Microsoft.DotNet.CMake.Sdk.csproj | Adds dependency needed for deriving from Exec (Microsoft.Build.Tasks.Core). |
| src/Microsoft.DotNet.CMake.Sdk/build/Microsoft.DotNet.CMake.Sdk.targets | Uses ExecWithMutex to serialize configure/build execution per $(CMakeOutputDir). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
MSBuild property passing currently breaks multi-target CMakeTargets (semicolon parsing) and target selection can silently succeed on typos, risking missing native outputs without failing the build.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
src/Microsoft.DotNet.CMake.Sdk/sdk/ProjectReference.targets — CMakeTargets is documented/treated as semicolon-separated, but it’s being embedded directly into… |
|
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs — When CMakeTargets is specified, the task silently succeeds even if one or more requested targets do… |
Issues resolved since last review (3)
| Severity | Finding |
|---|---|
|
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs — When CMakeTargets is specified, this task doesn’t use config.Directories, but it still fails if… View resolved comment |
|
src/Microsoft.DotNet.CMake.Sdk/src/ExecWithMutex.cs — System.IO is not used in this file, and the repo builds with TreatWarningsAsErrors=true, so… View resolved comment |
|
src/Microsoft.DotNet.CMake.Sdk/sdk/ProjectReference.targets — The %(CMakeProjectReference.CMakeTargets.Replace(...)) syntax is not a valid MSBuild metadata… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs:158
- The comment above this block notes that CMake’s File API “routinely emits” "."/".." segments, but dirSource is only canonicalized when it is not rooted. If CMake returns an absolute directory path containing "."/"..", the comparison against normalizedSourceDir can fail unexpectedly. Canonicalize dirSource for both rooted and non-rooted cases.
src/Microsoft.DotNet.CMake.Sdk/sdk/ProjectReference.targets:110
- ReferencedCMakeTargets is passed through the MSBuild task Properties list without escaping. If CMakeTargets contains multiple targets separated by ';', only the first target will be assigned to ReferencedCMakeTargets and the remaining segments will be parsed as additional (invalid) properties, breaking artifact selection/copy.
<MSBuild Projects="$(MSBuildProjectFile)"
Targets="CopyNativeProjectBinariesFromFileApi"
Properties="ReferencedCMakeProject=%(CMakeProjectReferenceNormalized.Identity);
ReferencedCMakeTargets=%(CMakeProjectReferenceNormalized.CMakeTargets);
%(CMakeProjectReferenceNormalized.AdditionalProperties)"
Comment on lines
+135
to
+138
| var requestedTargets = CMakeTargets.Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); | ||
| targets = config.Targets.Where(t => requestedTargets.Contains(t.Name, StringComparer.OrdinalIgnoreCase)); | ||
| Log.LogMessage(MessageImportance.Low, "Found {0} requested CMake target(s) for configuration '{1}'.", targets.Count(), Configuration); | ||
| } |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The MSBuild invocation that builds CMake project references can batch incorrectly (cross-product) and may build the wrong projects with the wrong CMakeBuildTarget values.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/Microsoft.DotNet.CMake.Sdk/sdk/ProjectReference.targets — The MSBuild invocation for CMake project references is batching over… |
Pre-existing issues (1)
| Severity | Finding |
|---|---|
|
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs — When CMakeTargets is specified, the task silently succeeds even if one or more requested targets do… View comment |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Microsoft.DotNet.CMake.Sdk/sdk/ProjectReference.targets — CMakeTargets is documented/treated as semicolon-separated, but it’s being embedded directly into… View resolved comment |
Comment on lines
+90
to
+95
| <MSBuild Projects="@(_UniqueNativeProjectsToBuild)" | ||
| Properties="%(AdditionalProperties)" | ||
| BuildInParallel="false" /> | ||
| <MSBuild Projects="@(_CMakeProjectsToBuild)" | ||
| Properties="CMakeBuildTarget=$([MSBuild]::Escape('%(CMakeProjectReferenceNormalized.CMakeTargets)'));%(AdditionalProperties)" | ||
| BuildInParallel="false" /> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CMake SDK consumers need to build and consume selected targets from a shared CMake project, while native build tools must not operate on the same build tree concurrently.
This change adds
CMakeProjectReferencewithCMakeTargets, selects artifacts directly from the CMake File API target descriptions, and serializes configure/build execution with a user-scoped cross-process mutex. ExistingNativeProjectReferencedirectory-based artifact handling remains supported.Validation:
To double check: