GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — PS should try to load the assembly from file path first before loading from assemblyName by chunqingchen · Pull Request #4196 · PowerShell/PowerShell

Conversation

@chunqingchen

@chunqingchen Chunqing Chen (chunqingchen) commented Jul 7, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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:
image

After the fix:
image

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
alpaix and mishra14 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 7, 2017
@daxian-dbw
Dongbo Wang (daxian-dbw) requested a review from Aditya Patwardhan (adityapatwardhan) July 7, 2017 16:26
@daxian-dbw Dongbo Wang (daxian-dbw) self-assigned this Jul 7, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Dongbo Wang (daxian-dbw) July 7, 2017 16:39
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jul 14, 2017

Copy link
Copy Markdown
Member

Can you add a synthetic test case?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch 4 times, most recently from 0e8289f to 0f4d8e8 Compare August 1, 2017 00:20
@chunqingchen Chunqing Chen (chunqingchen) changed the title PS should try to load the assembly from file path first before find a… PS should try to load the assembly from file path first before from assemblyName only Aug 1, 2017
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 7fdb970 to 8091493 Compare August 1, 2017 03:11
@chunqingchen Chunqing Chen (chunqingchen) changed the title PS should try to load the assembly from file path first before from assemblyName only PS should try to load the assembly from file path first before loading from assemblyName Aug 1, 2017
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 8091493 to 9bee62a Compare August 1, 2017 03:23
@chunqingchen

Chunqing Chen (chunqingchen) commented Aug 1, 2017

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) test is added
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Aug 1, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

add newline at end of file

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

delete this empty line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this is redundant from line 1329

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We are removing FullCLR code, so you should remove this since you're touching this file

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

be consistent with line 1338 and return loadedAssembly on line 1375

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

since the FullCLR code should be removed fixedName is only used within this if statement so move it inside the if statement

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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 loadedAssembly at the end

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

make this else if

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 2, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 9bee62a to 5e09204 Compare August 2, 2017 20:23
@chunqingchen

Chunqing Chen (chunqingchen) commented Aug 3, 2017

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) your comments are resolved. thank you
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Aug 3, 2017
View reviewed changes
Comment thread src/System.Management.Automation/engine/ExecutionContext.cs Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 3, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Delete blank line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 4, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 5e09204 to 963c997 Compare August 4, 2017 01:41
@chunqingchen

Chunqing Chen (chunqingchen) commented Aug 4, 2017

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) your comment is resolved thanks
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Aug 4, 2017
View reviewed changes

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Aug 4, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1 Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 4, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This works only because the actual assembly name of the assembly generated from Add-Type is not the same as the file name.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lzybkr Jason Shirk (lzybkr) Aug 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Aug 4, 2017

Copy link
Copy Markdown
Member

This change seems risky to me.

  1. If an assembly is already loaded, and you try to load another same assembly file of a different version, Assembly.LoadFrom will throw; if you want to load another assembly file with the same version, then the already loaded assembly will be returned. See an example here:
PS:6> [System.Reflection.AssemblyName]::GetAssemblyName("F:\tmp\System.Management.Automation.dll")

Version        Name
-------        ----
6.0.1.0        System.Management.Automation

PS:7> [psobject].Assembly.GetName()

Version        Name
-------        ----
6.0.0.0        System.Management.Automation

PS:8> [System.Reflection.Assembly]::LoadFrom("F:\tmp\System.Management.Automation.dll")
Exception calling "LoadFrom" with "1" argument(s): "Could not load file or assembly 'System.Management.Automation,
Version=6.0.1.0, Culture=neutral, PublicKeyToken=null'."
At line:1 char:1
+ [System.Reflection.Assembly]::LoadFrom("F:\tmp\System.Management.Auto ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : FileLoadException
  1. The original code prefers to assemblies in GAC, while the altered code prefers to file name. This is a behavior change, and since we now looking for GAC assemblies in powershell core for Assembly.Load call, this change could cause problems.

Add Jason Shirk (@lzybkr) to reviewers to get more inputs on this change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
Dongbo Wang (daxian-dbw) requested review from Jason Shirk (lzybkr) and removed request for Aditya Patwardhan (adityapatwardhan) August 4, 2017 19:26
lzybkr
Jason Shirk (lzybkr) reviewed Aug 7, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1 Outdated

@lzybkr Jason Shirk (lzybkr) Aug 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You should make this a complete cmdlet that can be called. That is the real scenario that needs to be tested.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 963c997 to f1947af Compare August 31, 2017 07:08
@chunqingchen
Chunqing Chen (chunqingchen) requested review from Bruce Payette (BrucePay) and sergei (vors) as code owners August 31, 2017 07:08
@chunqingchen
Chunqing Chen (chunqingchen) removed the request for review from Bruce Payette (BrucePay) August 31, 2017 07:08
@chunqingchen
Chunqing Chen (chunqingchen) removed the request for review from sergei (vors) August 31, 2017 07:08
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from f1947af to 37aa739 Compare August 31, 2017 08:31
@chunqingchen
PS should try to load the assembly from file path first before loadin…
cb1f0af 
…g from assemblyName
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 37aa739 to cb1f0af Compare August 31, 2017 08:38
@chunqingchen

Chunqing Chen (chunqingchen) commented Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

Dongbo Wang (@daxian-dbw)
According to dongbo's concern about the exception. If user has specified the file path of the assembly, we should load the assembly according to user's willing. If an exception is thrown as assembly is already loaded, user should correct the file path or just give assembly name instead.

Loading assembly in GAC regardless user's given file path is exactly where the bug scenario comes from.

Jason Shirk (@lzybkr) your comments are resolved.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) requested changes Sep 20, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1

