PS should try to load the assembly from file path first before loading from assemblyName #4196
Conversation
|
Can you add a synthetic test case? |
0e8289f to
0f4d8e8
Compare
|
@SteveL-MSFT test is added |
| $path = (Get-ChildItem $TESTDRIVE\System.Management.Automation.dll).FullName | ||
| [System.AppDomain]::CurrentDomain.GetAssemblies().Location.Contains($path) | Should be $true | ||
| } | ||
| } |
| public class TestBinaryModuleCmdlet1Command | ||
| { | ||
| } | ||
|
|
|
|
||
| if (!String.IsNullOrEmpty(filename)) | ||
| { | ||
| error = null; |
There was a problem hiding this comment.
this is redundant from line 1329
| } | ||
| catch (FileNotFoundException fileNotFound) | ||
| { | ||
| error = fileNotFound; |
There was a problem hiding this comment.
if we were given a filepath and it doesn't exist, it doesn't seem like we should try to load something that happens to have the same name but different path
| } | ||
| catch (FileLoadException fileLoadException) | ||
| { | ||
| error = fileLoadException; |
There was a problem hiding this comment.
if we were given a filepath and it doesn't exist, it doesn't seem like we should try to load something that happens to have the same name but different path
| return null; | ||
| } | ||
| } | ||
|
|
||
| #if !CORECLR// Assembly.LoadWithPartialName is not in CoreCLR. In CoreCLR, 'LoadWithPartialName' can be replaced by Assembly.Load with the help of AssemblyLoadContext. |
There was a problem hiding this comment.
We are removing FullCLR code, so you should remove this since you're touching this file
| } | ||
| } | ||
|
|
||
| if (loadedAssembly != null) |
There was a problem hiding this comment.
be consistent with line 1338 and return loadedAssembly on line 1375
| } | ||
| } | ||
|
|
||
| string fixedName = null; |
There was a problem hiding this comment.
since the FullCLR code should be removed fixedName is only used within this if statement so move it inside the if statement
| catch (BadImageFormatException badImage) | ||
| { | ||
| error = badImage; | ||
| return null; |
There was a problem hiding this comment.
if you didn't successfully load the assembly, loadedAssembly should still be null, so I think you can get rid of the return null; statements and just return loadedAssemb
67E6
ly at the end
| } | ||
|
|
||
| string fixedName = null; | ||
| if (!String.IsNullOrEmpty(name)) |
|
@SteveL-MSFT your comments are resolved. thank you |
| error = securityException; | ||
| } | ||
| } | ||
|
|
|
@SteveL-MSFT your comment is resolved thanks |
|
|
||
| #Ignore slash format difference under windows/Unix | ||
| $path = (Get-ChildItem $TESTDRIVE\System.Management.Automation.dll).FullName | ||
| [System.AppDomain]::CurrentDomain.GetAssemblies().Location.Contains($path) | Should be $true |
There was a problem hiding this comment.
This works only because the actual assembly name of the assembly generated from Add-Type is not the same as the file name.
There was a problem hiding this comment.
Instead of this, you can use Import-Module -Passthru and have the exact Assembly instance to validate.
We also want to avoid Should Be $true because the logs are less useful, so I'd like to see something like:
$assem = Import-Module -PassThru ...
$assem.Location | Should Be $Path|
This change seems risky to me.
Add @lzybkr to reviewers to get more inputs on this change. |
| using System; | ||
| namespace ModuleCmdlets | ||
| { | ||
| public class TestBinaryModuleCmdlet1Command |
There was a problem hiding this comment.
You should make this a complete cmdlet that can be called. That is the real scenario that needs to be tested.
…g from assemblyName
|
@daxian-dbw Loading assembly in GAC regardless user's given file path is exactly where the bug scenario comes from. @lzybkr your comments are resolved. |
|
|
||
| Describe "Import-Module for Binary Modules" -Tags 'CI' { | ||
|
|
||
| It "PS should try to load the assembly from file path first" { |
There was a problem hiding this comment.
Are you sure this test validates the fix? It passes even without your fix.
There was a problem hiding this comment.
previously when system.automation.management is used loading error will be occured.
Now we have to check the actual cmdlet is loaded.
Resolved.
|
@chunqingchen please don't override existing commit history. Instead, push new commits to address comments. |
|
@chunqingchen if we decide to take this breaking change, then it'd better go in as soon as possible so that we have more time to validate the potential impact. Can you please address the comments as soon as you get a chance? |
2902f5d to
ec9aa56
Compare
|
@daxian-dbw thanks for your comments. your comment is resolved. |
ec9aa56 to
192de66
Compare
| "@ | ||
|
|
||
| Add-Type -TypeDefinition $src -OutputAssembly $TESTDRIVE\System.dll | ||
| $assembly = Import-Module $TESTDRIVE\System.dll -Force -PassThru |
There was a problem hiding this comment.
Once the dll loaded, you will get errors when Pester tries to remove $TestDrive. I suggest you do import-module, $module.Path and Test-BinaryModuleCmdlet1 in a new powershell instance, and use the returned value for validation, so that there won't be any errors when Pester removes $TestDrive.
There was a problem hiding this comment.
Plus, Import-Module -PassThru returns a PSModuleInfo object, so the value name should be $module instead of $assembly.
|
|
||
| #Ignore slash format difference under windows/Unix | ||
| $path = (Get-ChildItem $TESTDRIVE\System.dll).FullName | ||
| $assembly.Path | Should be $path |
There was a problem hiding this comment.
I think your actual intent is to use $module.ImplementingAssembly.Location.
|
@chunqingchen Thanks for the update, I left a few more comments. Please push new commits to address the commits without overriding the commit history. |
| @@ -109,12 +109,17 @@ namespace ModuleCmdlets | |||
| "@ | |||
|
|
|||
| Add-Type -TypeDefinition $src -OutputAssembly $TESTDRIVE\System.dll | |||
| $assembly = Import-Module $TESTDRIVE\System.dll -Force -PassThru | |||
| ## Create a new Runspace and define helper functions in it | |||
| $powershell = [powershell]::Create() | |||
There was a problem hiding this comment.
Sorry that maybe I wasn't clear on my comment. I suggest you use powershell.exe to gather the information you want to test. In that way, the generated system.dll won't be loaded into the current app domain and thus Pester can remove it properly later.
It should be something like this:
$results = powershell -noprofile -c "`$module = Import-Module $TESTDRIVE\System.dll -Pssthru; `$module.ImplementingAssembly.Location; Test-BinaryModuleCmdlet1"
$results[0] | Should Be $path
$results[1] | Should BeExactly "BinaryModuleCmdlet1 exported by the ModuleCmdlets module."
c1086e2 to
3e99b29
Compare
|
@chunqingchen Please fix the test failure. |
3e99b29 to
52ac1bb
Compare
|
thanks @daxian-dbw , checks are passed. |
Fix issue #3325
Summary of the issue:
visual studio cannot import the right assembly contains in their test vsix package through package manage console. It will always load the assembly contains in the vs location
Root cause of the issue:
loadassembly funtion will always try to load the assembly based on the assembly name first, then try the filename. In this case, no matter what assembly path psd1 file gives, ps will ignore and load the assembly in the vs cache.
Fix:
Since the filename is given, we should try it first before going to the assembly name.
The test scenario of this issue is very specific and does not contains in our ps enviroment. Attached the screen shot before/after the fix.
Before the fix:
After the fix:
