[FIX] tool-install: Use restore action config options by edvilme · Pull Request #44309 · dotnet/sdk

edvilme · 2024-10-18T18:25:53Z

Addresses https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2221478

If user has an invalid PackageSource on NuGet.config and attempts to install a tool with the --ignore-failed-sources attribute, it still outputs an error.

RestoreActionConfig needs to be passed down to NuGetPackageDownloader

edvilme · 2024-10-18T20:38:49Z

Should also address #36473

nagilson

I tried this and it didn't seem to fix the issue for tool install or tool restore.

Let me know if you want to investigate together.
dotnet tool update dotnet-sos --add-source "c:\Users\***\downloads" --global --ignore-failed-sources

I used the failed source which you had provided.

I would also suggest you add a test like we discussed

edvilme · 2024-10-22T20:44:47Z

Missed adding the RestoreActionConfig to some other methods. It should be working fine now

nagilson · 2024-10-22T23:31:42Z

@baronfel Do we want to backport this to 8.0 once its good? I believe we should. Probably 8.0.3xx?

baronfel · 2024-10-22T23:32:55Z

When was the regression introduced?

nagilson

Fantastic job figuring the correct changes out and also investigating the tests :)
I have a remaining comment I'd like to hear on before we can merge this. There's also that final test you're working on. But the change itself looks great!

nagilson · 2024-10-22T23:46:46Z

When was the regression introduced?

Sometime between .NET 8 RC2 and .NET 7, most likely rc2

baronfel · 2024-10-22T23:50:12Z

Ok then I'd say we should backport all the way back to 8.0.1xx so that source build gets it too.

edvilme · 2024-10-22T23:59:29Z

Fantastic job figuring the correct changes out and also investigating the tests :) I have a remaining comment I'd like to hear on before we can merge this. There's also that final test you're working on. But the change itself looks great!

Thanks a lot! And thanks a lot for all the support.
I also had the concern on the internal vs private, I am happy to look into how the mock pattern as to not expose this unnecessarily. Let me know :)

I have also added the tests to check if the flag works correctly (aka, it does not throw when the feeds are invalid and the --ignore-failed-sources flag is used

edvilme · 2024-10-23T05:58:19Z

Maybe we can use https://api.nuget.org/v3/invalid.json ? That way we at least have some more influence/knowledge on what goes on with the urlSent from my iPhoneOn Oct 22, 2024, at 4:49 PM, Noah Gilson ***@***.***> wrote: @nagilson commented on this pull request. In test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs:

@@ -975,6 +989,16 @@ public void SetPermission(string path, string chmodArgument)

{ } } + + private string _nugetConfigWithInvalidSources = @"{ +<?xml version=""1.0"" encoding=""utf-8""?> +<configuration> + <packageSources> + <add key=""nuget"" value=""https://api.nuget.org/v3/index.json"" /> + <add key=""invalid_source"" value=""https://invalidsource.com/v3/index.json"" /> @dsplaisted Is there a standard feed we can use for testing an invalid source? I couldnt find one. Im not sure I like relying on this website but I dont have anything better unless we have something internal we can use for this. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

dsplaisted · 2024-10-23T11:31:03Z

Maybe we can use https://api.nuget.org/v3/invalid.json ? That way we at least have some more influence/knowledge on what goes on with the url

Yes, that seems like a good idea so that it's a domain we control.

nagilson

All of my comments are resolved now, so this should be good to go. while the blazor wasm test failure is not related to your change, it does look like one remaining test is failing.

I would suggest you prioritize this issue above all else because it seems to impact the most customers.

After that, I would backport your changes to release/8.0.1xx. Don't merge that PR until you get tactics approval and then approval after that to confirm the branch and release is open for changes. You can use the bot to create backports: #43607 (comment) Like how I did here. In that linked backport PR is a template to get tactics approval. Youll need to fill that out and do manual testing to verify that the fix worked on 8.0.1xx as well.

After that, youll need to send an email to the .NET Tactics alias with the same information asking for approval and add the label 'servicing-consider.' Definitely reach out to confirm once youve gotten it all written up or if you have questions 😄

nagilson · 2024-10-23T23:11:19Z

All of my comments are resolved now, so this should be good to go. while the blazor wasm test failure is not related to your change, it does look like one remaining test is failing.

I would suggest you prioritize this issue above all else because it seems to impact the most customers.

After that, I would backport your changes to release/8.0.1xx. Don't merge that PR until you get tactics approval and then approval after that to confirm the branch and release is open for changes. You can use the bot to create backports: #43607 (comment) Like how I did here. In that linked backport PR is a template to get tactics approval. Youll need to fill that out and do manual testing to verify that the fix worked on 8.0.1xx as well. After that, youll need to send an email to the .NET Tactics alias with the same information asking for approval and add the label 'servicing-consider.'

By the way, why are we doing all of this... this was broken starting in 8.0 and most people are going to be on 8.0 since its the latest stable and long term support version of .NET. We want them to get this fix because it's important. But because its a stable and already released build, the quality / QA check bar is a lot higher, so we have a process in place for these changes

edvilme · 2024-10-24T18:02:30Z

it does look like one remaining test is failing

Hello. Yes, I pushed a fix recently and it should be passing now on the ci/cd. I am rebasing and squashing the commits to avoid having lots of little commits.

nagilson · 2024-10-24T18:17:55Z

You can squah and merge the PR without having to rebase or do any extra work by clicking on this button 😉

@/rainersigwald is very good about having a clean PR history and always squashing but our team is much less loose on that sort of thing. I would generally recommend to squash and merge the PR using the GitHub option.

Force-pushing makes it harder to review the PR, because all of the existing changes get wiped away and the order of commits are lost, so it makes it so the entire PR needs to be reviewed again. This also saves you from having to wait for the pipeline to run again, and also if there is some other test that keeps failing that's not your fault, you dont have to deal with it a second/third/etc time~

nagilson

Let's merge
(and also backport) (this change is going to make a lot of people happy)

edvilme · 2024-10-24T18:55:46Z

/backport to to release/8.0.1xx

github-actions · 2024-10-24T18:55:58Z

Started backporting to to: https://github.com/dotnet/sdk/actions/runs/11505684487

github-actions · 2024-10-24T18:56:16Z

@edvilme an error occurred while backporting to to, please check the run log for details!

Error: @edvilme is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=edvilme

edvilme · 2024-10-24T20:08:28Z

/backport to to release/8.0.1xx

github-actions · 2024-10-24T20:08:39Z

Started backporting to to: https://github.com/dotnet/sdk/actions/runs/11506674169

github-actions · 2024-10-24T20:08:57Z

@edvilme an error occurred while backporting to to, please check the run log for details!

Error: The specified backport target branch to wasn't found in the repo.

edvilme · 2024-10-24T20:53:42Z

/backport to release/8.0.1xx

github-actions · 2024-10-24T20:53:53Z

Started backporting to release/8.0.1xx: https://github.com/dotnet/sdk/actions/runs/11507297518

github-actions · 2024-10-24T20:54:12Z

@edvilme backporting to release/8.0.1xx failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: [FIX] tool-install: Use config options
Using index info to reconstruct a base tree...
M	eng/dogfood.sh
M	src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
M	src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalInstaller.cs
A	test/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
A	test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
A	test/dotnet.Tests/CommandTests/ToolInstallLocalCommandTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Tests/dotnet.Tests/CommandTests/ToolInstallLocalCommandTests.cs
Auto-merging src/Tests/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
Auto-merging src/Tests/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
CONFLICT (content): Merge conflict in src/Tests/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
Auto-merging src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalInstaller.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalInstaller.cs
Auto-merging src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
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
Auto-merging src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
Auto-merging src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
Auto-merging eng/dogfood.sh
CONFLICT (content): Merge conflict in eng/dogfood.sh
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 0001 [FIX] tool-install: Use config options
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-10-24T20:54:13Z

@edvilme an error occurred while backporting to release/8.0.1xx, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

edvilme · 2024-10-25T06:53:01Z

Note: because of very big differences between the branches, this cannot be cherrypicked or backported automatically.

edvilme · 2024-10-30T17:03:12Z

/backport to release/9.0.1xx

github-actions · 2024-10-30T17:03:27Z

Started backporting to release/9.0.1xx: https://github.com/dotnet/sdk/actions/runs/11598094603

github-actions · 2024-10-30T17:03:47Z

@edvilme backporting to release/9.0.1xx failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: [FIX] tool-install: Use config options
Using index info to reconstruct a base tree...
M	eng/dogfood.sh
M	src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
M	src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
M	test/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
M	test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
Falling back to patching base and 3-way merge...
Auto-merging test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
Auto-merging test/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
CONFLICT (content): Merge conflict in test/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
Auto-merging src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
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
Auto-merging src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
Auto-merging src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
Auto-merging eng/dogfood.sh
CONFLICT (content): Merge conflict in eng/dogfood.sh
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 0001 [FIX] tool-install: Use config options
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-10-30T17:03:47Z

@edvilme an error occurred while backporting to release/9.0.1xx, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

edvilme · 2024-12-12T22:35:34Z

/backport to release/9.0.1xx

github-actions · 2024-12-12T22:35:47Z

Started backporting to release/9.0.1xx: https://github.com/dotnet/sdk/actions/runs/12305973640

github-actions · 2024-12-12T22:36:08Z

@edvilme backporting to release/9.0.1xx failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: [FIX] tool-install: Use config options
Using index info to reconstruct a base tree...
M	eng/dogfood.sh
M	src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
M	src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
M	src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalInstaller.cs
M	test/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
M	test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
M	test/dotnet.Tests/CommandTests/ToolInstallLocalCommandTests.cs
Falling back to patching base and 3-way merge...
Auto-merging test/dotnet.Tests/CommandTests/ToolInstallLocalCommandTests.cs
Auto-merging test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
CONFLICT (content): Merge conflict in test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs
Auto-merging test/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
CONFLICT (content): Merge conflict in test/Microsoft.DotNet.Tools.Tests.ComponentMocks/ToolPackageDownloaderMock.cs
Auto-merging src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallLocalCommand.cs
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
Auto-merging src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/ToolPackage/ToolPackageDownloader.cs
Auto-merging src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
CONFLICT (content): Merge conflict in src/Cli/dotnet/ToolPackage/IToolPackageDownloader.cs
Auto-merging eng/dogfood.sh
CONFLICT (content): Merge conflict in eng/dogfood.sh
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 0001 [FIX] tool-install: Use config options
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

ghost added Area-Tools untriaged Request triage from a team member labels Oct 18, 2024

edvilme marked this pull request as ready for review October 18, 2024 18:29

edvilme requested a review from a team October 18, 2024 18:34

nagilson self-requested a review October 18, 2024 19:56

This was referenced Oct 18, 2024

Test failure - AllWebProjectsRestoreAndBuild fails with Unbalanced Condition for F# MVC templates #40073

Open

Track signature errors #42870

Closed

nagilson reviewed Oct 21, 2024

View reviewed changes

nagilson approved these changes Oct 22, 2024

View reviewed changes

Comment thread src/Cli/dotnet/commands/dotnet-tool/install/ToolInstallGlobalOrToolPathCommand.cs

nagilson reviewed Oct 22, 2024

View reviewed changes

Comment thread test/dotnet.Tests/CommandTests/ToolInstallGlobalOrToolPathCommandTests.cs Outdated

nagilson reviewed Oct 23, 2024

View reviewed changes

[FIX] tool-install: Use config options

16d81f8

edvilme force-pushed the edvilme-tool-ignore-failed-sources branch from b6340f9 to 16d81f8 Compare October 24, 2024 18:00

nagilson approved these changes Oct 24, 2024

View reviewed changes

Merge branch 'main' into edvilme-tool-ignore-failed-sources

04d99d1

edvilme merged commit 57c6e5f into dotnet:main Oct 28, 2024

edvilme mentioned this pull request Oct 28, 2024

[FIX] tool-install: Support --ignore-failed-sources flag correctly #44487

Merged

Forgind mentioned this pull request Dec 2, 2024

dotnet tool install error installing when a private feed is in nuget.config #24140

Closed

baronfel mentioned this pull request Dec 12, 2024

dotnet tool CLI command fails when nuget.config includes a private package source #45235

Closed

Conversation

edvilme commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edvilme commented Oct 18, 2024

Uh oh!

nagilson left a comment

Choose a reason for hiding this comment

Uh oh!

edvilme commented Oct 22, 2024

Uh oh!

nagilson commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baronfel commented Oct 22, 2024

Uh oh!

nagilson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nagilson commented Oct 22, 2024

Uh oh!

Uh oh!

baronfel commented Oct 22, 2024

Uh oh!

edvilme commented Oct 22, 2024

Uh oh!

edvilme commented Oct 23, 2024 via email

Uh oh!

dsplaisted commented Oct 23, 2024

Uh oh!

nagilson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagilson commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edvilme commented Oct 24, 2024

Uh oh!

nagilson commented Oct 24, 2024

Uh oh!

nagilson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edvilme commented Oct 24, 2024

Uh oh!

github-actions Bot commented Oct 24, 2024

Uh oh!

github-actions Bot commented Oct 24, 2024

Uh oh!

edvilme commented Oct 24, 2024

Uh oh!

github-actions Bot commented Oct 24, 2024

Uh oh!

github-actions Bot commented Oct 24, 2024

Uh oh!

edvilme commented Oct 24, 2024

Uh oh!

github-actions Bot commented Oct 24, 2024

Uh oh!

github-actions Bot commented Oct 24, 2024

Uh oh!

github-actions Bot commented Oct 24, 2024

Uh oh!

edvilme commented Oct 25, 2024

Uh oh!

edvilme commented Oct 30, 2024

Uh oh!

github-actions Bot commented Oct 30, 2024

Uh oh!

github-actions Bot commented Oct 30, 2024

Uh oh!

github-actions Bot commented Oct 30, 2024

Uh oh!

edvilme commented Dec 12, 2024

Uh oh!

github-actions Bot commented Dec 12, 2024

Uh oh!

edvilme commented Oct 18, 2024 •

edited

Loading

nagilson commented Oct 22, 2024 •

edited

Loading

nagilson left a comment •

edited

Loading

nagilson commented Oct 23, 2024 •

edited

Loading

nagilson left a comment •

edited

Loading