Describe "Import-Module for Binary Modules" -Tags 'CI' {

It "PS should try to load the assembly from file path first" {

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are you sure this test validates the fix? It passes even without your fix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@chunqingchen Chunqing Chen (chunqingchen) Sep 27, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

previously when system.automation.management is used loading error will be occured.
Now we have to check the actual cmdlet is loaded.
Resolved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 20, 2017

Copy link
Copy Markdown
Member

Chunqing Chen (@chunqingchen) please don't override existing commit history. Instead, push new commits to address comments.
Also, please make sure you add [Feature] label to your next commit message so that all tests can be validated with your fix.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 22, 2017

Copy link
Copy Markdown
Member

Chunqing Chen (@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@msftgits Microsoft GitHub User (msftgits) removed the cla-not-required label Sep 26, 2017
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 2902f5d to ec9aa56 Compare September 27, 2017 12:32
@chunqingchen

Chunqing Chen (chunqingchen) commented Sep 27, 2017

Copy link
Copy Markdown
Contributor Author

Dongbo Wang (@daxian-dbw) thanks for your comments. your comment is resolved.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@chunqingchen
[Feature] fix test so it would test if the temporary assembly is real…
192de66 
…ly loaded
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from ec9aa56 to 192de66 Compare September 27, 2017 12:46
daxian-dbw
Dongbo Wang (daxian-dbw) requested changes Sep 27, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1 Outdated
"@

Add-Type -TypeDefinition $src -OutputAssembly $TESTDRIVE\System.dll
$assembly = Import-Module $TESTDRIVE\System.dll -Force -PassThru

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Plus, Import-Module -PassThru returns a PSModuleInfo object, so the value name should be $module instead of $assembly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1 Outdated

#Ignore slash format difference under windows/Unix
$path = (Get-ChildItem $TESTDRIVE\System.dll).FullName
$assembly.Path | Should be $path

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think your actual intent is to use $module.ImplementingAssembly.Location.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 27, 2017

Copy link
Copy Markdown
Member

Chunqing Chen (@chunqingchen) Thanks for the update, I left a few more comments. Please push new commits to address the commits without overriding the commit history.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) requested changes Sep 28, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1 Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from c1086e2 to 3e99b29 Compare September 28, 2017 23:06
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 29, 2017

Copy link
Copy Markdown
Member

Chunqing Chen (@chunqingchen) Please fix the test failure.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@chunqingchen
[Feature] fix test so it would test if the temporary assembly is real…
52ac1bb 
…ly loaded 2
@chunqingchen
Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 3e99b29 to 52ac1bb Compare September 29, 2017 01:43
@chunqingchen

Chunqing Chen (chunqingchen) commented Sep 29, 2017

Copy link
Copy Markdown
Contributor Author

thanks Dongbo Wang (@daxian-dbw) , checks are passed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Sep 29, 2017
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit e3a004c into PowerShell:master Sep 29, 2017
@iSazonov Ilya (iSazonov) mentioned this pull request Oct 11, 2017
Hosted PS core in ASP.Net core The system cannot find the file specified #5084
Closed
@chunqingchen Chunqing Chen (chunqingchen) mentioned this pull request Oct 19, 2017
Update PS should try to load the assembly from file path first before… #5161
Merged
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@chunqingchen @daxian-dbw
Change PS to load the assembly with 'Assembly.LoadFrom' before 'Assem…
403390f 
…bly.Load' when the file path is given (PowerShell#4196)

It's to solve a side-by-side problem we have with powershell core. If a .NET Core version assembly has the same name as it's .NET ancestor in GAC, then even if you specify the file path to `Import-Module`, powershell core will still load the one in GAC because it tries 'Assembly.Load' first.
Now we change it to use `Assembly.LoadFrom` first when a file path is given, so it works for modules that have side-by-side assemblies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
+1 more reviewer
@lzybkr Jason Shirk (lzybkr) lzybkr left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@chunqingchen @SteveL-MSFT @daxian-dbw @lzybkr @msftclas @msftgits
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.