Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DotNetCoreCLIV2 not being able to restore custom MsBuild SDKs. #10567

Merged
merged 13 commits into from
Jun 11, 2019

Conversation

Daniel-Khodabakhsh
Copy link
Contributor

Fix for issue #10498 where MsBuild SDKs cannot be restored properly because of a NuGet/dotnet issue where a passed config file is not being used for this part of the restore.

This fix copies over the generated nuget file into the source root before the restore command is used, then deletes it.
Any existing nuget.config files (case sensitive) are backed up and restored after the process.

Tested this with my own custom extension, as well as locally.

@zarenner
Copy link
Contributor

zarenner commented Jun 3, 2019

Thanks for the PR @Daniel-Khodabakhsh. I'd like someone on my team that's more recently familiar with the task than I am to take a look. I know that we've historically tried to keep generated nuget files out of the source dir due to potential security concerns whenever possible, but since this is within the dotnet restore task itself and cleaned up in a finally block it might be ok.

I also want to check if there's a reason (besides just not having been implemented) why we don't use the credential provider here.

@satbai @zjrunner @infin8x @jotaylo

@Daniel-Khodabakhsh
Copy link
Contributor Author

Glad I can help @zarenner!
To answer your question, it's not an issue with not using credential provider. The issue is with a bug in NuGet itself: NuGet/Home#7855
This PR is a work-around until NuGet can fix the issue.

@zarenner
Copy link
Contributor

zarenner commented Jun 4, 2019

I was suggesting that using the credential provider instead of generating temporary nuget.configs might be an alternative possible fix/workaround. My understanding is that

  1. We use --configfile in this task because we generate the temporary config and need to point to it.
  2. --configfile doesn't work with MSBuild SDK packages, as you note.
  3. If we used the credential provider instead of a generated nuget.config, there would be no need to use --configfile to point to the generated nuget.config, and thus we could avoid this known bug.
  4. We already use the credential provider (at least sometimes) in the nuget tasks, but not the dotnet task.

I'm still working through @jcagme's issue with him to see if there is an issue with using the credential provider with MSBuild SDK packages. I don't think there is, but we're still figuring out his issue.

@Daniel-Khodabakhsh
Copy link
Contributor Author

I don't know if using the credential provider is a good idea.
We have a use-case where we'd only want to restore and vet nuget sources in a nuget config and not necessarily any other source included via the csproj <RestoreSouces> PropertyGroup.

@satbai
Copy link
Contributor

satbai commented Jun 4, 2019

The way it works with the nuget restore task is that you will be able to set the package sources needed in the nuget.config file. Then you set PATs for each endpoint, and instead of creating a temp nuget.config with the creds, the task will use the cred provider. This is likely the approach we would take with the dotnet task as well if we decide to switch to the cred provider. In my opinion it would be a good idea to start using the cred provider in the dotnet tasks, because the creation of the temp nuget.config has caused some issues in the past for some people.

@Daniel-Khodabakhsh
Copy link
Contributor Author

Daniel-Khodabakhsh commented Jun 5, 2019

How long will it take to switch to using the credential provider?
Perhaps this is a good temporary fix for now.

@jotaylo
Copy link
Contributor

jotaylo commented Jun 7, 2019

@Daniel-Khodabakhsh your changes look fine, it's odd that the PR build is failing after your merge with master. I'll take a closer look at the failures on Monday.

@jotaylo
Copy link
Contributor

jotaylo commented Jun 11, 2019

I resolved the failing build issue, so I think you'll just need to merge and adjust the versions again (sorry about that!) In case you missed it, there's a little helper script for updating the versions in common/packaging-common

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants