Conversation
…daigle/pwsh-via-dotnet-tool
There was a problem hiding this comment.
Pull request overview
This PR updates the repo’s tooling and CI configuration to standardize on restoring and running local dotnet tools (notably pwsh) and adjusts a few test/build artifacts accordingly.
Changes:
- Add
powershell(pwsh) as a localdotnettool and update multiple build targets to invokepwshviadotnet tool run. - Introduce a reusable pipeline step template to run
dotnet tool restore, and wire it into CI/OneBranch plus the GitHub CodeQL workflow. - Quarantine a known-flaky manual test and remove
Microsoft.DotNet.XUnitExtensionsfrom the UnitTests project references.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/targets/TrimDocsForIntelliSense.targets | Switch XML doc trimming to run via dotnet tool run pwsh. |
| tools/targets/CompareMdsRefAssemblies.targets | Remove the internal tool-restore target dependency for ApiCompat comparisons. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj | Remove Microsoft.DotNet.XUnitExtensions package reference (currently breaks compilation where its attributes are used). |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/AdapterTest/AdapterTest.cs | Mark one test as flaky with an issue link. |
| src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.csproj | Switch TrimDocs invocation to dotnet tool run pwsh. |
| eng/pipelines/steps/tool-restore.yml | Add a reusable dotnet tool restore step template. |
| eng/pipelines/onebranch/jobs/build-signed-mds-package-job.yml | Invoke tool restore before building. |
| eng/pipelines/onebranch/jobs/build-signed-csproj-package-job.yml | Invoke tool restore before building. |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Invoke tool restore before building/tests. |
| eng/pipelines/common/templates/jobs/ci-build-nugets-job.yml | Invoke tool restore before building. |
| dotnet-tools.json | Add powershell tool (pwsh) to the local tool manifest. |
| .github/workflows/codeql.yml | Restore dotnet tools before CodeQL manual build. |
| .github/plans/plan-migrateToMtp.prompt.md | Add an internal planning prompt doc for MTP migration. |
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj:59
- UnitTests sources use Xunit extensions such as PlatformSpecific/SkipOnPlatform/TestPlatforms (e.g., SimulatedServerTests/ConnectionTests.cs, AlwaysEncrypted/NativeColumnEncryptionKeyBaseline.cs). Removing the Microsoft.DotNet.XUnitExtensions PackageReference here will cause compile-time missing type errors. Re-add the package reference (preferred), or remove/replace those attributes across UnitTests so the project no longer depends on that package.
<!-- References for netframework -->
<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
<Reference Include="System.Transactions" />
<PackageReference Include="Microsoft.Data.SqlClient.SNI" />
<PackageReference Include="Microsoft.NET.Test.Sdk" />
<PackageReference Include="Microsoft.SqlServer.Server" />
<PackageReference Include="System.Configuration.ConfigurationManager" />
<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" />
<PackageReference Include="xunit" />
<PackageReference Include="xunit.runner.console"
| @@ -3,7 +3,6 @@ The .NET Foundation licenses this file to you under the MIT license. --> | |||
| <Project> | |||
| <!-- This target runs after Build, and trims XML documentation generated in the $(OutputPath) of the project where this target is included.--> | |||
| <Target Name="TrimDocsForIntelliSense" AfterTargets="Build" Condition="'$(GenerateDocumentationFile)' == 'true'"> | |||
There was a problem hiding this comment.
This target now relies on dotnet tool run pwsh, which will fail if local tools haven’t been restored yet (common on first build after clone). Consider either (1) restoring tools as part of the build flow before this target runs, or (2) falling back to system-installed pwsh/powershell.exe when available and emit a clear error instructing dotnet tool restore otherwise.
| <Target Name="TrimDocsForIntelliSense" AfterTargets="Build" Condition="'$(GenerateDocumentationFile)' == 'true'"> | |
| <Target Name="TrimDocsForIntelliSense" AfterTargets="Build" Condition="'$(GenerateDocumentationFile)' == 'true'"> | |
| <Exec Command="dotnet tool restore" /> |
| <PropertyGroup> | ||
| <PowerShellCommand Condition="'$(OS)' == 'Windows_NT'">powershell.exe</PowerShellCommand> | ||
| <PowerShellCommand Condition="'$(OS)' != 'Windows_NT'">pwsh</PowerShellCommand> | ||
| <PowerShellCommand> | ||
| $(PowerShellCommand) | ||
| dotnet tool run pwsh | ||
| -NonInteractive | ||
| -ExecutionPolicy Unrestricted | ||
| -Command "$(RepoRoot)tools\intellisense\TrimDocs.ps1 -inputFile '$(DocumentationFile)' -outputFile '$(DocumentationFile)'" |
There was a problem hiding this comment.
The ref build’s TrimDocs step now shells out via dotnet tool run pwsh. If the local tool manifest hasn’t been restored, building the ref project will fail late in the build (after compilation) with a missing-tool error. Consider adding a precondition/error message (or performing tool restore earlier in the repo build) to make this failure mode clearer and avoid surprising local builds.
| <!-- _RunRefApiCompat --> | ||
| <!-- ================================================================== --> | ||
| <Target Name="_RunRefApiCompat" | ||
| DependsOnTargets="_DownloadBaselinePackage;_RestoreTools;_BuildLegacyRefNetFx;_BuildLegacyRefNetCore;_BuildNewRefProject"> | ||
| DependsOnTargets="_DownloadBaselinePackage;_BuildLegacyRefNetFx;_BuildLegacyRefNetCore;_BuildNewRefProject"> | ||
|
|
There was a problem hiding this comment.
Removing the _RestoreTools dependency means CompareMdsRefAssemblies now assumes the local tool manifest has already been restored (for apicompat). This is a behavior change that can break the documented one-liner usage unless callers remember to run dotnet tool restore first. Consider keeping a tool-restore dependency (optionally gated behind a property) or adding a clear error/message when the tool isn’t available.
| # Restores dotnet CLI tools defined in dotnet-tools.json. | ||
| # This step should be invoked after install-dotnet.yml and before any build | ||
| # steps that depend on the restored tools (e.g. pwsh, apicompat). | ||
|
|
||
| steps: | ||
| - script: dotnet tool restore | ||
| displayName: Restore .NET Tools | ||
| workingDirectory: $(Build.SourcesDirectory) |
There was a problem hiding this comment.
The PR description is still the default template and doesn’t describe the intent of these changes (tooling/pipeline updates, test package changes, flaky test quarantine). Please update the PR description with a short summary and testing notes so reviewers can validate scope and impact.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4136 +/- ##
==========================================
- Coverage 73.22% 66.39% -6.83%
==========================================
Files 280 274 -6
Lines 43000 65782 +22782
==========================================
+ Hits 31486 43678 +12192
- Misses 11514 22104 +10590
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Provide a summary of the changes being introduced. Important topics to cover
include:
High quality descriptions will lead to a smoother review experience.
Issues
Link to any relevant issues, bugs, or discussions (e.g.,
Closes #123,Fixes issue #456).Testing
Describe the automated tests (unit, integration) you created or modified.
Provide justification for any gap in automated testing. List any manual testing
steps that were performed to ensure the changes work.
Guidelines
Please review the contribution guidelines before submitting a pull request: