Ensure NestedModules property gets populated by Test-ModuleManifest #7859

SteveL-MSFT · 2018-09-25T21:10:38Z

PR Summary

This is a regression introduced by #7145 as an optimization where no further analysis of the module is done if none of the exported capabilities have a wildcard. However, the NestedModules property hasn't been populated yet, so it remains empty. This is a regression as previously this property is always populated if the manifest contains a value.

Fix is to create PSModuleInfo entries for each specified NestedModule even though we don't do actual loading and analysis. Creating the fake NestedModule is similar to how the code already creates fake RequiredModules when not importing.

Also some minor cleanup of repetitive code in the tests.

Fix #7833

PR Checklist

…er analysis

rjmholt · 2018-09-25T21:35:10Z

This seems like the right way to efficiently build/return the nested module info objects. But for the sake of having considered other ideas, wouldn't users expect a fully live nested module info object here?

Since the fake module info is the same type that would normally be returned by Test-ModuleManifest but is hollow, the fact that we took an optimised path means you now don't get back fields like ModuleBase and have no idea why. Unlike RequiredModules, NestedModules really forms a tree, and you might want to traverse that -- but now you wouldn't be able to consistently expect to be able to use that behaviour.

Further down in the same method when nested modules are read, we actually go and read their manifest and load a module info.

Creating fake info objects from the nested module specifications is certainly more efficient, but I think it's a breaking change from previous (pre-regression) behaviour. See Windows PowerShell's version of the same:

PS C:\Users\roholt\Documents\Dev\sandbox\mod> $x = Test-ModuleManifest .\mod.psd1
PS C:\Users\roholt\Documents\Dev\sandbox\mod> $x.NestedModules |fl *


LogPipelineExecutionDetails : False
Name                        : x
Path                        : C:\Users\roholt\Documents\Dev\sandbox\mod\x.psm1
ImplementingAssembly        :
Definition                  :
Description                 :
Guid                        : 00000000-0000-0000-0000-000000000000
HelpInfoUri                 :
ModuleBase                  : C:\Users\roholt\Documents\Dev\sandbox\mod
PrivateData                 :
Tags                        : {}
ProjectUri                  :
IconUri                     :
LicenseUri                  :
ReleaseNotes                :
RepositorySourceLocation    :
Version                     : 0.0
ModuleType                  : Script
Author                      :
AccessMode                  : ReadWrite
ClrVersion                  :
CompanyName                 :
Copyright                   :
DotNetFrameworkVersion      :
ExportedFunctions           : {[Banana, Banana]}
Prefix                      :
ExportedCmdlets             : {}
ExportedCommands            : {[Banana, Banana]}
FileList                    : {}
CompatiblePSEditions        : {}
ModuleList                  : {}
NestedModules               : {}
PowerShellHostName          :
PowerShellHostVersion       :
PowerShellVersion           :
ProcessorArchitecture       : None
Scripts                     : {}
RequiredAssemblies          : {}
RequiredModules             : {}
RootModule                  :
ExportedVariables           : {}
ExportedAliases             : {}
ExportedWorkflows           : {}
ExportedDscResources        : {}
SessionState                :
OnRemove                    :
ExportedFormatFiles         : {}
ExportedTypeFiles           : {}

I think we may be committed to doing the inefficient thing.

PaulHigin

LGTM

PaulHigin · 2018-09-25T21:45:41Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs


                if (!needToAnalyzeScriptModules)
                {
+                    // need to add nested modules to the manifestInfo when no more analysis needs to be done


// Add nested modules ...

SteveL-MSFT · 2018-09-25T22:01:41Z

@rjmholt You raise a fair point. However, I think most cases the user doesn't need a complete object. In the issue, the problem with the current behavior is that the NestedModules property gets cleared out when used with Update-ModuleManifest which is dataloss for the customer. The fake moduleinfo is sufficient as the modulemanifest is really just a hashtable. Although it seems I should add a test to Update-ModuleManifest.

I would suggest deferring whether we want to add capability to get the entire tree of objects until there is a customer request for it.

address codefactor issue added test for update-modulemanifest

iSazonov · 2018-09-26T05:20:49Z

I would suggest deferring whether we want to add capability to get the entire tree of objects until there is a customer request for it.

Maybe use lazy for the objects?

daxian-dbw · 2018-09-27T18:16:13Z

@SteveL-MSFT Thanks for fixing this regression! It looks good to me, but since the same code is used both here and for requiredModules, would it make sense to have a private helper method to encapsulate the code for creating the fake ModuleInfo object?

The helper method can be a iterator method, which can be used for both here an requiredModules:

private IEnumerable<ModuleInfo> CreateFakeModuleObject(IEnumerable<ModuleSpecification> moduleSpecs)
{
    foreach (ModuleSpecification moduleSpec in moduleSpecs)
    {
        var fakeModuleInfo = new PSModuleInfo(moduleSpec.Name, Context, null);
        if (moduleSpec.Guid.HasValue)
        {
            fakeModuleInfo.SetGuid(moduleSpec.Guid.Value);
        }
        fakeModuleInfo.SetVersion(moduleSpec.RequiredVersion ?? moduleSpec.Version);
        yield fakeModuleInfo;
    }
}

SteveL-MSFT · 2018-09-27T19:30:50Z

@daxian-dbw yes, I'll make that change. Thanks!

daxian-dbw

LGTM

iSazonov · 2018-09-28T04:05:12Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

        }

+        /// <summary>
+        /// Helper function to generate fake PSModuleInfo objects from ModuleSpecification objects


Please add final dot.

iSazonov · 2018-09-28T04:09:25Z

test/powershell/engine/Module/TestModuleManifest.Tests.ps1


+    BeforeEach {
+        $testModulePath = "testdrive:/module/test.psd1"
+        New-Item -ItemType Directory -Path testdrive:/module


Please suppress output New-Item -ItemType Directory -Path testdrive:/module > $null.

iSazonov · 2018-09-28T04:13:59Z

test/powershell/engine/Module/UpdateModuleManifest.Tests.ps1

+
+    BeforeEach {
+        $testModulePath = "testdrive:/module/test.psd1"
+        New-Item -ItemType Directory -Path testdrive:/module


iSazonov · 2018-09-28T04:14:46Z

test/powershell/engine/Module/UpdateModuleManifest.Tests.ps1

+    ) {
+        param($exportValue)
+
+        New-Item -ItemType File -Path testdrive:/module/foo.psm1


Fix ensuring NestedModules property gets populated even with no furth…

76c9da1

…er analysis

SteveL-MSFT assigned daxian-dbw Sep 25, 2018

SteveL-MSFT requested review from PaulHigin and daxian-dbw September 25, 2018 21:10

SteveL-MSFT requested a review from BrucePay as a code owner September 25, 2018 21:10

PaulHigin reviewed Sep 25, 2018

View reviewed changes

SteveL-MSFT force-pushed the test-modulemanifest-nestedmodules branch from fd861f1 to 6428aaf Compare September 25, 2018 22:13

address Paul's feedback

36fc6ea

address codefactor issue added test for update-modulemanifest

SteveL-MSFT force-pushed the test-modulemanifest-nestedmodules branch from 6428aaf to 36fc6ea Compare September 25, 2018 22:13

address Dongbo's feedback

a27c5c8

daxian-dbw approved these changes Sep 27, 2018

View reviewed changes

iSazonov reviewed Sep 28, 2018

View reviewed changes

daxian-dbw added 3 commits September 28, 2018 10:31

Add the ending dot for doc comment.

7d4b364

Ignore the output of 'New-Item' as appropriate

d7c718a

Ignore the output of 'New-Item' - take 2

89e330c

iSazonov approved these changes Sep 28, 2018

View reviewed changes

daxian-dbw merged commit cc44781 into PowerShell:master Sep 28, 2018

SteveL-MSFT deleted the test-modulemanifest-nestedmodules branch September 28, 2018 20:32

Ensure NestedModules property gets populated by Test-ModuleManifest #7859

Ensure NestedModules property gets populated by Test-ModuleManifest #7859

Uh oh!

Conversation

SteveL-MSFT commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

rjmholt commented Sep 25, 2018

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

PaulHigin Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Sep 25, 2018

Uh oh!

iSazonov commented Sep 26, 2018

Uh oh!

daxian-dbw commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Sep 27, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SteveL-MSFT commented Sep 25, 2018 •

edited

Loading

daxian-dbw commented Sep 27, 2018 •

edited

Loading