-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C#: Automatically use configured private registry feeds #18850
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
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
6b2f348
C#: Add `CODEQL_PROXY_URLS` environment variable
mbg 63d5517
C#: Add list of registries to `DependabotProxy`
mbg 11efb55
C#: Parse environment variables to obtain list of registry URLs
mbg 726123c
C#: Allow specifying package feeds for `dotnet restore` as command li…
mbg 0db6a26
C#: Propagate explicit feeds to `RestoreProjects`
mbg 6b15f77
C#: Fix test failures
mbg a8dde15
C#: Only provide feeds on command line if Dependabot proxy is enabled
mbg 9560593
C#: Fix `.ToList()` being called on `null`
mbg b6c74fe
C#: Narrow `Exception` to `JsonException`
mbg 284f612
C#: Use `StringBuilder` for feed arguments in `GetRestoreArgs`
mbg 51874b8
Apply suggestions from code review
mbg 7a92a72
C#: Change `RegistryConfig` to a record class
mbg d564529
C#: Change `RestoreSettings` to have general `extraArgs` parameter
mbg 92eab47
C#: Refactor `CheckFeeds` to have an overloaded variant that accepts …
mbg 4448369
C#: Check that private package registry feeds are reachable
mbg 7cea2ad
Apply suggestions from code review
mbg d2b88ae
C#: Rename overloaded `CheckFeeds` method and fix comment
mbg 4d3b024
C#: Do not manually add public feed when private registries are used
mbg 73ca2eb
C#: Use `allFeeds` rather than `explicitFeeds` for `RestoreProjects`
mbg be95d33
C#: Obtain all feeds from source directory if there are no `nuget.con…
mbg fe1c098
C#: Accept changes to `.expected` files
mbg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,10 +103,11 @@ | |
compilationInfoContainer.CompilationInfos.Add(("NuGet feed responsiveness checked", checkNugetFeedResponsiveness ? "1" : "0")); | ||
|
||
HashSet<string>? explicitFeeds = null; | ||
HashSet<string>? allFeeds = null; | ||
|
||
try | ||
{ | ||
if (checkNugetFeedResponsiveness && !CheckFeeds(out explicitFeeds)) | ||
if (checkNugetFeedResponsiveness && !CheckFeeds(out explicitFeeds, out allFeeds)) | ||
{ | ||
// todo: we could also check the reachability of the inherited nuget feeds, but to use those in the fallback we would need to handle authentication too. | ||
var unresponsiveMissingPackageLocation = DownloadMissingPackagesFromSpecificFeeds(explicitFeeds); | ||
|
@@ -156,7 +157,7 @@ | |
|
||
var restoredProjects = RestoreSolutions(out var container); | ||
var projects = fileProvider.Projects.Except(restoredProjects); | ||
RestoreProjects(projects, out var containers); | ||
RestoreProjects(projects, allFeeds, out var containers); | ||
|
||
var dependencies = containers.Flatten(container); | ||
|
||
|
@@ -260,8 +261,33 @@ | |
/// Populates dependencies with the relative paths to the assets files generated by the restore. | ||
/// </summary> | ||
/// <param name="projects">A list of paths to project files.</param> | ||
private void RestoreProjects(IEnumerable<string> projects, out ConcurrentBag<DependencyContainer> dependencies) | ||
private void RestoreProjects(IEnumerable<string> projects, HashSet<string>? configuredSources, out ConcurrentBag<DependencyContainer> dependencies) | ||
{ | ||
// Conservatively, we only set this to a non-null value if a Dependabot proxy is enabled. | ||
// This ensures that we continue to get the old behaviour where feeds are taken from | ||
// `nuget.config` files instead of the command-line arguments. | ||
string? extraArgs = null; | ||
|
||
if (this.dependabotProxy is not null) | ||
{ | ||
// If the Dependabot proxy is configured, then our main goal is to make `dotnet` aware | ||
// of the private registry feeds. However, since providing them as command-line arguments | ||
// to `dotnet` ignores other feeds that may be configured, we also need to add the feeds | ||
// we have discovered from analysing `nuget.config` files. | ||
var sources = configuredSources ?? new(); | ||
this.dependabotProxy.RegistryURLs.ForEach(url => sources.Add(url)); | ||
|
||
// Add package sources. If any are present, they override all sources specified in | ||
// the configuration file(s). | ||
var feedArgs = new StringBuilder(); | ||
foreach (string source in sources) | ||
{ | ||
feedArgs.Append($" -s {source}"); | ||
} | ||
|
||
extraArgs = feedArgs.ToString(); | ||
} | ||
|
||
var successCount = 0; | ||
var nugetSourceFailures = 0; | ||
ConcurrentBag<DependencyContainer> collectedDependencies = []; | ||
|
@@ -276,7 +302,7 @@ | |
foreach (var project in projectGroup) | ||
{ | ||
logger.LogInfo($"Restoring project {project}..."); | ||
var res = dotnet.Restore(new(project, PackageDirectory.DirInfo.FullName, ForceDotnetRefAssemblyFetching: true, TargetWindows: isWindows)); | ||
var res = dotnet.Restore(new(project, PackageDirectory.DirInfo.FullName, ForceDotnetRefAssemblyFetching: true, extraArgs, TargetWindows: isWindows)); | ||
assets.AddDependenciesRange(res.AssetsFilePaths); | ||
lock (sync) | ||
{ | ||
|
@@ -674,10 +700,42 @@ | |
return (timeoutMilliSeconds, tryCount); | ||
} | ||
|
||
private bool CheckFeeds(out HashSet<string> explicitFeeds) | ||
/// <summary> | ||
/// Checks that we can connect to all Nuget feeds that are explicitly configured in configuration files | ||
/// as well as any private package registry feeds that are configured. | ||
/// </summary> | ||
/// <param name="explicitFeeds">Outputs the set of explicit feeds.</param> | ||
/// <param name="allFeeds">Outputs the set of all feeds (explicit and inherited).</param> | ||
/// <returns>True if all feeds are reachable or false otherwise.</returns> | ||
private bool CheckFeeds(out HashSet<string> explicitFeeds, out HashSet<string> allFeeds) | ||
{ | ||
(explicitFeeds, allFeeds) = GetAllFeeds(); | ||
HashSet<string> feedsToCheck = explicitFeeds; | ||
|
||
// If private package registries are configured for C#, then check those | ||
// in addition to the ones that are configured in `nuget.config` files. | ||
this.dependabotProxy?.RegistryURLs.ForEach(url => feedsToCheck.Add(url)); | ||
|
||
var allFeedsReachable = this.CheckSpecifiedFeeds(feedsToCheck); | ||
|
||
var inheritedFeeds = allFeeds.Except(explicitFeeds).ToHashSet(); | ||
if (inheritedFeeds.Count > 0) | ||
{ | ||
logger.LogInfo($"Inherited Nuget feeds (not checked for reachability): {string.Join(", ", inheritedFeeds.OrderBy(f => f))}"); | ||
compilationInfoContainer.CompilationInfos.Add(("Inherited Nuget feed count", inheritedFeeds.Count.ToString())); | ||
} | ||
|
||
return allFeedsReachable; | ||
} | ||
|
||
/// <summary> | ||
/// Checks that we can connect to the specified Nuget feeds. | ||
/// </summary> | ||
/// <param name="feeds">The set of package feeds to check.</param> | ||
/// <returns>True if all feeds are reachable or false otherwise.</returns> | ||
private bool CheckSpecifiedFeeds(HashSet<string> feeds) | ||
{ | ||
logger.LogInfo("Checking Nuget feeds..."); | ||
(explicitFeeds, var allFeeds) = GetAllFeeds(); | ||
logger.LogInfo("Checking that Nuget feeds are reachable..."); | ||
|
||
var excludedFeeds = EnvironmentVariables.GetURLs(EnvironmentVariableNames.ExcludedNugetFeedsFromResponsivenessCheck) | ||
.ToHashSet(); | ||
|
@@ -689,7 +747,7 @@ | |
|
||
var (initialTimeout, tryCount) = GetFeedRequestSettings(isFallback: false); | ||
|
||
var allFeedsReachable = explicitFeeds.All(feed => excludedFeeds.Contains(feed) || IsFeedReachable(feed, initialTimeout, tryCount)); | ||
var allFeedsReachable = feeds.All(feed => excludedFeeds.Contains(feed) || IsFeedReachable(feed, initialTimeout, tryCount)); | ||
if (!allFeedsReachable) | ||
{ | ||
logger.LogWarning("Found unreachable Nuget feed in C# analysis with build-mode 'none'. This may cause missing dependencies in the analysis."); | ||
|
@@ -704,14 +762,6 @@ | |
} | ||
compilationInfoContainer.CompilationInfos.Add(("All Nuget feeds reachable", allFeedsReachable ? "1" : "0")); | ||
|
||
|
||
var inheritedFeeds = allFeeds.Except(explicitFeeds).ToHashSet(); | ||
if (inheritedFeeds.Count > 0) | ||
{ | ||
logger.LogInfo($"Inherited Nuget feeds (not checked for reachability): {string.Join(", ", inheritedFeeds.OrderBy(f => f))}"); | ||
compilationInfoContainer.CompilationInfos.Add(("Inherited Nuget feed count", inheritedFeeds.Count.ToString())); | ||
} | ||
|
||
return allFeedsReachable; | ||
} | ||
|
||
|
@@ -760,23 +810,33 @@ | |
} | ||
|
||
// todo: this could be improved. | ||
// We don't have to get the feeds from each of the folders from below, it would be enought to check the folders that recursively contain the others. | ||
var allFeeds = nugetConfigs | ||
.Select(config => | ||
{ | ||
try | ||
{ | ||
return new FileInfo(config).Directory?.FullName; | ||
} | ||
catch (Exception exc) | ||
HashSet<string>? allFeeds = null; | ||
|
||
if (nugetConfigs.Count > 0) | ||
{ | ||
// We don't have to get the feeds from each of the folders from below, it would be enought to check the folders that recursively contain the others. | ||
allFeeds = nugetConfigs | ||
.Select(config => | ||
{ | ||
logger.LogWarning($"Failed to get directory of '{config}': {exc}"); | ||
} | ||
return null; | ||
}) | ||
.Where(folder => folder != null) | ||
.SelectMany(folder => GetFeeds(() => dotnet.GetNugetFeedsFromFolder(folder!))) | ||
.ToHashSet(); | ||
try | ||
{ | ||
return new FileInfo(config).Directory?.FullName; | ||
} | ||
catch (Exception exc) | ||
{ | ||
logger.LogWarning($"Failed to get directory of '{config}': {exc}"); | ||
} | ||
return null; | ||
}) | ||
.Where(folder => folder != null) | ||
.SelectMany(folder => GetFeeds(() => dotnet.GetNugetFeedsFromFolder(folder!))) | ||
.ToHashSet(); | ||
} | ||
else | ||
{ | ||
// If we haven't found any `nuget.config` files, then obtain a list of feeds from the root source directory. | ||
allFeeds = GetFeeds(() => dotnet.GetNugetFeedsFromFolder(this.fileProvider.SourceDir.FullName)).ToHashSet(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice improvement! |
||
} | ||
|
||
logger.LogInfo($"Found {allFeeds.Count} Nuget feeds (with inherited ones) in nuget.config files: {string.Join(", ", allFeeds.OrderBy(f => f))}"); | ||
|
||
|
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
1 change: 1 addition & 0 deletions
1
csharp/ql/integration-tests/all-platforms/standalone_resx/CompilationInfo.expected
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
1 change: 1 addition & 0 deletions
1
csharp/ql/integration-tests/all-platforms/standalone_winforms/CompilationInfo.expected
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
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.
Check notice
Code scanning / CodeQL
Generic catch clause Note
Copilot Autofix
AI 3 months ago
To fix the problem, we should catch specific exceptions that are likely to be thrown by the operation being performed. In this case,
FileInfo(config).Directory?.FullName
can throw exceptions such asArgumentException
,PathTooLongException
,NotSupportedException
,UnauthorizedAccessException
, andDirectoryNotFoundException
. We should catch these specific exceptions and log them accordingly.