[dotnet] [build] Use paket as package manager in sdk projects#17577
Open
nvborisenko wants to merge 6 commits into
Open
[dotnet] [build] Use paket as package manager in sdk projects#17577nvborisenko wants to merge 6 commits into
nvborisenko wants to merge 6 commits into
Conversation
selenium-ci
added
C-dotnet
Contributor
Review Summary by QodoMigrate .NET SDK projects to Paket package manager
WalkthroughsDescription• Migrate .NET SDK projects to use Paket package manager • Remove PackageReference entries from .csproj files • Add paket.references files for dependency declarations • Add Paket.Restore.targets imports to all projects • Support .NET 10.0 framework and suppress package pruning warnings Diagramflowchart LR
A["PackageReference in .csproj"] -->|"Remove"| B["paket.references files"]
C["paket.dependencies"] -->|"Add net10.0 support"| D["Updated framework list"]
B -->|"Import"| E["Paket.Restore.targets"]
F["Directory.Build.props"] -->|"Suppress NU1510"| G["Clean .NET 10+ builds"]
File Changes1. dotnet/Directory.Build.props
|
Contributor
Code Review by Qodo
1. Missing newline in dotnet-tools.json
|
Member
|
Thank you, @nvborisenko for this code suggestion. The support packages contain example code that many users find helpful, but they do not necessarily represent After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
|
Code review by qodo was updated up to the latest commit 781c035 |
Comment on lines
+74
to
+78
| <!-- Second test: Call 'dotnet paket' and see if it returns without an error. Mute all the output. Only run if previous test failed and the test has not been disabled. --> | ||
| <!-- WARNING: This method can lead to processes hanging forever, and should be used as little as possible. See https://github.com/fsprojects/Paket/issues/3705 for details. --> | ||
| <Exec Condition=" '$(PaketExePath)' == '' AND !$(PaketDisableCliTest) AND !$(_ConfigContainsPaket)" Command="dotnet paket --version" IgnoreExitCode="true" StandardOutputImportance="low" StandardErrorImportance="low" > | ||
| <Output TaskParameter="ExitCode" PropertyName="LocalPaketToolExitCode" /> | ||
| </Exec> |
Contributor
There was a problem hiding this comment.
1. dotnet paket probe can hang 📘 Rule violation ☼ Reliability
The build target runs dotnet paket --version as an availability probe while explicitly warning it “can lead to processes hanging forever”, and it ignores the exit code. This can cause non-deterministic CI/build hangs instead of failing fast with actionable diagnostics.
Agent Prompt
## Issue description
`SetPaketCommand` probes for Paket by executing `dotnet paket --version` with `IgnoreExitCode="true"` and no timeout, despite an in-file warning that the command can hang forever.
## Issue Context
This targets file runs during MSBuild evaluation/restore and impacts developer builds and CI reliability. Compliance requires validating external command outcomes and failing safely with deterministic behavior.
## Fix Focus Areas
- dotnet/.paket/Paket.Restore.targets[66-83]
## Suggested remediation
- Avoid executing `dotnet paket --version` as a probe where possible (prefer deterministic checks such as `dotnet tool list --local` when `.config/dotnet-tools.json` exists).
- If an Exec probe remains necessary, add an explicit timeout (MSBuild `Exec` supports `Timeout` in newer MSBuilds; otherwise wrap the command in a platform-appropriate timeout helper) and surface a clear error when it times out.
- Don’t silently ignore failures: either remove `IgnoreExitCode="true"` or gate subsequent decisions on validated output/exit code and emit an actionable message when Paket cannot be located.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+261
to
+267
| <PropertyGroup> | ||
| <PaketCliToolFilePath>$(PaketIntermediateOutputPath)/$(MSBuildProjectFile).paket.clitools</PaketCliToolFilePath> | ||
| </PropertyGroup> | ||
|
|
||
| <ReadLinesFromFile File="$(PaketCliToolFilePath)" > | ||
| <Output TaskParameter="Lines" ItemName="PaketCliToolFileLines"/> | ||
| </ReadLinesFromFile> |
Contributor
There was a problem hiding this comment.
2. Missing exists() before readlines 📘 Rule violation ☼ Reliability
ReadLinesFromFile unconditionally reads $(PaketCliToolFilePath) (i.e., $(PaketIntermediateOutputPath)/$(MSBuildProjectFile).paket.clitools) without checking that the file exists, which can hard-fail builds when Paket doesn’t generate it or restore was skipped. This violates the requirement to validate required artifacts/preconditions and fail safely with clear diagnostics.
Agent Prompt
## Issue description
The targets file reads `$(PaketCliToolFilePath)` (the generated `.paket.clitools` file under `$(PaketIntermediateOutputPath)`) unconditionally via `ReadLinesFromFile`, which will fail with a file-not-found error when the file is not generated for a project or restore was skipped.
## Issue Context
Compliance requires validating preconditions/artifacts in automation and failing safely with clear diagnostics. In the same area of the targets file, the `.paket.resolved` read above is conditionally executed, but the `.paket.clitools` read is not, making builds sensitive to whether that file exists.
## Fix Focus Areas
- dotnet/.paket/Paket.Restore.targets[261-278]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+162
to
+171
| <PropertyGroup Condition="'$(PaketPropsVersion)' != '6.0.0' "> | ||
| <PaketRestoreRequired>true</PaketRestoreRequired> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Do a global restore if required --> | ||
| <Warning Text="This version of MSBuild (we assume '$(DetectedMSBuildVersion)' or older) doesn't support GetFileHash, so paket fast restore is disabled." Condition=" '$(MSBuildSupportsHashing)' != 'true' " /> | ||
| <Error Text="Stop build because of PAKET_ERROR_ON_MSBUILD_EXEC and we always call the bootstrapper" Condition=" '$(PAKET_ERROR_ON_MSBUILD_EXEC)' == 'true' AND '$(PaketBootstrapperStyle)' == 'classic' AND Exists('$(PaketBootStrapperExePath)') AND !(Exists('$(PaketExePath)'))" /> | ||
| <Exec Command='$(PaketBootStrapperCommand)' Condition=" '$(PaketBootstrapperStyle)' == 'classic' AND Exists('$(PaketBootStrapperExePath)') AND !(Exists('$(PaketExePath)'))" ContinueOnError="false" /> | ||
| <Error Text="Stop build because of PAKET_ERROR_ON_MSBUILD_EXEC and we need a full restore (hashes don't match)" Condition=" '$(PAKET_ERROR_ON_MSBUILD_EXEC)' == 'true' AND '$(PaketRestoreRequired)' == 'true' AND '$(PaketDisableGlobalRestore)' != 'true'" /> | ||
| <Exec Command='$(PaketCommand) restore' Condition=" '$(PaketRestoreRequired)' == 'true' AND '$(PaketDisableGlobalRestore)' != 'true' " ContinueOnError="false" /> |
Contributor
There was a problem hiding this comment.
3. Restore runs every build 🐞 Bug ➹ Performance
PaketRestoreRequired is forced to true whenever $(PaketPropsVersion) != '6.0.0'; since PaketPropsVersion is not set by the imported projects, this evaluates true and triggers `paket restore` on every build, defeating the lock/cache fast-path and making builds slower and more network/flakiness-prone.
Agent Prompt
### Issue description
`PaketRestoreRequired` is unconditionally flipped back to `true` via the `PaketPropsVersion` check, which (when `PaketPropsVersion` is unset) forces a restore every build.
### Issue Context
Projects in this repo import `Paket.Restore.targets` directly and do not import any generated Paket props that would define `PaketPropsVersion`, so `$(PaketPropsVersion)` is typically empty.
### Fix
Update the condition so it only forces restore when `PaketPropsVersion` is actually present and mismatched (or set a sane default in a single place if that’s the intended contract).
### Fix Focus Areas
- dotnet/.paket/Paket.Restore.targets[158-171]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+174
to
+213
| <ItemGroup> | ||
| <MyTargetFrameworks Condition="'$(TargetFramework)' != '' " Include="$(TargetFramework)"></MyTargetFrameworks> | ||
| <!-- Don't include all frameworks when msbuild explicitly asks for a single one --> | ||
| <MyTargetFrameworks Condition="'$(TargetFrameworks)' != '' AND '$(TargetFramework)' == '' " Include="$(TargetFrameworks)"></MyTargetFrameworks> | ||
| <PaketResolvedFilePaths Include="@(MyTargetFrameworks -> '$(PaketIntermediateOutputPath)\$(MSBuildProjectFile).%(Identity).paket.resolved')"></PaketResolvedFilePaths> | ||
| </ItemGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <PaketReferencesCachedFilePath>$(PaketIntermediateOutputPath)\$(MSBuildProjectFile).paket.references.cached</PaketReferencesCachedFilePath> | ||
| <!-- MyProject.fsproj.paket.references has the highest precedence --> | ||
| <PaketOriginalReferencesFilePath>$(MSBuildProjectFullPath).paket.references</PaketOriginalReferencesFilePath> | ||
| <!-- MyProject.paket.references --> | ||
| <PaketOriginalReferencesFilePath Condition=" !Exists('$(PaketOriginalReferencesFilePath)')">$(MSBuildProjectDirectory)\$(MSBuildProjectName).paket.references</PaketOriginalReferencesFilePath> | ||
| <!-- paket.references --> | ||
| <PaketOriginalReferencesFilePath Condition=" !Exists('$(PaketOriginalReferencesFilePath)')">$(MSBuildProjectDirectory)\paket.references</PaketOriginalReferencesFilePath> | ||
|
|
||
| <DoAllResolvedFilesExist>false</DoAllResolvedFilesExist> | ||
| <DoAllResolvedFilesExist Condition="Exists(%(PaketResolvedFilePaths.Identity))">true</DoAllResolvedFilesExist> | ||
| <PaketRestoreRequired>true</PaketRestoreRequired> | ||
| <PaketRestoreRequiredReason>references-file-or-cache-not-found</PaketRestoreRequiredReason> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Step 2 a Detect changes in references file --> | ||
| <PropertyGroup Condition="Exists('$(PaketOriginalReferencesFilePath)') AND Exists('$(PaketReferencesCachedFilePath)') "> | ||
| <PaketRestoreCachedHash>$([System.IO.File]::ReadAllText('$(PaketReferencesCachedFilePath)'))</PaketRestoreCachedHash> | ||
| <PaketRestoreReferencesFileHash>$([System.IO.File]::ReadAllText('$(PaketOriginalReferencesFilePath)'))</PaketRestoreReferencesFileHash> | ||
| <PaketRestoreRequiredReason>references-file</PaketRestoreRequiredReason> | ||
| <PaketRestoreRequired Condition=" '$(PaketRestoreReferencesFileHash)' == '$(PaketRestoreCachedHash)' ">false</PaketRestoreRequired> | ||
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup Condition="!Exists('$(PaketOriginalReferencesFilePath)') AND !Exists('$(PaketReferencesCachedFilePath)') "> | ||
| <!-- If both don't exist there is nothing to do. --> | ||
| <PaketRestoreRequired>false</PaketRestoreRequired> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Step 2 b detect relevant changes in project file (new targetframework) --> | ||
| <PropertyGroup Condition=" '$(DoAllResolvedFilesExist)' != 'true' "> | ||
| <PaketRestoreRequired>true</PaketRestoreRequired> | ||
| <PaketRestoreRequiredReason>target-framework '$(TargetFramework)' or '$(TargetFrameworks)' files @(PaketResolvedFilePaths)</PaketRestoreRequiredReason> | ||
| </PropertyGroup> |
Contributor
There was a problem hiding this comment.
4. Multi-target resolved check flawed 🐞 Bug ≡ Correctness
DoAllResolvedFilesExist is set to true if any *.paket.resolved file exists, not if all of them exist; for multi-target builds this can incorrectly skip restore when one framework’s resolved file is missing, leading to missing/incorrect PackageReference forwarding or subsequent file-read failures.
Agent Prompt
### Issue description
`DoAllResolvedFilesExist` is computed using a scalar property assignment batched over an item list, which makes it effectively mean “any exists” rather than “all exist”. This breaks restore decisioning for multi-target projects.
### Issue Context
`PaketResolvedFilePaths` is built from `@(MyTargetFrameworks)`, so it commonly contains multiple items. The restore decision (and later error checking) relies on `DoAllResolvedFilesExist`.
### Fix
Replace the property batching with an item-based missing-file check, e.g.:
- Create `MissingResolvedFiles` item group with `Condition="!Exists('%(Identity)')"`
- Set `DoAllResolvedFilesExist` based on whether `@(MissingResolvedFiles)` is empty
- Reuse the same logic in both the pre-restore and post-restore checks.
### Fix Focus Areas
- dotnet/.paket/Paket.Restore.targets[173-227]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit ff29936 |
Comment on lines
+68
to
+83
| <PropertyGroup Condition=" '$(PaketExePath)' == '' "> | ||
| <_DotnetToolsJson Condition="Exists('$(PaketRootPath)/.config/dotnet-tools.json')">$([System.IO.File]::ReadAllText("$(PaketRootPath)/.config/dotnet-tools.json"))</_DotnetToolsJson> | ||
| <_ConfigContainsPaket Condition=" '$(_DotnetToolsJson)' != ''">$(_DotnetToolsJson.Contains('"paket"'))</_ConfigContainsPaket> | ||
| <_ConfigContainsPaket Condition=" '$(_ConfigContainsPaket)' == ''">false</_ConfigContainsPaket> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Second test: Call 'dotnet paket' and see if it returns without an error. Mute all the output. Only run if previous test failed and the test has not been disabled. --> | ||
| <!-- WARNING: This method can lead to processes hanging forever, and should be used as little as possible. See https://github.com/fsprojects/Paket/issues/3705 for details. --> | ||
| <Exec Condition=" '$(PaketExePath)' == '' AND !$(PaketDisableCliTest) AND !$(_ConfigContainsPaket)" Command="dotnet paket --version" IgnoreExitCode="true" StandardOutputImportance="low" StandardErrorImportance="low" > | ||
| <Output TaskParameter="ExitCode" PropertyName="LocalPaketToolExitCode" /> | ||
| </Exec> | ||
|
|
||
| <!-- If paket is installed as CLI use that. Again, only if paket haven't already been found in standard locations. --> | ||
| <PropertyGroup Condition=" '$(PaketExePath)' == '' AND ($(_ConfigContainsPaket) OR '$(LocalPaketToolExitCode)' == '0') "> | ||
| <_PaketCommand>dotnet paket</_PaketCommand> | ||
| </PropertyGroup> |
Contributor
There was a problem hiding this comment.
1. Dotnet tool restore missing 🐞 Bug ☼ Reliability
SetPaketCommand can select dotnet paket based on the local tool manifest, but PaketRestore invokes $(PaketCommand) restore without ever running dotnet tool restore. On a clean checkout (or any environment where the local tool manifest hasn’t been restored yet), MSBuild can fail before compilation because the paket tool command isn’t available.
Agent Prompt
### Issue description
`Paket.Restore.targets` may choose `PaketCommand = "dotnet paket"` (local tool), but it never ensures the tool manifest has been restored (`dotnet tool restore`) before invoking `$(PaketCommand) restore`. This can fail builds on clean machines / first VS load.
### Issue Context
The repo uses a local tool manifest (`dotnet/.config/dotnet-tools.json`) and documentation/scripts already call `dotnet tool restore`. The MSBuild integration should do the same (ideally only when `dotnet paket` is selected) to avoid requiring an undocumented manual pre-step.
### Fix approach (one good option)
1. Add a new target, e.g. `PaketDotnetToolRestore`, that runs `dotnet tool restore` with `WorkingDirectory="$(PaketRootPath)"`.
2. Make `PaketRestore` depend on it (after `SetPaketCommand`).
3. Condition it to only run when the chosen `PaketCommand` is `dotnet paket` (or when `.config/dotnet-tools.json` exists and `_ConfigContainsPaket` is true).
### Fix Focus Areas
- dotnet/.paket/Paket.Restore.targets[66-83]
- dotnet/.paket/Paket.Restore.targets[117-172]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 1257a45 |
Contributor
|
Code review by qodo was updated up to the latest commit 77556b6 |
| "rollForward": false | ||
| } | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
1. Missing newline in dotnet-tools.json 📘 Rule violation ⚙ Maintainability
dotnet/.config/dotnet-tools.json is missing a final newline, which violates the repository’s EditorConfig formatting requirement and can cause formatting checks to fail. This conflicts with the requirement to run/commit formatting so it leaves no remaining diff.
Agent Prompt
## Issue description
`dotnet/.config/dotnet-tools.json` is missing a final newline (`insert_final_newline = true`), which can cause formatting/CI checks to fail.
## Issue Context
Repo-wide formatting rules are defined in `.editorconfig`.
## Fix Focus Areas
- dotnet/.config/dotnet-tools.json[13-13]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 1572713 |
Member
|
The reason I wanted to do #17563 was to make it easier for Renovate to update versions for us. Renovate doesn't work with Paket for some reason, and moving everything over still has us manually updating versions or discovering issues ourselves. |
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
3 participants
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.
💥 What does this PR do?
This pull request migrates the .NET projects to use Paket for dependency management instead of direct NuGet
PackageReferenceentries in the.csprojfiles. The changes remove package references from project files, add Paket references, and update build configuration to ensure compatibility with .NET 10+. This streamlines dependency management and makes transitive dependencies explicit.Dependency management migration:
<PackageReference>entries for libraries such asMicrosoft.Bcl.AsyncInterfaces,System.Collections.Immutable,System.Text.Json,System.Threading.Channels,NUnit,Moq, and others from.csprojfiles, moving their declarations topaket.referencesfiles instead. [1] [2] [3] [4]paket.referencesfiles for each project, explicitly listing the required dependencies per project and target framework. [1] [2] [3] [4]Build and compatibility updates:
paket.dependenciesto add support fornet10.0and included missing dependencies.Paket.Restore.targetsto all relevant.csprojfiles to ensure Paket restores dependencies during build. [1] [2] [3] [4] [5]Directory.Build.propsto suppress .NET 10+ package pruning warnings (NU1510), ensuring clean builds with the new framework.Project configuration cleanup:
paket.referencesfiles where appropriate to conform to Paket's expected format.These changes collectively improve dependency management, future-proof the build for .NET 10+, and simplify project maintenance by consolidating dependency declarations.
🔧 Implementation Notes
paket.dependenciesis single source of truth.🤖 AI assistance
🔄 Types of changes