Fix NullReferenceException for dotnet tool update -g --all
#43157
Conversation
dotnet-issue-labeler
bot
added
Area-Workloads
untriaged
|
/azp run dotnet-sdk-public-ci |
|
Commenter does not have sufficient privileges for PR 43157 in repo dotnet/sdk |
This comment has been minimized.
This comment has been minimized.
DL444
force-pushed
the
dev/dl/global-tools-update-nre
branch
from
4d4e411
to
be0e59c
Compare
There was a problem hiding this comment.
Thank you for this change and contributing to .NET Tools. Your fix looks great to me and I appreciate the thorough writeup. I agree with how you handled the --tool-path scenario though I am admittedly unfamiliar with this part of the codebase as well.
It would be ideal to have a test to demonstrate the behavior is fixed and prevent regression, but not a merge blocker.
Note since this is in main only, it would only go into .NET 10 most likely. I think we want this in .NET 9 too.
|
/backport to release/9.0.1xx-rc2 |
|
Started backporting to release/9.0.1xx-rc2: https:/dotnet/sdk/actions/runs/10910656220 |
|
@dsplaisted Are you ok to merge this without a test and do you agree this is a good candidate for rc2? |
|
/backport to release/8.0.4xx |
|
@DL444 We would be happy to merge this and we are going to merge this fix in 8.0.400 and 9.0.100 rc2 now but it wont be in any of the future releases until we merge it into |
|
/backport to release/8.0.4xx |
|
Started backporting to release/8.0.4xx: https:/dotnet/sdk/actions/runs/10910761384 |
|
@nagilson backporting to release/8.0.4xx failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix null reference exception for full global tools update.
Applying: Fix empty ID in message when global package is up to date.
Using index info to reconstruct a base tree...
M src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 Fix empty ID in message when global package is up to date.
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@nagilson an error occurred while backporting to release/8.0.4xx, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Bumping this to the top so it's more visible. For 8.0.4xx, the code is significantly different so some time will have to be spent to backport it. We would also appreciate it if you could backport this fix so it can go into .NET 8. I will try to take a look at this if it's not done when I have finished some other work items. |
Sure, I'll see what it takes soon, and I assume it should be relatively straightforward. Backporting to .NET 8 probably can't be done automatically, but it shouldn't be too complicated since I originally developed this fix for .NET 8 and then ported to main. By the way, just to be sure: by "retargeting this to
Tests are currently not added because of testability problems I see, and I may need some guidance to proceed. To reproduce the problem, tools have to be actually installed into the runner's global store. Any mocking using an alternative store location will mask the problematic code path, which is why the issue wasn't caught by existing tests. I'm not sure testing this way is acceptable, or if there are better ways to approach this. |
|
Thank you for your help. You can keep this PR as is and change the merge target. Any changes in 9.0.1xx will automatically get merged into the main branch as well. We appreciate your contribution in improving .NET. 😊 Sorry it took a bit for me to get to the review. I will go through the process to get it approved in RC 2 as well as 8.0.400 once the PR is created. To add tests, I see what you mean now and how the original test did not cover this case. I will have to look and see if there is any possible way for us to test this well, so dont worry about that. Off the top of my head, you're right in that the existing test code don't really support testing this scenario. |
DL444
force-pushed
the
dev/dl/global-tools-update-nre
branch
from
be0e59c
to
4e55fda
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
The failures are an unrelated repo issue. Will check in on this again later to rerun |
|
/azp run dotnet-sdk-public-ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The 8.0.4xx backport merged and was servicing approved - @nagilson was this one approved at the same time? |
|
The changes were previously approved but merging was blocked by irrelevant CI problems. |
|
@DL444 they are PR code approved, but we also have a .NET process requirement to get changes approved by our management because of how close we are to release. |
|
Oh, I misunderstood that. Approval status for this PR wasn't communicated before so I'm unable to tell. |
|
This was approved, thank you for checking. |
Fixes #42598 and fixes #43158.