GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Ensure NestedModules property gets populated by Test-ModuleManifest by SteveL-MSFT · Pull Request #7859 · PowerShell/PowerShell

Conversation

Steve Lee (SteveL-MSFT) commented Sep 25, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
User-facing changes
- Not Applicable
- OR
- User-facing Documentation needed
  - Issue filed - Issue link:
Testing - New and feature
- Not Applicable or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
  - Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

maddieclayton reacted with thumbs up emoji

All reactions

👍 1 reaction


          Fix ensuring NestedModules property gets populated even with no furth…

76c9da1

…er analysis

Steve Lee (SteveL-MSFT) assigned Dongbo Wang (daxian-dbw)

Sep 25, 2018

Steve Lee (SteveL-MSFT) requested review from Paul Higinbotham (PaulHigin) and Dongbo Wang (daxian-dbw)

September 25, 2018 21:10

Steve Lee (SteveL-MSFT) requested a review from Bruce Payette (BrucePay) as a code owner

September 25, 2018 21:10

Rob Holt (rjmholt) commented Sep 25, 2018

Copy link

Copy Markdown

Collaborator

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Paul Higinbotham (PaulHigin) reviewed

Sep 25, 2018

View reviewed changes

Paul Higinbotham (PaulHigin) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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


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

Paul Higinbotham (PaulHigin) Sep 25, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

// Add nested modules ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) commented Sep 25, 2018

Copy link

Copy Markdown

Member Author

Rob Holt (@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.

rjmholt and daxian-dbw reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (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

Steve Lee (SteveL-MSFT) force-pushed the test-modulemanifest-nestedmodules branch from 6428aaf to 36fc6ea Compare

September 25, 2018 22:13

Ilya (iSazonov) commented Sep 26, 2018

Copy link

Copy Markdown

Collaborator

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) commented Sep 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Steve Lee (@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;
    }
}

rjmholt and SteveL-MSFT reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) commented Sep 27, 2018

Copy link

Copy Markdown

Member Author

Dongbo Wang (@daxian-dbw) yes, I'll make that change. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          address Dongbo's feedback

a27c5c8

Dongbo Wang (daxian-dbw) approved these changes

Sep 27, 2018

View reviewed changes

Dongbo Wang (daxian-dbw) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Ilya (iSazonov) reviewed

Sep 28, 2018

View reviewed changes

Comment thread

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

		}

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

Ilya (iSazonov) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add final dot.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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


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

Ilya (iSazonov) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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


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

Ilya (iSazonov) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The same.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

		) {
		param($exportValue)

		New-Item -ItemType File -Path testdrive:/module/foo.psm1

Ilya (iSazonov) Sep 28, 2018

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The same.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (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

Ilya (iSazonov) approved these changes

Sep 28, 2018

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit cc44781 into PowerShell:master

Sep 28, 2018

Steve Lee (SteveL-MSFT) deleted the test-modulemanifest-nestedmodules branch

September 28, 2018 20:32

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Ensure NestedModules property gets populated by Test-ModuleManifest (P…

141c316

…owerShell#7859)

Reviewers

Dongbo Wang (daxian-dbw)

daxian-dbw approved these changes

Ilya (iSazonov)

iSazonov approved these changes

Bruce Payette (BrucePay)

Awaiting requested review from BrucePay

+1 more reviewer

Paul Higinbotham (PaulHigin)

PaulHigin left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test-ModuleManifest does not populate the NestedModules field

Uh oh!

There was an error while loading. Please reload this page.

5 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Steve Lee (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!

Rob Holt (rjmholt) commented Sep 25, 2018

Uh oh!

Paul Higinbotham (PaulHigin) left a comment

Choose a reason for hiding this comment

Uh oh!

Paul Higinbotham (PaulHigin) Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Sep 25, 2018

Uh oh!

Ilya (iSazonov) commented Sep 26, 2018

Uh oh!

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

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) commented Sep 27, 2018

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Steve Lee (SteveL-MSFT) commented Sep 25, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dongbo Wang (daxian-dbw) commented Sep 27, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.