From bad1221b36d6ca978e73ef0cbc7a9b93ac4181f0 Mon Sep 17 00:00:00 2001 From: Andrew Date: Fri, 28 Apr 2017 10:55:44 -0700 Subject: [PATCH 1/2] PR feedback and resolved merge conflict --- .../engine/Modules/ModuleCmdletBase.cs | 57 ++++++------ .../Module/TestModuleManifest.Tests.ps1 | 93 +++++++++++++++++++ 2 files changed, 119 insertions(+), 31 deletions(-) diff --git a/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs b/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs index ee2a571458a..7c5857db3a3 100644 --- a/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs +++ b/src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs @@ -4103,11 +4103,15 @@ internal static PSModuleInfo LoadRequiredModule(ExecutionContext context, Dictionary> requiredModules = new Dictionary>(new ModuleSpecificationComparer()); if (currentModule != null) { - requiredModules.Add(new ModuleSpecification(currentModule), new List { requiredModuleSpecification }); + requiredModules.Add(new ModuleSpecification(currentModule), new List { requiredModuleSpecification }); + } + if (requiredModuleSpecification != null) + { + requiredModules.Add(requiredModuleSpecification, new List(requiredModuleInfo.RequiredModulesSpecification)); } // We always need to check against the module name and not the file name - hasRequiredModulesCyclicReference = HasRequiredModulesCyclicReference(requiredModuleInfo.Name, + hasRequiredModulesCyclicReference = HasRequiredModulesCyclicReference(requiredModuleSpecification, new List(requiredModuleInfo.RequiredModulesSpecification), new Collection { requiredModuleInfo }, requiredModules, @@ -4426,15 +4430,15 @@ internal static Collection GetModuleIfAvailable(ModuleSpecificatio return result; } - private static bool HasRequiredModulesCyclicReference(string currentModuleName, List requiredModules, IEnumerable moduleInfoList, Dictionary> nonCyclicRequiredModules, out ErrorRecord error) + private static bool HasRequiredModulesCyclicReference(ModuleSpecification currentModuleSpecification, List requiredModules, IEnumerable moduleInfoList, Dictionary> nonCyclicRequiredModules, out ErrorRecord error) { error = null; - if (requiredModules == null || requiredModules.Count == 0) + if (requiredModules == null || requiredModules.Count == 0 || currentModuleSpecification == null) { return false; } - foreach (var moduleSpecification in requiredModules) + foreach (var requiredModuleSpecification in requiredModules) { // The dictionary holds the key-value pair with the following convention // Key --> Module @@ -4447,56 +4451,47 @@ private static bool HasRequiredModulesCyclicReference(string currentModuleName, // Cycle // 1 --->2---->3---->4---> 2 - if (nonCyclicRequiredModules.ContainsKey(moduleSpecification)) + if (nonCyclicRequiredModules.ContainsKey(requiredModuleSpecification)) { // Error out saying there is a cyclic reference PSModuleInfo mo = null; foreach (var i in moduleInfoList) { - if (i.Name.Equals(currentModuleName, StringComparison.OrdinalIgnoreCase)) + if (i.Name.Equals(currentModuleSpecification.Name, StringComparison.OrdinalIgnoreCase)) { mo = i; break; } } Dbg.Assert(mo != null, "The moduleInfo should be present"); - string message = StringUtil.Format(Modules.RequiredModulesCyclicDependency, currentModuleName, moduleSpecification.Name, mo.Path); + string message = StringUtil.Format(Modules.RequiredModulesCyclicDependency, currentModuleSpecification.ToString(), requiredModuleSpecification.ToString(), mo.Path); MissingMemberException mm = new MissingMemberException(message); error = new ErrorRecord(mm, "Modules_InvalidManifest", ErrorCategory.ResourceUnavailable, mo.Path); return true; } - else // Go for the recursive check for the RequiredModules of current module + else // Go for recursive check for the RequiredModules of current requiredModuleSpecification { - // Add required Modules of m to the list - Collection availableModules = GetModuleIfAvailable(moduleSpecification); - List list = new List(); - string moduleName = null; + Collection availableModules = GetModuleIfAvailable(requiredModuleSpecification); if (availableModules.Count == 1) { - moduleName = availableModules[0].Name; - foreach (var rm in availableModules[0].RequiredModulesSpecification) - { - list.Add(rm); - } - // Only add if this element has a required module (meaning, it could lead to a circular reference) + List list = new List(availableModules[0].RequiredModulesSpecification); + // Only add if this required module has nested required modules (meaning, it could lead to a circular reference) if (list.Count > 0) { - nonCyclicRequiredModules.Add(moduleSpecification, list); - } - } - - // We always need to check against the module name and not the file name - if (HasRequiredModulesCyclicReference(moduleName, list, availableModules, nonCyclicRequiredModules, out error)) - { - return true; + nonCyclicRequiredModules.Add(requiredModuleSpecification, list); + // We always need to check against the module specification and not the file name + if (HasRequiredModulesCyclicReference(requiredModuleSpecification, list, availableModules, nonCyclicRequiredModules, out error)) + { + return true; + } + } } } } - // Once depth recursive returns a value, we should remove the current module from the CyclicRequiredModules check list. - // This prevent non related modules get involved in the cycle list. - ModuleSpecification currentModule = new ModuleSpecification(currentModuleName); - nonCyclicRequiredModules.Remove(currentModule); + // Once nested recursive calls are complete, we should remove the current module from the nonCyclicRequiredModules check list. + // This prevents non related modules from getting involved in the cycle list. + nonCyclicRequiredModules.Remove(currentModuleSpecification); // this uses ModuleSpecificationComparer equality comparer return false; } diff --git a/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 b/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 index bcc67145424..f3eb91ba0da 100644 --- a/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 +++ b/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 @@ -125,3 +125,96 @@ Describe "Test-ModuleManifest tests" -tags "CI" { { Test-ModuleManifest -Path $testModulePath -ErrorAction Stop } | ShouldBeErrorId "$error,Microsoft.PowerShell.Commands.TestModuleManifestCommand" } } + + +Describe "Tests for circular references in required modules" -tags "CI" { + + function CreateTestModules([string]$RootPath, [string[]]$ModuleNames, [bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference) + { + $RequiredModulesSpecs = @(); + foreach($moduleDir in New-Item $ModuleNames -ItemType Directory -Force) + { + if ($lastItem) + { + if ($AddVersion -or $AddGuid) {$RequiredModulesSpecs += $lastItem} + else {$RequiredModulesSpecs += $lastItem.ModuleName} + } + + $ModuleVersion = '3.0' + $GUID = New-Guid + + New-ModuleManifest ((join-path $moduleDir.Name $moduleDir.Name) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $ModuleVersion -Guid $GUID + + $lastItem = @{ ModuleName = $moduleDir.Name} + if ($AddVersion) {$lastItem += @{ ModuleVersion = $ModuleVersion}} + if ($AddGuid) {$lastItem += @{ GUID = $GUID}} + } + + if ($AddCircularReference) + { + # rewrite first module's manifest to have a reference to the last module, i.e. making a circular reference + if ($AddVersion -or $AddGuid) + { + $firstModuleName = $RequiredModulesSpecs[0].ModuleName + $firstModuleVersion = $RequiredModulesSpecs[0].ModuleVersion + $firstModuleGuid = $RequiredModulesSpecs[0].GUID + $RequiredModulesSpecs = $lastItem + } + else + { + $firstModuleName = $RequiredModulesSpecs[0] + $firstModuleVersion = '3.0' # does not matter - not used in references + $firstModuleGuid = New-Guid # does not matter - not used in references + $RequiredModulesSpecs = $lastItem.ModuleName + } + + New-ModuleManifest ((join-path $firstModuleName $firstModuleName) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $firstModuleVersion -Guid $firstModuleGuid + } + } + + function TestImportModule([bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference) + { + $moduleRootPath = Join-Path $TestDrive 'TestModules' + New-Item $moduleRootPath -ItemType Directory -Force + Push-Location $moduleRootPath + + $moduleCount = 6 # this depth was enough to find a bug in cyclic reference detection product code; greater depth will slow tests down + $ModuleNames = 1..$moduleCount|%{"TestModule$_"} + + CreateTestModules $moduleRootPath $ModuleNames $AddVersion $AddGuid $AddCircularReference + + $newpath = ";$moduleRootPath" + $env:psmodulepath += $newpath + $lastModule = $ModuleNames[$moduleCount - 1] + + try + { + Import-Module $lastModule -ErrorAction Stop + Get-Module $lastModule | Should Not BeNullOrEmpty + } + finally + { + #cleanup + Remove-Module $ModuleNames -Force -ErrorAction SilentlyContinue + $env:psmodulepath = $env:psmodulepath.TrimEnd($newpath) + Pop-Location + Remove-Item $moduleRootPath -Recurse -Force -ErrorAction SilentlyContinue + } + } + + It "No circular references and RequiredModules field has only module names" { + TestImportModule $false $false $false + } + + It "No circular references and RequiredModules field has module names and versions" { + TestImportModule $true $false $false + } + + It "No circular references and RequiredModules field has module names, versions and GUIDs" { + TestImportModule $true $true $false + } + + It "Add a circular reference to RequiredModules and verify error" { + { TestImportModule $false $false $true } | ShouldBeErrorId "Modules_InvalidManifest,Microsoft.PowerShell.Commands.ImportModuleCommand" + } +} From 64b71d15983c504b321de7289c3de0161ba58f1d Mon Sep 17 00:00:00 2001 From: Andrew Date: Fri, 28 Apr 2017 13:43:13 -0700 Subject: [PATCH 2/2] Fixed test failure on Linux --- .../Module/TestModuleManifest.Tests.ps1 | 89 ++++++++++--------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 b/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 index f3eb91ba0da..0b2325e7ff7 100644 --- a/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 +++ b/test/powershell/engine/Module/TestModuleManifest.Tests.ps1 @@ -129,47 +129,47 @@ Describe "Test-ModuleManifest tests" -tags "CI" { Describe "Tests for circular references in required modules" -tags "CI" { - function CreateTestModules([string]$RootPath, [string[]]$ModuleNames, [bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference) - { - $RequiredModulesSpecs = @(); - foreach($moduleDir in New-Item $ModuleNames -ItemType Directory -Force) - { - if ($lastItem) - { - if ($AddVersion -or $AddGuid) {$RequiredModulesSpecs += $lastItem} - else {$RequiredModulesSpecs += $lastItem.ModuleName} - } - - $ModuleVersion = '3.0' - $GUID = New-Guid - - New-ModuleManifest ((join-path $moduleDir.Name $moduleDir.Name) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $ModuleVersion -Guid $GUID - - $lastItem = @{ ModuleName = $moduleDir.Name} - if ($AddVersion) {$lastItem += @{ ModuleVersion = $ModuleVersion}} - if ($AddGuid) {$lastItem += @{ GUID = $GUID}} - } - - if ($AddCircularReference) - { - # rewrite first module's manifest to have a reference to the last module, i.e. making a circular reference - if ($AddVersion -or $AddGuid) - { - $firstModuleName = $RequiredModulesSpecs[0].ModuleName - $firstModuleVersion = $RequiredModulesSpecs[0].ModuleVersion - $firstModuleGuid = $RequiredModulesSpecs[0].GUID - $RequiredModulesSpecs = $lastItem - } - else - { - $firstModuleName = $RequiredModulesSpecs[0] - $firstModuleVersion = '3.0' # does not matter - not used in references - $firstModuleGuid = New-Guid # does not matter - not used in references - $RequiredModulesSpecs = $lastItem.ModuleName - } - - New-ModuleManifest ((join-path $firstModuleName $firstModuleName) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $firstModuleVersion -Guid $firstModuleGuid - } + function CreateTestModules([string]$RootPath, [string[]]$ModuleNames, [bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference) + { + $RequiredModulesSpecs = @(); + foreach($moduleDir in New-Item $ModuleNames -ItemType Directory -Force) + { + if ($lastItem) + { + if ($AddVersion -or $AddGuid) {$RequiredModulesSpecs += $lastItem} + else {$RequiredModulesSpecs += $lastItem.ModuleName} + } + + $ModuleVersion = '3.0' + $GUID = New-Guid + + New-ModuleManifest ((join-path $moduleDir.Name $moduleDir.Name) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $ModuleVersion -Guid $GUID + + $lastItem = @{ ModuleName = $moduleDir.Name} + if ($AddVersion) {$lastItem += @{ ModuleVersion = $ModuleVersion}} + if ($AddGuid) {$lastItem += @{ GUID = $GUID}} + } + + if ($AddCircularReference) + { + # rewrite first module's manifest to have a reference to the last module, i.e. making a circular reference + if ($AddVersion -or $AddGuid) + { + $firstModuleName = $RequiredModulesSpecs[0].ModuleName + $firstModuleVersion = $RequiredModulesSpecs[0].ModuleVersion + $firstModuleGuid = $RequiredModulesSpecs[0].GUID + $RequiredModulesSpecs = $lastItem + } + else + { + $firstModuleName = $RequiredModulesSpecs[0] + $firstModuleVersion = '3.0' # does not matter - not used in references + $firstModuleGuid = New-Guid # does not matter - not used in references + $RequiredModulesSpecs = $lastItem.ModuleName + } + + New-ModuleManifest ((join-path $firstModuleName $firstModuleName) + ".psd1") -RequiredModules $RequiredModulesSpecs -ModuleVersion $firstModuleVersion -Guid $firstModuleGuid + } } function TestImportModule([bool]$AddVersion, [bool]$AddGuid, [bool]$AddCircularReference) @@ -183,8 +183,9 @@ Describe "Tests for circular references in required modules" -tags "CI" { CreateTestModules $moduleRootPath $ModuleNames $AddVersion $AddGuid $AddCircularReference - $newpath = ";$moduleRootPath" - $env:psmodulepath += $newpath + $newpath = [system.io.path]::PathSeparator + "$moduleRootPath" + $OriginalPSModulePathLength = $env:PSModulePath.Length + $env:PSModulePath += $newpath $lastModule = $ModuleNames[$moduleCount - 1] try @@ -196,7 +197,7 @@ Describe "Tests for circular references in required modules" -tags "CI" { { #cleanup Remove-Module $ModuleNames -Force -ErrorAction SilentlyContinue - $env:psmodulepath = $env:psmodulepath.TrimEnd($newpath) + $env:PSModulePath = $env:PSModulePath.Substring(0,$OriginalPSModulePathLength) Pop-Location Remove-Item $moduleRootPath -Recurse -Force -ErrorAction SilentlyContinue }