Update PS should try to load the assembly from file path first before… #5161
Conversation
chunqingchen
requested review from
BrucePay,
daxian-dbw,
lzybkr and
vors
as code owners
chunqingchen
force-pushed
the
bugfix4
branch
2 times, most recently
from
c76f23a to
4aa9b99
Compare
chunqingchen
force-pushed
the
bugfix4
branch
4 times, most recently
from
9822a2e to
2777567
Compare
… loading from assemblyName to avoid breaks for existed code
| } | ||
| } | ||
| else if (!String.IsNullOrEmpty(name)) | ||
|
|
There was a problem hiding this comment.
How about we check if the file exists before trying Assembly.LoadFrom? So maybe just change if (!String.IsNullOrEmpty(filename)) to if (File.Exists(filename))?
|
@daxian-dbw your comments are resolved |
| catch (BadImageFormatException badImage) | ||
| { | ||
| error = badImage; | ||
| return null; |
There was a problem hiding this comment.
The return null at this line and line 1269 are not needed.
| // this is a legitimate error on CoreCLR for a newly emited with Add-Type assemblies | ||
| // they cannot be loaded by name, but we are only interested in importing them by path | ||
| } | ||
| catch (BadImageFormatException badImage) |
There was a problem hiding this comment.
The comments in the catch (FileLoadException fileLoadException) block right above this line seems not needed anymore, since we now try loading from path first.
| $results[1] | Should BeExactly "BinaryModuleCmdlet1 exported by the ModuleCmdlets module." | ||
| } | ||
|
|
||
| It "PS should try to load the assembly from assembly name if file path doesn't exist" -skip:(!$IsWindows) { |
There was a problem hiding this comment.
Why is this test skipped on Unix plats?
| $module = Import-Module $TESTDRIVE\test.psd1 -PassThru | ||
| $module.NestedModules | Should Not BeNullOrEmpty | ||
| $assemblyLocation = [psobject].Assembly.location | ||
| $module.NestedModules.ImplementingAssembly.Location | Should Be $assemblyLocation |
There was a problem hiding this comment.
Please unload the module after the test.
|
@daxian-dbw your comment is resolved |
| else | ||
| { | ||
| New-ModuleManifest -Path $TESTDRIVE/test.psd1 -NestedModules /NOExistedPath/System.Management.Automation.dll | ||
| } |
There was a problem hiding this comment.
Can you use Join-Path to avoid the if ($IsWindows) else blocks? For example,
$psdFile = Join-Path $TESTDRIVE test.psd1
$nestedModule = Join-Path NOExistedPath System.Management.Automation.dll
New-ModuleManifest -Path $psdFile -NestedModules $nestedModule
| } | ||
| finally | ||
| { | ||
| Remove-Module $TESTDRIVE\test.psd1 -ErrorAction SilentlyContinue |
There was a problem hiding this comment.
Remove-Module $module -ErrorAction SilentlyContinue would do
| } | ||
| try | ||
| { | ||
| $module = Import-Module $TESTDRIVE\test.psd1 -PassThru |
There was a problem hiding this comment.
Once you have $psdFil replace the $TESTDRIVE\test.psd1 with $psdFil.
|
@daxian-dbw your comment is resolved |
chunqingchen
force-pushed
the
bugfix4
branch
3 times, most recently
from
689ce0c to
87a9bd7
Compare
daxian-dbw
left a comment
daxian-dbw
left a comment
There was a problem hiding this comment.
LGTM. Can you please add [Feature] tag to the commit message of your last commit so that we can validate the fix with a full test run?
|
@chunqingchen I have assigned #5084 to you so you can verify whether this PR fixes that issue. |
2 participants
fix issue #3325
After we merge #4196, the fix will break some our partner's working code. Although the working code often works with an invalid assembly path.
This fix is to follow the old logic to minimize the breaking change.