Conversation
…daigle/pwsh-via-dotnet-tool
There was a problem hiding this comment.
Pull request overview
This PR switches IntelliSense XML doc trimming to use pwsh via a .NET CLI tool (from dotnet-tools.json) and updates CI/OneBranch pipelines to restore tool manifests before builds that depend on them.
Changes:
- Add
powershell(pwsh) to the repo’sdotnet-tools.jsontool manifest. - Update doc-trimming MSBuild logic to invoke
pwshviadotnet tool run. - Add a reusable pipeline step to run
dotnet tool restore, and invoke it from key CI/OneBranch job templates.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/targets/TrimDocsForIntelliSense.targets | Switches trimming Exec to dotnet tool run pwsh. |
| tools/targets/CompareMdsRefAssemblies.targets | Removes _RestoreTools target (currently leaving dangling dependencies). |
| src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.csproj | Updates TrimDocs target to use dotnet tool run pwsh and adds a dependency (currently incorrect). |
| eng/pipelines/steps/tool-restore.yml | New pipeline step template to restore dotnet tools. |
| eng/pipelines/onebranch/steps/build-all-configurations-signed-dlls-step.yml | Invokes tool restore before building. |
| eng/pipelines/onebranch/jobs/build-signed-sqlclient-package-job.yml | Invokes tool restore before building. |
| eng/pipelines/onebranch/jobs/build-signed-csproj-package-job.yml | Invokes tool restore before building. |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Invokes tool restore after SDK install. |
| eng/pipelines/common/templates/jobs/ci-build-nugets-job.yml | Invokes tool restore after SDK install. |
| dotnet-tools.json | Adds the powershell tool entry exposing pwsh. |
Comments suppressed due to low confidence (1)
tools/targets/CompareMdsRefAssemblies.targets:118
- The
_RestoreToolstarget was removed, but_RunRefApiCompatstill depends on_RestoreToolslater in this file. As-is, invokingCompareMdsRefAssemblieswill fail with a missing target error. Either reintroduce_RestoreTools(to rundotnet tool restore) or update_RunRefApiCompat.DependsOnTargetsto reference the new tool-restore mechanism (and ensure it exists for non-pipeline/manual invocations too).
<!-- ================================================================== -->
<!-- _BuildLegacyRefNetFx -->
<!-- ================================================================== -->
<Target Name="_BuildLegacyRefNetFx" DependsOnTargets="_SetApiCompatProperties" Condition="'$(IsEnabledWindows)' == 'true'">
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4134 +/- ##
==========================================
- Coverage 73.22% 66.51% -6.71%
==========================================
Files 280 274 -6
Lines 43000 65782 +22782
==========================================
+ Hits 31486 43755 +12269
- Misses 11514 22027 +10513
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:
|
| <PowerShellCommand Condition="'$(OS)' != 'Windows_NT'">pwsh</PowerShellCommand> | ||
| <PowerShellCommand> | ||
| $(PowerShellCommand) | ||
| dotnet pwsh |
There was a problem hiding this comment.
dotnet pwsh depends on the PowerShell tool being runnable as a dotnet subcommand (i.e., resolved as dotnet-pwsh). If the powershell tool only installs a pwsh shim, this invocation won’t work even after restore. Using dotnet tool run pwsh -- ... is more resilient and still keeps the invocation bound to the restored local tool.
| dotnet pwsh | |
| dotnet tool run pwsh -- |
There was a problem hiding this comment.
I think this is fine since we're explicitly installing the dotnet pwsh tool.
| - **Visual Studio 2019** with imported components: [VS19Components](/tools/vsconfig/VS19Components.vsconfig) | ||
|
|
||
| - **Powershell**: To build SqlClient on Linux, powershell is needed as well. Follow the distro specific instructions at [Install Powershell on Linux](https://learn.microsoft.com/en-us/powershell/scripting/install/installing-powershell-on-linux?view=powershell-7.4) | ||
| - **Powershell**: PowerShell is included as a .NET local tool in this repository. Running `dotnet tool restore` (see below) will make it available. No separate installation is required. |
There was a problem hiding this comment.
Typo/casing: use “PowerShell” (official product name) instead of “Powershell” in this prerequisite bullet for consistency with the rest of the document.
| - **Powershell**: PowerShell is included as a .NET local tool in this repository. Running `dotnet tool restore` (see below) will make it available. No separate installation is required. | |
| - **PowerShell**: PowerShell is included as a .NET local tool in this repository. Running `dotnet tool restore` (see below) will make it available. No separate installation is required. |
| - template: /eng/pipelines/steps/install-dotnet.yml@self | ||
|
|
||
| # Restore dotnet CLI tools (e.g. pwsh, apicompat) before building. | ||
| - template: /eng/pipelines/steps/dotnet-tool-restore.yml@self |
There was a problem hiding this comment.
nit: Most of our other template files use verb-noun names, so maybe this file should be restore-dotnet-tools.yml. Not worth changing unless there are other things to fix.
| <PowerShellCommand Condition="'$(OS)' != 'Windows_NT'">pwsh</PowerShellCommand> | ||
| <PowerShellCommand> | ||
| $(PowerShellCommand) | ||
| dotnet pwsh |
There was a problem hiding this comment.
I think this is fine since we're explicitly installing the dotnet pwsh tool.
| <PowerShellCommand Condition="'$(OS)' != 'Windows_NT'">pwsh</PowerShellCommand> | ||
| <PowerShellCommand> | ||
| $(PowerShellCommand) | ||
| dotnet pwsh |
There was a problem hiding this comment.
For dotnet commands, our entry point to them has been build(2).proj, and in those files, we have a way to override the path to dotnet. With this change, the path to dotnet can't be overridden. Whether that's acceptable or not is up for discussion, but we already have pipeline mechanisms that use the dotnet override for x86 behavior.
There was a problem hiding this comment.
I see what you mean, but this step isn't architecture sensitive. Whatever dotnet version is on the path works just fine.
This pull request improves the consistency and reliability of using PowerShell scripts and .NET CLI tools across the build and test pipelines. It standardizes the way the
pwshcommand is invoked by leveraging a restored .NET tool, and ensures all required CLI tools are restored before builds or tests run. It also simplifies the MSBuild targets by removing redundant tool restore steps.Passing non-official build: https://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=145950&view=results