Skip to content

Fixed ScriptsToProcess when -Version param is used in Import-Module #3897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 3 commits into from
Jun 5, 2017
Merged

Fixed ScriptsToProcess when -Version param is used in Import-Module #3897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71098a3
Fixed ScriptsToProcess when -Version param is used in Import-Module
May 30, 2017
9ec6651
Addressed feedback
May 31, 2017
03d4772
Updated tests
Jun 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 47 additions & 27 deletions src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2768,38 +2768,58 @@ internal PSModuleInfo LoadModuleManifest(
}

if (scriptsToProcess != null)
{
foreach (string scriptFile in scriptsToProcess)
{
bool found = false;
PSModuleInfo module = LoadModule(scriptFile,
moduleBase,
string.Empty, // prefix (-Prefix shouldn't be applied to dot sourced scripts)
null,
ref options,
manifestProcessingFlags,
out found);

// If we're in analysis, add the detected exports to this module's
// exports
if (found && (ss == null))
{
Version savedBaseMinimumVersion = BaseMinimumVersion;
Version savedBaseMaximumVersion = BaseMaximumVersion;
Version savedBaseRequiredVersion = BaseRequiredVersion;
Guid? savedBaseGuid = BaseGuid;

try
{
BaseMinimumVersion = null;
BaseMaximumVersion = null;
BaseRequiredVersion = null;
BaseGuid = null;

foreach (string scriptFile in scriptsToProcess)
{
foreach (string detectedCmdlet in module.ExportedCmdlets.Keys)
{
manifestInfo.AddDetectedCmdletExport(detectedCmdlet);
}
bool found = false;
PSModuleInfo module = LoadModule(scriptFile,
Copy link
Member

@adityapatwardhan adityapatwardhan May 31, 2017

Choose a reason for hiding this comment

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

Can you use named parameters. Example: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/named-and-optional-arguments

Copy link
Author

@anmenaga anmenaga May 31, 2017

Choose a reason for hiding this comment

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

That is actually existing code; I didn't change it; looks like Github's diff'er has room for improvement :) - see picture below.
Anyway, there are multiple LoadModule calls like this one throughout the file, so if refactoring is to be done - it has to be done to all of them - and that is out of scope of this bugfix.

diff

moduleBase,
string.Empty, // prefix (-Prefix shouldn't be applied to dot sourced scripts)
null,
ref options,
manifestProcessingFlags,
out found);

foreach (string detectedFunction in module.ExportedFunctions.Keys)
// If we're in analysis, add the detected exports to this module's
// exports
if (found && (ss == null))
{
manifestInfo.AddDetectedFunctionExport(detectedFunction);
}
foreach (string detectedCmdlet in module.ExportedCmdlets.Keys)
{
manifestInfo.AddDetectedCmdletExport(detectedCmdlet);
Copy link
Member

@adityapatwardhan adityapatwardhan May 31, 2017

Choose a reason for hiding this comment

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

AddDetected* methods from PSModuleInfo in most cases seem to be called multiple times using a for loop. Will it make sense to add an overload which accepts IEnumerable?

Copy link
Author

@anmenaga anmenaga May 31, 2017

Choose a reason for hiding this comment

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

Maybe. But as previous comment - this would be refactoring of unrelated code - out of scope of this bug fix.

}

foreach (string detectedAlias in module.ExportedAliases.Keys)
{
manifestInfo.AddDetectedAliasExport(detectedAlias,
module.ExportedAliases[detectedAlias].Definition);
foreach (string detectedFunction in module.ExportedFunctions.Keys)
{
manifestInfo.AddDetectedFunctionExport(detectedFunction);
}

foreach (string detectedAlias in module.ExportedAliases.Keys)
{
manifestInfo.AddDetectedAliasExport(detectedAlias,
module.ExportedAliases[detectedAlias].Definition);
}
}
}
}
}
finally
{
BaseMinimumVersion = savedBaseMinimumVersion;
BaseMaximumVersion = savedBaseMaximumVersion;
BaseRequiredVersion = savedBaseRequiredVersion;
BaseGuid = savedBaseGuid;
}
}

Expand Down
41 changes: 41 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,44 @@
(Get-Module -Name $moduleName).Name | Should Be $moduleName
}
}

Describe "Import-Module with ScriptsToProcess" -Tags "CI" {

BeforeAll {
$moduleRootPath = Join-Path $TestDrive 'TestModules'
New-Item $moduleRootPath -ItemType Directory -Force | Out-Null
Push-Location $moduleRootPath
Copy link
Member

@adityapatwardhan adityapatwardhan May 31, 2017

Choose a reason for hiding this comment

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

Why do you need to push location? Can you use full paths for all cmdlets like New-Item, New-ModuleManifest etc?

Copy link
Author

@anmenaga anmenaga May 31, 2017

Choose a reason for hiding this comment

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

I chose push location specifically because I didn't want to deal with full paths. :)


"1 | Out-File out.txt -Append -NoNewline" | Out-File script1.ps1
"2 | Out-File out.txt -Append -NoNewline" | Out-File script2.ps1
New-ModuleManifest module1.psd1 -ScriptsToProcess script1.ps1
New-ModuleManifest module2.psd1 -ScriptsToProcess script2.ps1 -NestedModules module1.psd1
}

AfterAll {
Pop-Location
}

BeforeEach {
New-Item out.txt -ItemType File -Force | Out-Null
}

AfterEach {
$m = @('module1','module2','script1','script2')
remove-module $m -Force -ErrorAction SilentlyContinue
Remove-Item out.txt -Force -ErrorAction SilentlyContinue
}

$testCases = @(
@{ TestNameSuffix = 'for top-level module'; ipmoParms = @{'Name'='.\module1.psd1'}; Expected = '1' }
@{ TestNameSuffix = 'for top-level and nested module'; ipmoParms = @{'Name'='.\module2.psd1'}; Expected = '21' }
@{ TestNameSuffix = 'for top-level module when -Version is specified'; ipmoParms = @{'Name'='.\module1.psd1'; 'Version'='1.0'}; Expected = '1' }
@{ TestNameSuffix = 'for top-level and nested module when -Version is specified'; ipmoParms = @{'Name'='.\module2.psd1'; 'Version'='1.0'}; Expected = '21' }
)

It "Verify ScriptsToProcess are executed <TestNameSuffix>" -TestCases $testCases {
param($TestNameSuffix,$ipmoParms,$Expected)
Import-Module @ipmoParms
Get-Content out.txt | Should Be $Expected
}
}