Run tests for #1963 - #2035
Conversation
…wershell dep resolution, add warning when filtering libs
…FM or RID is specified
…into runtimeResolution
Support root and NuGet RID layouts, scope filtered-content merging per install work item, make the platform tests cross-edition and architecture-safe, and fall back to unauthenticated DSC release lookup for fork builds. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Avoid accessing an undefined JsonElement when package metadata contains an empty dependencyGroups array, and cover the case across PowerShell editions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect CI authentication, platform filtering, and dependency handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds platform-aware package installation for #1963, including RID/TFM filtering, dependency selection, and content merging.
Changes:
- Adds runtime identifier and target framework filtering.
- Adds dependency-group parsing and merge support.
- Expands platform tests and CI authentication handling.
File summaries
| File | Description |
|---|---|
test/PSResourceInfo.Tests.ps1 |
Tests JSON dependency conversion. |
test/PlatformFilteringTests/RuntimePackageHelper.Tests.ps1 |
Tests runtime asset filtering. |
test/PlatformFilteringTests/RuntimeIdentifierHelper.Tests.ps1 |
Tests RID detection and compatibility. |
test/PlatformFilteringTests/PlatformAwareInstall.Tests.ps1 |
Tests integrated installation behavior. |
src/code/Utils.cs |
Adds recursive merge support. |
src/code/RuntimePackageHelper.cs |
Parses and filters runtime assets. |
src/code/RuntimeIdentifierHelper.cs |
Detects and resolves RID compatibility. |
src/code/PSResourceInfo.cs |
Selects framework-specific dependencies. |
src/code/InternalHooks.cs |
Exposes testing hooks. |
src/code/InstallPSResource.cs |
Adds RID and TFM parameters. |
src/code/InstallHelper.cs |
Applies RID/TFM filtering and merge behavior. |
src/code/ArgumentCompleter.cs |
Adds RID/TFM completions. |
.ci/test.yml |
Handles CI authentication. |
Review details
Suppressed comments (9)
src/code/PSResourceInfo.cs:744
GetString()throws for a dependency group whosetargetFrameworkis JSON null, and the outer conversion catch then rejects the entire package metadata. Treat null/non-string target frameworks as the existingAnyFrameworkfallback instead of callingGetString()unconditionally.
string tfmString = tfmElement.GetString();
if (!string.IsNullOrWhiteSpace(tfmString))
{
NuGetFramework parsed = NuGetFramework.Parse(tfmString);
src/code/RuntimeIdentifierHelper.cs:178
- The explicit-target overload has the same reversed compatibility test, so
-RuntimeIdentifier linux-x64will includelinux-musl-x64assets even though the target is glibc-based. Only accept candidate RIDs found in the target RID's fallback chain; a candidate's descendants are not valid fallbacks for the target.
// Check if the target is in the candidate RID's compatibility chain
var candidateCompatibleRids = BuildCompatibleRidList(candidateRid);
foreach (var compatibleRid in candidateCompatibleRids)
{
if (string.Equals(targetRid, compatibleRid, StringComparison.OrdinalIgnoreCase))
src/code/RuntimeIdentifierHelper.cs:340
- The musl RID graph is missing the generic
linux-muslfallback. A package containing onlyruntimes/linux-musl/...is therefore not selected forlinux-musl-x64, despite being the generic musl asset; add that node to the chain and cover it with a test.
compatibleRids.Add($"linux-{arch}");
compatibleRids.Add("linux");
compatibleRids.Add("unix");
compatibleRids.Add("any");
src/code/RuntimeIdentifierHelper.cs:405
- The unknown-OS fallback returns
unix-{arch}, but this branch only addsany; consequently a package with a genericunixRID is excluded on that supported fallback platform. Includeunixin the compatibility chain beforeany.
else if (primaryRid.StartsWith("unix", StringComparison.OrdinalIgnoreCase))
{
// Generic Unix compatibility chain
compatibleRids.Add("any");
src/code/RuntimePackageHelper.cs:57
- Requiring a hyphen excludes valid generic RIDs such as
win,linux,osx, andunixfrom root-level filtering. A package containingwin/native.dllis therefore treated as ordinary content and can be installed on non-Windows hosts, even thoughBuildCompatibleRidListmodels these RIDs. Include generic RIDs in the root-level detector.
if (string.IsNullOrEmpty(folderName) || !folderName.Contains("-"))
{
return false;
src/code/RuntimePackageHelper.cs:64
StartsWithalone accepts non-RIDs such aswindows-x64andlinuxfoo-x64because their final segment is a known architecture. On a nonmatching host those ordinary package folders are then removed as runtime assets; require a valid OS/RID token boundary or use a real RID parser.
// Must start with a known OS prefix
bool startsWithKnownOs = false;
foreach (string prefix in s_knownOsPrefixes)
{
if (folderName.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))
test/PSResourceInfo.Tests.ps1:67
- This test only supplies an empty dependency group, so it never exercises the newly added TFM selection or verifies that dependencies and version ranges from the selected group are retained. Add a JSON fixture with at least two framework groups and assert the dependency selected for the current host.
It "Handles an empty dependencyGroups array" {
$resourceInfo = [Microsoft.PowerShell.PSResourceGet.UtilClasses.TestHooks]::ConvertFromJson(
'{"version":"1.0.0","id":"TestPackage","authors":"Test Author","dependencyGroups":[]}',
$repository)
test/PlatformFilteringTests/PlatformAwareInstall.Tests.ps1:236
- Because the assertions are guarded by
if (Test-Path $libDir), a regression that extracts no lib assets makes this test pass without checking any TFM. Assert that$libDirexists before enumerating it so the test actually verifies framework selection.
if (Test-Path $libDir) {
$installedTfmFolders = @((Get-ChildItem $libDir -Directory).Name)
# Should have exactly 1 TFM folder (the best match)
$installedTfmFolders.Count | Should -Be 1
test/PlatformFilteringTests/RuntimeIdentifierHelper.Tests.ps1:167
- This expectation encodes the reverse RID relationship that the production filter uses incorrectly: a generic
win-x64asset is compatible with awin10-x64target, but awin10-x64package asset is not a fallback for awin-x64host. Test the target-aware overload in the valid direction so the test does not require the unsafe behavior.
if ($currentRid -eq 'win-x64') {
# win10-x64 package folder should be compatible on a win-x64 machine
$InternalHooks::IsCompatibleRid('win10-x64') | Should -BeTrue
}
- Files reviewed: 13/13 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ([string]::IsNullOrWhiteSpace($env:GITHUB_TOKEN) -or | ||
| $env:GITHUB_TOKEN -match '^\$\([^)]+\)$') { | ||
| $null = $headers.Remove("Authorization") | ||
| } | ||
|
|
||
| # Get releases | ||
| $releases = Invoke-RestMethod -Uri $api -Headers $headers -RetryIntervalSec 30 -MaximumRetryCount 10 | ||
|
|
| // TFM filtering: for lib/ entries, only extract the best matching TFM | ||
| if (bestLibFramework != null && !ShouldIncludeLibEntry(entry.FullName, bestLibFramework)) | ||
| { |
| _cmdletPassedIn.WriteDebug($"Selected best matching TFM: {bestMatch.GetShortFolderName()} (from {string.Join(", ", libFrameworks)})"); | ||
| } | ||
|
|
||
| return bestMatch; |
| // Check if our platform is in the package RID's compatibility chain | ||
| // e.g., our platform is win-x64, and package has 'win10-x64' folder -> compatible | ||
| // because win10-x64's chain includes win-x64 | ||
| var packageRidCompatibles = BuildCompatibleRidList(rid); | ||
| foreach (var compatibleRid in packageRidCompatibles) | ||
| { | ||
| if (string.Equals(currentRid, compatibleRid, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; |
| _cmdletPassedIn.WriteWarning( | ||
| $"The specified TargetFramework '{_targetFramework}' was not found in this package. " + | ||
| $"No lib/ assemblies were installed. Available TFMs: {(string.IsNullOrEmpty(available) ? "none" : available)}"); |
| companyName: String.Empty, | ||
| copyright: pkgMetadata["copyright"] as String, | ||
| dependencies: new Dependency[] { }, | ||
| dependencies: ParseNuspecDependencyGroups(pkgMetadata), |
| # Current platform RID folder should exist | ||
| $currentRidDir = Join-Path $installPath $currentRid | ||
| Test-Path $currentRidDir | Should -BeTrue |
| @{ Id = 'Newtonsoft.Json'; Version = '[13.0.1, )' }, | ||
| @{ Id = 'System.Memory'; Version = '[4.5.4, )' } | ||
| ) ` | ||
| -IncludeModuleManifest |
| if ($found.Dependencies -and $found.Dependencies.Count -gt 0) { | ||
| $depNames = $found.Dependencies | ForEach-Object { $_.Name } | ||
| $depNames | Should -Contain 'Newtonsoft.Json' | ||
| $depNames | Should -Contain 'System.Memory' | ||
| } |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
PR Summary
PR Context
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.