-
Couldn't load subscription status.
- Fork 379
Add CMake File API support to Microsoft.DotNet.CMake.Sdk #16233
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: jkoritzinsky <[email protected]>
Co-authored-by: jkoritzinsky <[email protected]>
Co-authored-by: jkoritzinsky <[email protected]>
Co-authored-by: jkoritzinsky <[email protected]>
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.
Please address
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs
Outdated
Show resolved
Hide resolved
|
@copilot please address the above comments |
- Use client stateless query file with client name "Microsoft.DotNet.CMake.Sdk" - Remove old fallback logic - File API is now required - Define C# object models for File API JSON structures - Add error logging when configuration not found - Add error logging when source directory not found - Add warning logging when no artifacts found - Condition System.Text.Json package reference to .NET Framework TFM only Co-authored-by: jkoritzinsky <[email protected]>
All review feedback has been addressed in commit d5a1e0c:
The implementation has been tested and verified to work correctly. |
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.
@copilot please address this feedback
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.
@copilot please address this feedback
src/Microsoft.DotNet.CMake.Sdk/src/GetCMakeArtifactsFromFileApi.cs
Outdated
Show resolved
Hide resolved
The CopyNativeProjectBinaries target is no longer used after switching to File API-based artifact detection. Removed the entire target and its legacy filtering logic. Co-authored-by: jkoritzinsky <[email protected]>
- Updated CreateCMakeFileApiQuery and GetCMakeArtifactsFromFileApi to derive from BuildTask instead of Task - Replaced loop-based configuration and directory finding with LINQ FirstOrDefault - Updated Log method calls to use BuildTask's Log API (LogImportance enum and params object[] format) - Added using Microsoft.Build.Utilities for TaskItem Co-authored-by: jkoritzinsky <[email protected]>
All feedback addressed in commit {short_hash}:
|
- Sealed all classes in CMakeFileApiModels.cs as they don't need to be inherited - Added comprehensive documentation comment explaining these types represent a subset of the CMake File API - Included links to the official CMake File API specification (index file and codemodel v2) Co-authored-by: jkoritzinsky <[email protected]>
|
This is blocked on getting a newer CMake that has a new enough file API implementation rolled out on the Windows machines so we don't break our WinForms users. Validated locally that this works for the WinForms use case. |
| <Import Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" /> | ||
|
|
||
| <PropertyGroup> | ||
| <VSWhereVersion Condition="'$(VSWhereVersion)' == ''">3.1.7</VSWhereVersion> |
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.
do we really need this? VSWhereVersion should always be defined by arcade SDK's DefaultVersions.props
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.
@copilot remove this VSWhereVersion property.
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.
Done in commit {short_hash}. Removed the redundant VSWhereVersion property since it's already defined by Arcade SDK's DefaultVersions.props.
|
Blocked on #16233. |
VSWhereVersion is already defined by arcade SDK's DefaultVersions.props, so the default value here is redundant and can be removed. Co-authored-by: jkoritzinsky <[email protected]>
This links to this PR 😄 is it still blocked or good to go? |
|
I messed up the copy, dang it. It's blocked on an update to the helix machines (need a newer CMake for WinForms) which isn't in yet. |
Implementation Plan: CMake File API Integration
Summary
Successfully implemented CMake File API integration for the Microsoft.DotNet.CMake.Sdk package and addressed all PR review feedback.
Changes Made:
New C# Tasks: Created two MSBuild tasks:
CreateCMakeFileApiQuery: Generates File API client stateless query before CMake configureGetCMakeArtifactsFromFileApi: Parses File API response to locate build artifactsC# Object Models: Created strongly-typed models in
CMakeFileApiModels.csfor JSON deserializationUpdated CMake SDK: Modified Microsoft.DotNet.CMake.Sdk.csproj to build C# tasks alongside SDK files
Updated Targets:
CreateFileApiQuerytarget that runs beforeConfigureProjectReference.targetsto use File API exclusively (removed fallback logic)CopyNativeProjectBinariestargetCode Quality:
Benefits:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.