Add tests in CI that our NuGet packages can reliably install - #84711
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
| // The trailing ':' makes the value optional, so a bare --check-package-install turns the | ||
| // check on. Without it Mono.Options silently discards an explicit value and | ||
| // --check-package-install=false would still run the check. | ||
| { "check-package-install:", "Verify our NuGet packages can be installed", value => checkPackageInstall = value is null || bool.Parse(value) }, |
There was a problem hiding this comment.
Copilot chose to make this optional given it touches network and such; we could also remove the switch if that seems easier.
There was a problem hiding this comment.
Pull request overview
This PR extends BuildBoss (the CI build-artifact validator) with an opt-in check that validates Roslyn shipping NuGet packages can actually be installed and used to compile a consumer project, and wires that check into the Correctness_Build_Artifacts pipeline job.
Changes:
- Added an opt-in
--check-package-installmode to BuildBoss and enabled it in the correctness artifacts CI job. - Introduced a new
PackageInstallCheckerthat generates a scratch project, installs selected Roslyn packages, and builds code that references their APIs across target frameworks. - Centralized NuPkg discovery/version parsing in
SharedUtiland updated existing package checks to use it; documented the new option in repo docs.
Show a summary per file
| File | Description |
|---|---|
| src/Tools/BuildBoss/SharedUtil.cs | Adds shared NuPkg discovery/version helpers and a process runner for capturing dotnet output. |
| src/Tools/BuildBoss/README.md | Documents the new --check-package-install option and its intent. |
| src/Tools/BuildBoss/Program.cs | Adds CLI flag parsing and conditionally runs the package-install check. |
| src/Tools/BuildBoss/PackageInstallCheckerUtil.cs | New checker that restores/installs/builds against selected shipping packages in a scratch directory. |
| src/Tools/BuildBoss/CompilerNuGetCheckerUtil.cs | Switches to the new shared NuPkg finder and removes the local implementation. |
| azure-pipelines.yml | Enables --check-package-install in Correctness_Build_Artifacts. |
| .github/memory/API_MAP.md | Updates documented BuildBoss invocation and notes the new opt-in check. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 2
b833ed6 to
ebc8fe0
Compare
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (3)
src/Tools/BuildBoss/SharedUtil.cs:40
FindNuGetPackageuses.SingleOrDefault(); if more than one matching.nupkgexists, this will throw anInvalidOperationException("Sequence contains more than one element") which loses the helpful context (partialName/directory). Since this runs in CI, it’s better to detect 0/1/many explicitly and include the matches in the exception message.
var regex = $@"^{Regex.Escape(partialName)}\.\d.*\.nupkg$";
var file = Directory
.EnumerateFiles(directory, "*.nupkg")
.Where(filePath => Regex.IsMatch(Path.GetFileName(filePath), regex))
.SingleOrDefault();
return file ?? throw new Exception($"Unable to find unique '{partialName}' in '{directory}'");
src/Tools/BuildBoss/SharedUtil.cs:105
ProcessUtil.Runstarts async output reads viaBeginOutputReadLine/BeginErrorReadLinebut immediately callsWaitForExit()and returns. In this pattern, the finalOutputDataReceived/ErrorDataReceivedcallbacks can still be in-flight afterWaitForExit()returns, so the captured output can be truncated (andoutput.ToString()can race with appends). Add an extra wait and snapshot theStringBuilderunder the same lock before returning.
process.Start();
process.BeginOutputReadLine();
process.BeginErrorReadLine();
process.WaitForExit();
return new ProcessResult(process.ExitCode, output.ToString());
src/Tools/BuildBoss/PackageInstallCheckerUtil.cs:206
GenerateNuGetConfigassumesNuGet.confighas a<packageSources>element and will throw aNullReferenceExceptionif it doesn’t (or if the root is missing), which then gets reported as a generic "Error verifying". This check will be much easier to diagnose if it validates the expected structure and throws a targeted exception.
var repositoryConfigPath = Path.Combine(RepositoryDirectory, "NuGet.config");
var repositoryConfig = XDocument.Load(repositoryConfigPath);
foreach (var element in repositoryConfig.Root.Element("packageSources").Elements("add"))
{
sources.Add(new XElement(element));
}
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
ebc8fe0 to
230a3fe
Compare
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (1)
src/Tools/BuildBoss/PackageInstallCheckerUtil.cs:84
- The scratch directory is a fixed temp path ("RoslynPackageInstallValidation"). If BuildBoss is invoked concurrently (or a prior run left files behind), the recursive delete/create can race or fail, leading to flaky CI runs. Use a unique per-run scratch directory to avoid collisions.
// Build outside the repository so the generated projects behave like a customer's: no
// Directory.Build.props, central package management, or NuGet.config walked up into.
var scratchDirectory = Path.Combine(Path.GetTempPath(), "RoslynPackageInstallValidation");
var globalPackagesDirectory = Path.Combine(scratchDirectory, ".nuget");
PrepareScratchDirectory(scratchDirectory, packagesDirectory, globalPackagesDirectory);
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
230a3fe to
d2e9688
Compare
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (2)
src/Tools/BuildBoss/SharedUtil.cs:116
ProcessUtil.Runstarts async output/error reads, then immediately snapshotsoutputafterWaitForExit(). WithBeginOutputReadLine/BeginErrorReadLine, the finalOutputDataReceived/ErrorDataReceivedcallbacks can still be in-flight after the process exits, so the captured output may be incomplete (making CI failure diagnostics harder). Consider waiting for async reads to finish before returning the buffered output.
process.Start();
process.BeginOutputReadLine();
process.BeginErrorReadLine();
process.WaitForExit();
return new ProcessResult(process.ExitCode, output.ToString());
src/Tools/BuildBoss/PackageInstallCheckerUtil.cs:104
- The catch-all handler only prints
ex.Message, which can omit critical diagnostics (exception type, inner exception, stack) when this checker fails in CI. Since this path is already for unexpected failures, consider writingex.ToString()(or interpolating{ex}) to aid investigation.
catch (Exception ex)
{
textWriter.WriteLine($"Error verifying: {ex.Message}");
return false;
}
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
We've previously had manual testing prior to any release of our NuGet packages to ensure that the packages could actually be installed and represent a complete set without missing dependencies. This automates that testing so we catch issues sooner. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
d2e9688 to
5ab4907
Compare
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
We've previously had manual testing prior to any release of our NuGet packages to ensure that the packages could actually be installed and represent a complete set without missing dependencies. This automates that testing so we catch issues sooner.
Microsoft Reviewers: Open in CodeFlow