Enable import-module to be case insensitive #5097

SteveL-MSFT · 2017-10-12T19:45:13Z

Apparently macOS is case insensitive

iSazonov · 2017-10-13T14:12:30Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

If it is only for Linux maybe use "#if UNIX"?

it's technically any filesystem that's case sensitive and technically (although unlikely), it seems you can make NTFS case sensitive. for practical cases, it's probably fine to #ifdef this and if a customer has an issue on Windows, we can revisit

fixed and added comment

adityapatwardhan · 2017-10-13T17:13:08Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

The cyclomatic complexity of this function is pretty high. Can this be refactored into a smaller function?

I could simplify it a bit if we can make the assumption that the psd1 or psm1 file always has the same casing as the folder name. It's not clear to me if that is a safe assumption or not. The current code is conservative in that you could have: "...\Modules\FOO\foo.psd1"

I'll check with the PSGet folks

per PSGet, it is safe to assume the folder and file names have the same casing. So I'll simplify this code.

adityapatwardhan · 2017-10-13T17:30:11Z

test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1

Please redirect to null.
New-Item -ItemType Directory -Path "$modulesPath/$modulePath" -Force > $null

adityapatwardhan · 2017-10-13T17:32:24Z

test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1

Maybe -Force to ensure you are not loading a cached version.

actually, as part of this test, I'm removing the module with a different casing, so I don't want to -force loading

adityapatwardhan · 2017-10-13T17:33:06Z

test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1

adityapatwardhan · 2017-10-13T17:33:33Z

test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1

SteveL-MSFT · 2017-10-13T18:32:48Z

@adityapatwardhan feedback addressed

iSazonov · 2017-10-13T18:53:02Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

In line 294 the same cycle? Can we remove it and replace with the new one on all platforms?

We still need both as the first one will find an exact match and fall through to this if it doesn't find it. Also the above loop also handles a file path to the module which I think should be case sensitive since it's a literal file path. The lower code only handles the module name which we can treat as case insensitive. The code is similar, but I don't think it's worth combining which will make it more complex.

Why we should use different logic for module folder names and module file names?

If someone uses: Import-module /users/steve/dev/MyModule/MyModule.psd1 then that should be treated as a case-sensitive path. If they use Import-module mymodule, then that's case-insensitive as that's a module name and not a filepath.

Ah, I meant folder and file names not paths.

I don't see an easy way to have a single loop between the two without incurring a cost of enumerating the folders for the purpose of case-insensitive comparison in the "normal" case where either the case matches or the filesystem is case-insensitive. If there's two modules "Foo" and "foo", and the user does import-module foo we want the first loop to match foo and not Foo as that is an exact match.

We have the same problem in #4699 - can you review the idea to use here?

In the tab completion case, I think you have to cover more cases as ab might match ABc, abc, or aBcdefg, etc... so creating a case-insenstive match pattern makes sense. In this case, we have a specific name (wildcard is not accepted) and just need a case-insensitive match if we haven't already matched the exact casing (which is preferred). And we don't need to return all matches, the first one wins.

If we want just the first match to win even if there is an exact match, it would be easy to merge the two loops. However, I think we want to get an exact match if it exists, then fallback on case-sensitive search. For example, if the user typed ipmo AAA which exists in the last modulepath, but aaa exists in the first, I would have to enumerate all module folder names in every modulepath part every time just to make sure AAA wasn't found after my first case-insensitive match.

We could have solved this situation on the other hand. Do we want all the modules to be portable? If so, we must document this and exclude the case-sensitivity in this code.

That's a good point on portable scripts being able to rely on module names being case insensitive. It would make sense to just say module names are case insensitive everywhere and have it enforced by install-module and publish-module.

iSazonov · 2017-10-16T11:56:50Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

Perhaps we could exclude #if here and leave one line.
Or maybe create helper functions - It will be easier to read and understand.

I think this line isn't needed as I have qualifiedPath = folder above, was probably thinking this, but didn't put it together. Will fix.

iSazonov · 2017-10-17T03:42:03Z

Should we add negative tests too (to reflect the situations we have decided to exclude)? What errors should appear (throw)?
Also why we skip Windows in new test? If we want to have all modules portable all test should run on all platforms identically.

SteveL-MSFT · 2017-10-17T05:21:45Z

@iSazonov test was originally skipped due to the previous code for an exact match since it wasn't possible to create two modules that differ only by casing on Windows. With the new change, no reason not to run it on Windows. I also noticed a bug in the test where I wasn't removing the test module between runs.

iSazonov · 2017-10-17T06:55:43Z

I don't see still that tests reflect to "StringComparison.OrdinalIgnoreCase".

SteveL-MSFT · 2017-10-17T15:42:12Z

@iSazonov the new test creates a module called TESTMODULE and imports it using testMODULE, this is where the case-insensitive comparison occurs. In the UNIX code path, if it enumerates the module folders in the module path and sees if it matches the name of the given module. Not clear to me if you were expecting a different test?

iSazonov · 2017-10-17T17:54:24Z

@SteveL-MSFT My thoughts was that we should do `Import-Module testMODULE` and `Import-Module TESTMODULE` and check that we load the same module. Although I'm not sure it's necessary.

SteveL-MSFT · 2017-10-17T20:37:28Z

@iSazonov I don't think that's necessary

simplify search loops with assumption that module file casing matches module folder name which is appropriate per PSGet

combine loops

removed test that is no longer valid

enable test for Windows, removed invalid test where a module with version folder requires a manifest

adityapatwardhan

LGTM

adityapatwardhan · 2017-10-19T16:05:52Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

+                            {
+                                qualifiedPath = Path.Combine(qualifiedPath, fileBaseName);
+                            }
+                            else if (Directory.Exists(qualifiedPath))


Please use Utils.NativeDirectoryExists()
https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/Utils.cs#L912

Will change

adityapatwardhan · 2017-10-19T16:06:33Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

-
-                if (!found)
+#if UNIX
+                foreach (string folder in Directory.EnumerateDirectories(path))


Please use Utils.NativeEnumerateDirectory()
https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/Utils.cs#L926

Will change

This method appears to be only implemented for Win32. Since this path is only for UNIX, I'll leave it as-is.

adityapatwardhan · 2017-10-19T16:07:58Z

test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1

changed to using Utils.NativeDirectoryExists() for perf reasons

SteveL-MSFT assigned adityapatwardhan Oct 12, 2017

SteveL-MSFT requested review from anmenaga and chunqingchen October 12, 2017 19:45

SteveL-MSFT requested review from BrucePay, daxian-dbw, lzybkr and vors as code owners October 12, 2017 19:45

SteveL-MSFT force-pushed the import-module-casing branch from fe348b9 to 31926ea Compare October 13, 2017 00:03

iSazonov reviewed Oct 13, 2017

View reviewed changes

adityapatwardhan requested changes Oct 13, 2017

View reviewed changes

iSazonov reviewed Oct 13, 2017

View reviewed changes

iSazonov reviewed Oct 16, 2017

View reviewed changes

SteveL-MSFT force-pushed the import-module-casing branch 2 times, most recently from d00ab71 to 43e45cf Compare October 16, 2017 18:24

SteveL-MSFT added 5 commits October 17, 2017 14:15

enable import-module to be case insensitive

247b0db

address PR feedback

6077272

simplify search loops with assumption that module file casing matches module folder name which is appropriate per PSGet

[feature]

fac1155

combine loops

[feature]

b4f7c01

removed test that is no longer valid

[feature]

fd7dcfb

enable test for Windows, removed invalid test where a module with version folder requires a manifest

SteveL-MSFT force-pushed the import-module-casing branch from cefbc40 to fd7dcfb Compare October 17, 2017 21:32

iSazonov approved these changes Oct 18, 2017

View reviewed changes

adityapatwardhan approved these changes Oct 18, 2017

View reviewed changes

adityapatwardhan requested changes Oct 19, 2017

View reviewed changes

[feature]

169e597

changed to using Utils.NativeDirectoryExists() for perf reasons

adityapatwardhan approved these changes Oct 19, 2017

View reviewed changes

adityapatwardhan merged commit e908b8a into PowerShell:master Oct 19, 2017

SteveL-MSFT deleted the import-module-casing branch November 12, 2017 05:17

4ndri mentioned this pull request May 14, 2022

Import-Module not working if psd1 and module folder has different casing (on unix) #17342

Closed

5 tasks

Enable import-module to be case insensitive #5097

Enable import-module to be case insensitive #5097

Uh oh!

Conversation

SteveL-MSFT commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov Oct 13, 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!

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!

SteveL-MSFT commented Oct 13, 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!

iSazonov Oct 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!

iSazonov Oct 16, 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!

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

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 17, 2017

Uh oh!

iSazonov commented Oct 17, 2017

Uh oh!

SteveL-MSFT commented Oct 12, 2017 •

edited

Loading

iSazonov Oct 13, 2017 •

edited

Loading

iSazonov Oct 15, 2017 •

edited

Loading

iSazonov Oct 16, 2017 •

edited

Loading

iSazonov commented Oct 17, 2017 •

edited

Loading