Skip to content

Run tests for #1963 - #2035

Open
alerickson wants to merge 12 commits into
masterfrom
runtimePackageResolution2
Open

Run tests for #1963#2035
alerickson wants to merge 12 commits into
masterfrom
runtimePackageResolution2

Conversation

@alerickson

Copy link
Copy Markdown
Member

PR Summary

PR Context

PR Checklist

Justin Chung and others added 11 commits March 16, 2026 17:50
…wershell dep resolution, add warning when filtering libs
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

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@alerickson

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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 whose targetFramework is JSON null, and the outer conversion catch then rejects the entire package metadata. Treat null/non-string target frameworks as the existing AnyFramework fallback instead of calling GetString() 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-x64 will include linux-musl-x64 assets 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-musl fallback. A package containing only runtimes/linux-musl/... is therefore not selected for linux-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 adds any; consequently a package with a generic unix RID is excluded on that supported fallback platform. Include unix in the compatibility chain before any.
            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, and unix from root-level filtering. A package containing win/native.dll is therefore treated as ordinary content and can be installed on non-Windows hosts, even though BuildCompatibleRidList models these RIDs. Include generic RIDs in the root-level detector.
            if (string.IsNullOrEmpty(folderName) || !folderName.Contains("-"))
            {
                return false;

src/code/RuntimePackageHelper.cs:64

  • StartsWith alone accepts non-RIDs such as windows-x64 and linuxfoo-x64 because 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 $libDir exists 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-x64 asset is compatible with a win10-x64 target, but a win10-x64 package asset is not a fallback for a win-x64 host. 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.

Comment thread .ci/test.yml
Comment on lines +112 to 119
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

Comment thread src/code/InstallHelper.cs
Comment on lines +1357 to +1359
// TFM filtering: for lib/ entries, only extract the best matching TFM
if (bestLibFramework != null && !ShouldIncludeLibEntry(entry.FullName, bestLibFramework))
{
Comment thread src/code/InstallHelper.cs
_cmdletPassedIn.WriteDebug($"Selected best matching TFM: {bestMatch.GetShortFolderName()} (from {string.Join(", ", libFrameworks)})");
}

return bestMatch;
Comment on lines +135 to +147
// 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;
Comment thread src/code/InstallHelper.cs
Comment on lines +1413 to +1415
_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),
Comment on lines +180 to +182
# 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
Comment on lines +399 to +403
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'
}
@alerickson

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

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.

2 participants