Add more Extra variables to be allowed to be referenced in module manifest

[fMichaleczek]

PR Summary

Module Manifest can contain the following special variables : PSScriptRoot, PSEdition and EnabledExperimentalFeatures.

The aim of this PR is to extend the allowed variable list to :

• PSHome
• PSCulture,
• PSUICulture,
• IsCoreCLR,
• IsLinux,
• IsMacOS,
• IsWindow

PR Context

As a PowerShell developper, I deal with cross platform and cross language, to allow more special variables let me handle more heterogeneous within the module manifest.

This PR is a followup of

• Allow automatic variable Is* inside PowerShell module manifest file
• Current module manifest schema is not adequate to properly support cross-platform modules

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • [] Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed

[msftclas]

All CLA requirements met.

[fMichaleczek]

@PoshChan Please rerun windows

[PoshChan]

@fMichaleczek, you are not authorized to request a rebuild

[vexx32]

@fMichaleczek seems to be a widespread issue with Azure's windows agents. Nothing to do but wait on that one, I'm afraid. 🙂

[daxian-dbw]

Close and reopen the PR to force Windows CI build working properly.

[fMichaleczek]

Summary

Current

Environment Variables Allow : yes

Allowed PS Variables : PSCulture, PSUICulture, true, false, null, PSScriptRoot, PSEdition, EnabledExperimentalFeatures

Permitted Cmdlets :

• Import-LocalizedData
• ConvertFrom-StringData
• Write-Host
• Out-Host
• Join-Path

Change

Environment Variables :

• Home => Let import a dll across nuget repository "$Home/.nuget/packages/htmlagilitypack/1.8.9/lib/netstandard2.0/htmlagilitypack.dll"
• PSHome => I prefer to let it by consistency as env:, psscriptroot and home are allowed.
• PSCulture => already allowed by default.
• PSUICulture => already allowed by default.
• IsCoreCLR => To remove : not usefull in our context.
• IsLinux
• IsMacOS
• IsWindow

Improvement that need feedback

these cmdlets are good candidates :

• Test-Path
• Split-Path => If I can join a path, I should be able to split a path
• Resolve-Path
• Convert-Path

And maybe the Write subset :

• Write-Information
• Write-Verbose
• Write-Warning
• Write-Error
• Write-Debug

Any feedback are welcome, I can create a new issue, modify in this PR or make a new one.

[KirkMunro]

To answer your question about what cmdlets to add, I'm going share details about something related that I have planned.

I have a PR planned that I want to submit once I get some of the other debugging work I have ongoing finished is to prevent the debugger from stepping into a module manifest by default (i.e. treat them as if they have the DebuggerHidden attribute set on them -- something that you cannot do manually to a manifest at this time because it does not support the param statement).

The reason:

I'm trying to improve debugging efficiency in PowerShell, and if I am stepping through code that invokes a command from a module that is not loaded, and I want to step into that command, I end up stepping into the manifest, and it can take a very long time to go through that process. I find debugging a manifest to be pretty useless because it's not really code (there really isn't much to see/do here).

This PR adds a bunch more variables and some additional commands to the manifest, but in my mind it's still not enough to warrant debugging.

I think that this approach (that a manifest is not cannot be debugged) is a good measure for whether or not additional commands should be added to the manifest. The more we add, the more we open the opportunity for wanting to debug it, but it shouldn't be "code" per se...it should just be module metadata specified in a PowerShell-ish way.

With that in mind, I would avoid adding support for any commands other than those that we deem absolutely necessary in a manifest.

[fMichaleczek]

@KirkMunro
Ok, I don't changed Permitted Cmdlets and will make an issue if it's necessary.

For our context, this code is valid with PS5+

Set-Content -Path .\Test5-Dependencies.psm1 -Value @'
    $env:IsLinux = $IsLinux -eq $true
    $env:IsMacOS = $IsMacOS -eq $true
    $env:IsWindows = (-not $IsLinux -or -not $IsMacOS) -and -not [System.Environment]::Is64BitProcess
    $env:IsWindows64 = (-not $IsLinux -or -not $IsMacOS) -and [System.Environment]::Is64BitProcess
'@

Set-Content -Path .\Test5.psm1 -Value @'
    Remove-Module Test5-Dependencies
'@

Set-Content -Path .\Test5.psd1 -Value @'
@{
    RootModule = "Test5.psm1"
    ModuleVersion = '0.1.0'
    NestedModules = "Test5-Dependencies.psm1"
    RequiredAssemblies   = @(
        if($env:IsLinux -eq 'true') {
           Write-Host "IsLinux" -Foreground Green
        }
        elseif($env:IsMacOS -eq 'true') {
            Write-Host "IsMacOS" -Foreground Green
        }
        elseif($env:IsWindows64 -eq 'true') {
            Write-Host "IsWindows64" -Foreground Green
        }
        elseif($env:IsWindows -eq 'true') {
            Write-Host "IsWindows" -Foreground Green
        }
    )
}
'@
Import-Module .\test5.psd1 -Force
Get-Module test5*

After this change, this code is valid with PS7+

Set-Content -Path .\test7.psd1 -Value @'
@{
    ModuleVersion = '0.1.0'
    RequiredAssemblies   = @(
        if($IsLinux) {
           Write-Host "IsLinux" -Foreground Green
        }
        elseif($IsMacOS) {
            Write-Host "IsMacOS" -Foreground Green
        }
        elseif($IsWindows) {
            Write-Host "IsWindows" -Foreground Green
        }
    )
}
'@
Import-Module .\test7.psd1
Get-Module test7

So this PR is a beginning to handle 3 cases : Linux/MacOS/Windows but doesn't resolve all the problems like compatibility with 5.1 and native dll loading problems.

[fMichaleczek]

It's possible to restart the CI ? The macos failed for an external reason.
In this kind of problem, do you prefer I ask or I submit a new commit ?

[TravisEz13]

@fMichaleczek I restarted the PR but it does not look like external reasons.

[vexx32]

@PoshChan please get failures

[PoshChan]

@vexx32, test results were not published for PowerShell-CI-linux at vstfs:///Build/Build/25574

[PoshChan]

@vexx32, your last commit had 23 failures in PowerShell-CI-macos
(These are 5 of the failures)

float/double precision when converting to string.[double]-to-[string] conversion in PowerShell should use the precision specifier G15

Expected strings to be the same, but they were different.
Expected length: 3
Actual length:   18
Strings differ at index 3.
Expected: '3.3'
But was:  '3.3000000000000003'
at <ScriptBlock>, /Users/vsts/agent/2.153.2/work/1/s/test/powershell/Language/Parser/Conversions.Tests.ps1: line 540
540:         $value -as [string] | Should -BeExactly $StringConversionResult

float/double precision when converting to string.[double]-to-[string] conversion in PowerShell should use the precision specifier G15

Expected strings to be the same, but they were different.
Expected length: 3
Actual length:   18
Strings differ at index 3.
Expected: '6.6'
But was:  '6.6000000000000005'
at <ScriptBlock>, /Users/vsts/agent/2.153.2/work/1/s/test/powershell/Language/Parser/Conversions.Tests.ps1: line 540
540:         $value -as [string] | Should -BeExactly $StringConversionResult

float/double precision when converting to string.[double]-to-[string] conversion in PowerShell should use the precision specifier G15

Expected strings to be the same, but they were different.
Expected length: 16
Actual length:   17
Strings differ at index 15.
Expected: '2.71828182845905'
But was:  '2.718281828459045'
at <ScriptBlock>, /Users/vsts/agent/2.153.2/work/1/s/test/powershell/Language/Parser/Conversions.Tests.ps1: line 540
540:         $value -as [string] | Should -BeExactly $StringConversionResult

float/double precision when converting to string.[double]-to-[string] conversion in PowerShell should use the precision specifier G15

Expected strings to be the same, but they were different.
Expected length: 16
Actual length:   17
Strings differ at index 16.
Expected: '3.14159265358979'
But was:  '3.141592653589793'
at <ScriptBlock>, /Users/vsts/agent/2.153.2/work/1/s/test/powershell/Language/Parser/Conversions.Tests.ps1: line 540
540:         $value -as [string] | Should -BeExactly $StringConversionResult

float/double precision when converting to string.[float]-to-[string] conversion in PowerShell should use the precision specifier G7

Expected strings to be the same, but they were different.
Expected length: 3
Actual length:   9
Strings differ at index 3.
Expected: '3.3'
But was:  '3.3000002'
at <ScriptBlock>, /Users/vsts/agent/2.153.2/work/1/s/test/powershell/Language/Parser/Conversions.Tests.ps1: line 540
540:         $value -as [string] | Should -BeExactly $StringConversionResult

[PoshChan]

@vexx32, test results were not published for PowerShell-CI-static-analysis at vstfs:///Build/Build/25572

[PoshChan]

@vexx32, test results were not published for PowerShell-CI-windows at vstfs:///Build/Build/25575

[vexx32]

@fMichaleczek you might try rebasing off of the current master branch here, by the looks of those.

[iSazonov]

I do not like this trend - after some time this file will turn into a script. I would prefer to keep the declarative nature of this file, which was originally meant in manifest design. Perhaps PowerShell/PowerShell-RFC#188 could address this directly or indirectly.

[daxian-dbw]

I second @iSazonov's opinion. I don't see the need to expand the allowed cmdlets. As for the variables, it makes sense to add $IsWindows, $IsLinux, $IsMacOS, but I don't see why a module manifest cares $PSHome.

[iSazonov]

We have never even discussed alternatives to these variables in manifest file. The direction in RFC 188 seems more correct.
If the main intention is to make ported modules, then I would even say that there should not be anything related with the OS type in the manifest. The engine should be so smart to do the rough work for us.
.Net Core allows us to control how native assemblies to load and we could make enhancements to benefit from this. For the rest we could maintain simplicity in the manifest and file structure.

[daxian-dbw]

The engine should be so smart to do the rough work for us.

There is a possibility that a module want to expose certain functions/cmdlets only on certain platforms, not all. In that case, $IsWindows, $IsLinux and $IsMacOS could be useful.

[KirkMunro]

The engine should be so smart to do the rough work for us.

There is a possibility that a module want to expose certain functions/cmdlets only on certain platforms, not all. In that case, $IsWindows, $IsLinux and $IsMacOS could be useful.

Indeed, I have one module that comes to mind where I need to do this.

[iSazonov]

There is a possibility that a module want to expose certain functions/cmdlets only on certain platforms, not all. In that case, $IsWindows, $IsLinux and $IsMacOS could be useful.

My thoughts is that never discuss alternatives publicly and that preferred way is declarative, not script, manifests.

Currently in the repo we have to use #if !UNIX to exclude code from compilation and a manifest per platform. It seems there is no better way in the case - we can not expose a cmdlet which is not compile. If a cmdlet compiled but can not be used on a platform (or we don't want to expose it), for the case (it is true for script cmdlet too) we could use an attribute (new parameter in existing attributes) to inform engine to load the cmdlet/function/alias only on platform specified in the attribute. (Verbose message could inform users that the cmdlet is not compatible.)
Or we could have platform sections in manifests like:

Windows {
    CmdletsToExport = @()
}

[KirkMunro]

Or we could have platform sections in manifests like:

Windows {
    CmdletsToExport = @()
}

Interesting. My original thinking was if that was implemented to be additive, such that the platform-specific properties are combined with the platform-agnostic properties in the manifest, I'd like it much better than having conditional logic here and there. It would also make it easier to view as a property file, which is its intent, and it would be easier to author correctly as well since it would be more about defining properties and less about conditional logic.

There is a problem though. What do you do for metadata that you want to define for multiple platforms, but not all platforms? Do you define it once for each platform, resulting in duplication? That's not great. So how could we specify properties for multiple platforms this way? That's where conditional logic comes in handy, because then I can do this (contrived, assumes that we get support for Out-Printer on macos but not on Linux):

# ...
CmdletsToExport = @(
    'Out-String'
    'Out-File'
    'Out-Host'
    'Out-Null'
    if ($IsWindows -or $IsMacOS) {'Out-Printer'}
)
# ...

Since we already support conditional logic in manifest, I think this is probably the easiest path forward to success.

That said, I think we need to be minimalistic and only add the bare minimum variables that we know would be useful, i.e. $IsWindows, $IsLinux, and $IsMacOS. Others can be added in separate PRs if there is a driving need for them.

I would also drop $PSHome from this PR, because module authors shouldn't be doing anything directly with that path, and if they do have that need, that may point to issues that should be addressed in a different manner.

[iSazonov]

CmdletsToExport = @() # For all platforms
RequiredAssemblies = @()
Windows {
    CmdletsToExport = @() # Additions for Windows platform
    RequiredAssemblies = @()
}

If we want more features we could consider definitions like runtime.config.json.

[fMichaleczek]

If a cmdlet is compatible with Linux x64 and not with Linux arm, this PR doesn't help. The only workaround is to test if a path exists. At a moment, the RID should be available in the system (I understand it's not a powershell's priority)
I will update the PR before the end of the week.

[anmenaga]

Restarted CI-macos.

[KirkMunro]

This seems to have stalled.

Unresolved discussion/decision points:

• $PSHome and $Home -- should those be added?

  Recommendation: Unless there is a specific use case that these are needed for right now, I would err on the conservative side and keep it simple for now.

  Rationale: Those can be added later should the need arise.

• $IsWindows, $IsLinux, and $IsmacOS vs a more declaritive manifest style that doesn't use variables?

  Recommendation: Taking the variable approach for now solves the immediate need, so go with that to allow other modules such as Microsoft.PowerShell.GraphicalTools to start using it.

  Rationale: Making manifests more declarative and less programmatic is worthy of a discussion, but that discussion should not get in the way of solving the immediate need. As long as we keep the list of variables and commands very small, we should be good for now. Also, as has been demonstrated on other discussion threads, the bar is very high for manifest format changes, and that's without introducing new syntax/keywords.

If we can agree with those recommendations, let's remove $Home and $PSHome from this PR and set it up to be merged in.

If someone disagrees with the removal of $Home and $PSHome because those are needed for a scenario right now, or if we really need to make this more complicated by discussing bigger manifest changes today, please chime in.

[TravisEz13]

cc @TylerLeonhardt Do you have an opinion here?

[TylerLeonhardt]

I agree with Kirk that it would be nice to have $Is* variables to fill the immediate need of helping with crossplat modules that have different native libraries for each platform.

That said, what are we going to do when we need to pull in native libs that only work on linux-arm or some other specific runtime?

In those cases, $Is* is not enough.

What we'll need to truly help cross-platform module dev is a change in PowerShellGet, the PowerShell Gallery, and PowerShell itself to handle bits that are specific to a runtime. That will be a lengthy change and something that's not coming in PSGet 3.0... So in the meantime, I'm in favor of allowing $Is* because of the utility and easy of use.

[anmenaga]

Personally, i agree with @KirkMunro - for this PR - add just $IsWindows, $IsLinux, and $IsMacOS.
But I would like this to get reviewed by PS Committee to ensure that allowing new vars in manifests won't backfire in the future.

[SteveL-MSFT]

@PowerShell/powershell-committee reviewed this and have concerns about usage that would limit cross-PowerShell modules supporting older versions of PowerShell. Also, there have been other asks for changes in module manifest, so if we were to make such a breaking change, we should it holistically rather than just this aspect. Although having these variables in the module manifest will make it simpler, you can replicate the same capability in a psm1 file while maintaining downlevel compatibility.

[TylerLeonhardt]

@SteveL-MSFT an important scenario here is on install time.

By allowing the $Is* variables, you give the ability for a module to require different dependencies for different platforms and only install the needed ones.

This is impossible to replicate by putting it in a psm1 because installing required dependencies would then be forced at import time instead of install time. Then the initial import will be forced to fetch dependencies (which will be harder to install in the correct scope)

I urge @PowerShell/powershell-committee to reconsider as this is a very important scenario, especially for the future of xplat modules.

[vexx32]

I'd also like to point out that such a feature is entirely opt-in. So any module author using the feature should be well aware that its use restricts their module to v7+

I don't see a reason to raise compatibility concerns over an explicitly opt-in feature? 🤔

[iSazonov]

By allowing the $Is* variables, you give the ability for a module to require different dependencies for different platforms and only install the needed ones.

@TylerLeonhardt What is the dependencies? Can you share examples?

In #11032 we are trying to address native dll resolving by using a folder tree. Perhaps we could enhance this on other scenarios.
I'd prefer to do not delegate the problem to users and get solution OOB in engine.

[TylerLeonhardt]

I'm specifically talking about module dependencies.

Look at this hack that we had to do in PoshNotify. That should not be the behavior we ask our customers to depend on if they want to write xplat models with different dependencies.

Not only that, but the first import is slow having to install dependencies which is sad considering the module is about doing something in real time (notifications).

I also think about the GraphicalTools module. We could probably leverage this feature (thus pulling down less junk) which will bring the size down drastically. That module only supports PowerShell 6+ so I would definitely opt-in to this behavior if given the opportunity.

#11032 would not fix either of these because it's at the install time that I care about.

[KirkMunro]

Related: #10958 (comment).

And: #10251 (comment).

Y'all seem to want to keep manifests stagnant, with a focus on legacy support, but history has shown that manifest changes do happen over time and are necessary. The changes that have happened over the past few years were not anticipated, but they still happened. Stop being the blocker for progress in this area so much, please, so that we can be forward thinking and move forward with better solutions. Otherwise we're going to be looking back again thinking gee, wouldn't it have been great if we fixed that problem before.

Also related, although in the other direction (this one is about backwards compatibility, and is an issue because not enough thought was put into manifest changes over time in the past): #10931.

[iSazonov]

#11032 would not fix either of these because it's at the install time that I care about.

@TylerLeonhardt My proposal is to put all in one nuget using subfolders named as portable RID - PowerShell could be load right components in runtime based on current environment.

notify.psd1
    |
    |--- linux-x64
    |        |
    |        |--- notify.psm1, notify.so
    |
    |--- osx-x64
    |        |
    |        |--- notify.psm1, notify.dylib

It looks so simple that we could add this in days before 7.0 GA.

[vexx32]

@iSazonov that still has a significant disadvantage of requiring components for all platforms to be present in the nupkg, whereas if we have the ability to do an install-time check, we can have PSGet only pull dependencies for the platform required. 🙂

This is typically relatively minor, but for modules like GraphicalTools where the dependency is quite large, it would make a very big difference.

[iSazonov]

@vexx32 I don't see problems. You could split a project to some nugets, only keep proposed folder tree.

[RokeJulianLockhart]

@fMichaleczek, why was this closed?