Skip to content

C#: Try fallback dotnet restore without nuget.config #15395

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

Merged

Conversation

tamasvajk
Copy link
Contributor

@tamasvajk tamasvajk commented Jan 22, 2024

This PR slightly changes the package restore logic in the dependency manager. When a package couldn't be restored through sln or csproj files, we fall back to a dedicated dotnet restore on the package name. This fallback restore uses the nuget.config file in the repo. This PR changes the fallback logic to first try restoring with the nuget.config, and if that fails with NU1301 error, then the restore is tried again without the nuget.config.

@github-actions github-actions bot added the C# label Jan 22, 2024
@tamasvajk tamasvajk changed the title C#: Try fallback nuget restore without nuget.config C#: Try fallback dotnet restore without nuget.config Jan 23, 2024
@tamasvajk tamasvajk marked this pull request as ready for review January 23, 2024 07:30
@tamasvajk tamasvajk requested a review from a team as a code owner January 23, 2024 07:30
@michaelnebel michaelnebel self-requested a review January 23, 2024 12:40
if (outputLines?.Any(s => s.Contains("NU1301")) == true)
{
// Restore could not be completed because the listed source is unavailable. Try without the nuget.config:
success = dotnet.RestoreProjectToDirectory(tempDir.DirInfo.FullName, missingPackageDirectory.DirInfo.FullName, forceDotnetRefAssemblyFetching: false, out var _, out var _, pathToNugetConfig: null, force: true);
Copy link
Contributor

@michaelnebel michaelnebel Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a suggestion and nothing that should block merging: Since we are now adding a second out parameter (which contains all output lines) and since we have several ignores of the list of assets files (which is calculated eagerly in the call to dotnet.RestoreProjectToDirectory) it might make sense to introduce a new class to carry the result of a "restore" call.
Maybe something like (RestoreResult should be the return type of RestoreProjectToDictionary and RestoreSolutionToDictionary):

        public class RestoreResult
        {
            private readonly IList<string> output;
            public bool Success { get; }

            public RestoreResult(bool success, IList<string>? output)
            {
                Success = success;
                this.output = output ?? Array.Empty<string>();
            }

            private static IEnumerable<string> GetFirstGroupOnMatch(Regex regex, IEnumerable<string> lines) =>
                lines
                    .Select(line => regex.Match(line))
                    .Where(match => match.Success)
                    .Select(match => match.Groups[1].Value);

            public IEnumerable<string> GetAssetsFilePaths() => GetFirstGroupOnMatch(AssetsFileRegex(), output);

            public IEnumerable<string> GetRestoredProjects() => GetFirstGroupOnMatch(RestoredProjectRegex(), output);

            public bool HasNugetError() => output.Any(s => s.Contains("NU1301"));
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll do it in a followup PR.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
I have added a suggestion for a possible refactor.

@tamasvajk tamasvajk merged commit df8d453 into github:main Jan 23, 2024
@tamasvajk tamasvajk deleted the feature/standalone-nuget-restore-retry branch January 23, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants