Make ResourceManagerCache check for alternative resource paths by anmenaga · Pull Request #4139 · PowerShell/PowerShell

anmenaga · 2017-06-29T00:54:58Z

This PR is for 2 things:

From WindowsPS to PSCore resource paths have changed like this example:
WindowsPS: FileSystemProviderStrings
PSCore: System.Management.Automation.resources.FileSystemProviderStrings
... and some existing modules use 'WindowsPS' syntax in their "format.ps1xml" files; for example:
<Text AssemblyName="System.Management.Automation" BaseName="FileSystemProviderStrings" ResourceId="DirectoryDisplayGrouping"/>
So in these cases resource is not found and Import-Module/Update-FormatData returns error on PSCore.
The fix is to check for alternative resource path if original fails.
when a resource is not found, currently error message has wrong assembly path which was tried for loading the resource; for example:
... resource NonExistingResource in assembly C:\System.Management.Automation is not found.
This is rather confusing, specifically because assembly, containing the resource, can be located anywhere in the filesystem. The fix makes error message accurate: ... resource NonExistingResource in assembly C:\GitHub\PowerShell\src\powershell-win-core\bin\Debug\netcoreapp2.0\win10-x64\publish\System.Management.Automation.dll is not found.

Results on PSCore after the fix:

Results on PSCore before the fix:

adityapatwardhan · 2017-06-29T01:07:12Z

src/System.Management.Automation/utils/ResourceManagerCache.cs

+                }
+                else
+                {
+                    newBaseName = assembly.GetName().Name + resourcesSubstring + baseName; // e.g. "System.Management.Automation.resources.FileSystemProviderStrings"


Do not concatenate strings, use a StringBuilder.

A single call to string.Concat with 4 or fewer arguments could be better than using a StringBuilder - the length of the string can be pre-calculated, so there would only be 1 allocation instead of multiple for the StringBuilder, the array of characters in StringBuilder, and the final string allocation.

Updated to string.Concat

adityapatwardhan · 2017-06-29T01:07:47Z

test/powershell/Modules/Microsoft.PowerShell.Utility/FileInfo.format.ps1xml

+            </TableControl>
+        </View>
+    </ViewDefinitions>
+</Configuration>


Missing newline

I think the test should not look anything like something that is built-in - otherwise it might cause problems with other tests.

added newline;
renamed the format file to indicate relation to tests;
these tests are run in separate PowerShell/Runspace instances, so there should be no conflict with other tests.

Today there is no conflict.

You should write the test so there can never be a conflict, even if the code is reused.

It's really simple to use a type name specific to this scenario, so I see no reason to not do as I've suggested.

adityapatwardhan · 2017-06-29T01:11:02Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1

+            $ps.Invoke()
+			$sma = [appdomain]::CurrentDomain.GetAssemblies() | ? { if ($_.Location) {$_.Location.EndsWith("System.Management.Automation.dll")}}
+			$smaLocation = $sma.Location
+			$ps.Streams.Error | %{ $_.Exception.Message.Contains($smaLocation) | Should be $true }


Can you do:

$_.Exception.Message | Should Match $smaLocation

this fails with
parsing 'C:\Users\GitHub\PowerShell\New\src\powershell-win-core\bin\Debug\netcoreapp2.0\win10-x64\publish\System.Management.Automation.dll' - Unrecognized escape sequence \\U.

MatchExactly fails as well.

adityapatwardhan · 2017-06-29T01:11:13Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1

+			$ps.Streams.Error | %{ $_.Exception.Message.Contains($smaLocation) | Should be $true }
+	    }
+    }
+}


Missing newline

adityapatwardhan · 2017-06-29T01:19:51Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1

+			$ps.Streams.Error | %{ $_.Exception.Message.Contains($smaLocation) | Should be $true }
+	    }
+    }
+}


Can we add a test where we lookup are string resource which does not exist and expect an MissingManifestResourceException

this last test is for when resource is not found;
I've added check for FullyQualifiedErrorId

iSazonov · 2017-07-15T16:50:04Z

src/System.Management.Automation/utils/ResourceManagerCache.cs

+
+            // FYI: for a non-existing resource defined by {assembly,baseName,resourceId}
+            // MissingManifestResourceException is thrown only at the time when resource retrieval method such as ResourceManager.GetString or ResourceManager.GetObject is called,
+            // Not when you instantiate a ResourceManager object.


It seems 'FYI' abbreviation should be removed or expanded.
Please reformat and even a width of lines.

iSazonov · 2017-07-15T16:56:10Z

src/System.Management.Automation/utils/ResourceManagerCache.cs

+            // Not when you instantiate a ResourceManager object.
+            try
+            {
+                // try with original baseName


The comment as a single is obvious. So we should combining the comment with next one (move from line 194) to give a reader a full description.

iSazonov · 2017-07-15T16:59:38Z

test/powershell/Modules/Microsoft.PowerShell.Utility/FileInfo.format.ps1xml

@@ -0,0 +1,88 @@
+<Configuration>
+    <SelectionSets>


We should put the file in 'asserts' subfolder.

iSazonov · 2017-07-15T17:03:52Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1

+
+
+Describe "Update-FormatData with resources in CustomControls" -Tags "CI" {
+


The file has multiple wrong indentations. See raw view https://raw.githubusercontent.com/anmenaga/PowerShell/fd95dd19b90e7cb5d0094f52503b59c59ec1ca5b/test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1
It seems you copy-paste tabs.
Could you please reformat by first commit and put new code in next commits?

fixed indentations.

TravisEz13 · 2017-07-20T16:44:08Z

@adityapatwardhan Can you update your review?

PR Changed since review

TravisEz13

No blocking issues

TravisEz13 · 2017-07-21T18:25:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1

+            $null = $ps.AddScript("Update-FormatData -PrependPath $formatFilePath")
+            $ps.Streams.Error.Clear()
+            $ps.Invoke()
+            $ps.HadErrors | Should be $false


Would testing $ps.Streams.Error give a more meaningful error if something went wrong in the test?

good idea; updated.

TravisEz13 · 2017-07-21T18:25:10Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Update-FormatData.Tests.ps1

+            $null = $ps.AddScript("Update-FormatData -PrependPath $formatFilePath")
+            $ps.Streams.Error.Clear()
+            $ps.Invoke()
+            $ps.HadErrors | Should be $false


See previous

adityapatwardhan

LGTM

Update to ResourceManagerCache

fd95dd1

msftclas added the cla-not-required label Jun 29, 2017

anmenaga requested a review from adityapatwardhan June 29, 2017 00:55

adityapatwardhan previously requested changes Jun 29, 2017

View reviewed changes

daxian-dbw assigned TravisEz13 Jul 5, 2017

Andrew added 2 commits July 11, 2017 12:00

PR feedback

b1bc8f9

Updated format ps1xml

baddeeb

anmenaga mentioned this pull request Jul 14, 2017

format.ps1xml files referencing built-in assemblies fail to load #4256

Closed

iSazonov reviewed Jul 15, 2017

View reviewed changes

Andrew added 2 commits July 18, 2017 13:22

fixed tabs in indentation

4ed51b0

PR feedback

5bc3344

TravisEz13 reviewed Jul 21, 2017

View reviewed changes

minor update to verification in tests

70d5bbd

TravisEz13 approved these changes Jul 25, 2017

View reviewed changes

adityapatwardhan approved these changes Aug 4, 2017

View reviewed changes

TravisEz13 changed the title ~~Making ResourceManagerCache check for alternative resource paths~~ Make ResourceManagerCache check for alternative resource paths Aug 4, 2017

TravisEz13 merged commit 99236b1 into PowerShell:master Aug 4, 2017

anmenaga deleted the ResourceManagerCachePatch branch October 31, 2018 21:20



		Describe "Update-FormatData with resources in CustomControls" -Tags "CI" {

Conversation

anmenaga commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Jul 20, 2017

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

iSazonov Jul 15, 2017 •

edited

Loading