-
Notifications
You must be signed in to change notification settings - Fork 136
Fix#37/initial split of functions #229
Fix#37/initial split of functions #229
Conversation
Benny1007
commented
Feb 12, 2018
- Moves private functions into respective /private folder.
- Moves public functions as defined in PSModule.psd1 into respective /public folder.
- Removes all the functions from the PSModule.psm1 file.
- Dot sources the functions from the PSModule.psm1 file.
- Uses Export-ModuleMember to export the public functions from the PSModule.psm1 file.
@Benny1007 thanks a lot for contributing to the PowerShellGet repo. Please take a look at the AppVeyor build failures. |
PowerShellGet/PowerShellGet.psd1
Outdated
'Register-PSRepository', | ||
'Unregister-PSRepository', | ||
'Update-ModuleManifest') | ||
FunctionsToExport = @() |
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.
Any specific reason for removing the explicit export lists?
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.
Hi,
I have been using this method for a few years and found it to work well, during build process I normally populate the psd1 “FunctionsToExport” key but it is not required really if the .psm1 file explicitly exports them.
What I don’t understand is that with this module the “Export-ModuleMember -Function” call in the psd1 exported more members than there were in the .psd1 file?
If I look at a machine that has downloaded the module via the PowerShell gallery, I see the functions and alias’s exported by the .psd1 file not the .psm1
If I import the module from my PR, I also get the correct module members exposed:-
M:\dev\repos\PowerShellGet\PowerShellGet [fix#37/Initial-split-of-functions ≡]> import-module .\PSModule.psm1
M:\dev\repos\PowerShellGet\PowerShellGet [fix#37/Initial-split-of-functions ≡]> Get-Command -Module PSModule
CommandType Name Version Source
Function Find-Command 0.0 PSModule
Function Find-DscResource 0.0 PSModule
Function Find-Module 0.0 PSModule
Function Find-RoleCapability 0.0 PSModule
Function Find-Script 0.0 PSModule
Function Get-InstalledModule 0.0 PSModule
Function Get-InstalledScript 0.0 PSModule
Function Get-PSRepository 0.0 PSModule
Function Install-Module 0.0 PSModule
Function Install-Script 0.0 PSModule
Function New-ScriptFileInfo 0.0 PSModule
Function Publish-Module 0.0 PSModule
Function Publish-Script 0.0 PSModule
Function Register-PSRepository 0.0 PSModule
Function Save-Module 0.0 PSModule
Function Save-Script 0.0 PSModule
Function Set-PSRepository 0.0 PSModule
Function Test-ScriptFileInfo 0.0 PSModule
Function Uninstall-Module 0.0 PSModule
Function Uninstall-Script 0.0 PSModule
Function Unregister-PSRepository 0.0 PSModule
Function Update-Module 0.0 PSModule
Function Update-ModuleManifest 0.0 PSModule
Function Update-Script 0.0 PSModule
Function Update-ScriptFileInfo 0.0 PSModule
M:\dev\repos\PowerShellGet\PowerShellGet [fix#37/Initial-split-of-functions ≡]> Get-Alias | Where-Object {$_.Source -eq 'PSModule'}
CommandType Name Version Source
Alias fimo -> Find-Module 0.0 PSModule
Alias inmo -> Install-Module 0.0 PSModule
Alias pumo -> Publish-Module 0.0 PSModule
Alias upmo -> Update-Module 0.0 PSModule
Therefore I would like to understand the tests more. I believe they could be calling into the private member functions directly (and not finding them since they have moved to /private) rather than importing the module and using the Pester InModuleScope feature. Any guidance on the test setup would be appreciated 😉
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.
As I mentioned in another comment, we need to export the PackageManagement provider functions in PSModule.psm1 file, those are public and required for exposing this module as a PackageManagement provider.
Export-ModuleMember in PSModule.psm1 includes the PackageManagement provider public functions too.
Regarding FunctionsToExport and AliasesToExport in psd1 : We recommend explicitly specifying the exported members in .psd1, this also improves the performance of module analysis cache for command auto-loading.
In reply to: 168164835 [](ancestors = 168164835)
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 that's the case the exported members at the bottom of the .psm1 file were quite different to the ones in the .psd1
Export-ModuleMember -Function Find-Module, Save-Module,
Install-Module, Update-Module,
Publish-Module, Uninstall-Module,
Get-InstalledModule, Find-Command,
Find-DscResource, Find-RoleCapability,
Install-Script, Find-Script,
Save-Script, Update-Script,
Publish-Script, Get-InstalledScript,
Uninstall-Script, Test-ScriptFileInfo,
New-ScriptFileInfo, Update-ScriptFileInfo,
Get-PSRepository, Register-PSRepository,
Unregister-PSRepository, Set-PSRepository,
Find-Package, Get-PackageDependencies,
Download-Package, Install-Package,
Uninstall-Package, Get-InstalledPackage,
Remove-PackageSource, Resolve-PackageSource,
Add-PackageSource, Get-DynamicOptions,
Initialize-Provider, Get-Feature,
Get-PackageProviderName, Update-ModuleManifest
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.
PackageManagement imports the PSModule.psm1 file directly to get the provider functions, please take a look at https://github.com/PowerShell/PowerShellGet/blob/development/PowerShellGet/PowerShellGet.psd1#L46
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.
Thanks! From the failures in CI test runs, it looks like the newer version of PowerShellGet is not loaded as a PackageManagement provider. PackageManagement is loading the older version of PowerShellGet as a provider, that's tests are failing.
Did you try whether Get-PackageProvider is returning the newer version of PowerShellGet as a provider?
To check whether the newer version, with your changes, is working fine, please rename the existing versions under "$env:ProgramFiles\WindowsPowerShell\Modules\PowerShellGet"
folder on your machine and create a new version with the new set of files from your changes. Now, run Get-PackageProvider then few PowerShellGet cmdlets, like Find-Module, Install-Module, etc.
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.
C:\Users\bensh> get-module
ModuleType Version Name ExportedCommands
Binary 1.0.0.0 CimCmdlets {Export-BinaryMiLog, Get-CimAssociatedInstance, Get-CimCla...
Manifest 3.1.0.0 Microsoft.PowerShell.Management {Add-Computer, Add-Content, Checkpoint-Computer, Clear-Con...
Manifest 3.1.0.0 Microsoft.PowerShell.Utility {Add-Member, Add-Type, Clear-Variable, Compare-Object...}
Script 2.0.1 nvm {Get-NodeInstallLocation, Get-NodeVersions, Install-NodeVe...
Binary 1.0.0.1 PackageManagement {Find-Package, Find-PackageProvider, Get-Package, Get-Pack...
Script 4.1.1 Pester {Add-AssertionOperator, AfterAll, AfterEach, AfterEachFeat..
With the existing system's PowerShellGet module removed from the directory a Get-Module lists the above. PackageManagement is listed as 1.0.0.1 and in the module dependency in the .psd1 it specifies:-
RequiredModules = @(@{ModuleName='PackageManagement';ModuleVersion='1.1.7.0'})
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.
C:\Program Files\WindowsPowerShell\Modules> import-module .\PowerShellGet
import-module : The required module 'PackageManagement' with version '1.1.7.0' is not loaded. Load the module or
remove the module from 'RequiredModules' in the file 'C:\Program
Files\WindowsPowerShell\Modules\PowerShellGet\PowerShellGet.psd1'.
At line:1 char:1
- import-module .\PowerShellGet
-
+ CategoryInfo : ResourceUnavailable: (C:\Program File...erShellGet.psd1:String) [Import-Module], Missing MemberException + FullyQualifiedErrorId : Modules_InvalidManifest,Microsoft.PowerShell.Commands.ImportModuleCommand
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.
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.
As part of the CI tests, we install the latest version of PackageManagement module from the PSGallery --> https://github.com/PowerShell/PowerShellGet/blob/development/tools/build.psm1#L190
Please install the latest version of PackageManagement module on your machine.
Install-Package -Provider NuGet -Source https://powershellgallery.com/api/v2 -name PackageManagement -Destination "$env:ProgramFiles\WindowsPowerShell\Modules\" -Force -ExcludeVersion
…d code in the .psm1 to load these files.
@Benny1007 Please bump up the ModuleVersion in PowerShellGet.psd1. This also helps us in validating the newer version being loaded during the CI tests runs. |
@Benny1007 It looks like AppVeyor VM is installed with the DockerMsftProvider module using PowerShellGet cmdlet. After investigation, I found that a test for Update-Module (without any parameter values) prompted for updating DockerMsftProvider to the latest version as the PSGallery repository's default installation policy is untrusted. Please update the Line# 266 in build.psm1 to include the DockerMsftProvider module to resolve the timeout issue in AppVeyor test runs. https://github.com/PowerShell/PowerShellGet/blob/development/tools/build.psm1#L266 Get-InstalledModule -Name Pester,PackageManagement,DockerMsftProvider -ErrorAction SilentlyContinue | Foreach-Object { |
Something has changed in the PowerShellEdition=Core tests this was working a few commits back.
@Benny1007<https://github.com/benny1007> It looks like AppVeyor VM is installed with the DockerMsftProvider module using PowerShellGet cmdlet. After investigation, I found that a test for Update-Module (without any parameter values) prompted for updating DockerMsftProvider to the latest version as the PSGallery repository's default installation policy is untrusted.
Please update the Line# 266 in build.psm1 to include the DockerMsftProvider module to resolve the timeout issue in AppVeyor test runs.
https://github.com/PowerShell/PowerShellGet/blob/development/tools/build.psm1#L266
Get-InstalledModule -Name Pester,PackageManagement,DockerMsftProvider -ErrorAction SilentlyContinue | Foreach-Object {
|
@Benny1007 I have submitted https://github.com/PowerShell/PowerShellGet/pull/235 to resolve the two issues in CI test runs, 1) PSCore installation issue due to the recent changes in PS/PS repo, and 2) DockerMsftProvider module in AppVeyor VMs. After merging this PR, I will restart the CI test runs for this PR. |
@Benny1007 Get-Help is not working with this PR changes. PS C:\WINDOWS\system32> Update-Help PowerShellGet -verbose -Force
VERBOSE: Resolving URI: "https://go.microsoft.com/fwlink/?linkid=855963"
VERBOSE: Your connection has been redirected to the following URI: "https://pshelp.blob.core.windows.net/powershell/help/6/PowerShellGet/"
VERBOSE: Performing the operation "Update-Help" on target "PowerShellGet, Current Version: 6.0, Available Version: 6.0, UICulture: en-US".
VERBOSE: PowerShellGet: Updated C:\Program Files\WindowsPowerShell\Modules\PowerShellGet\1.6.1\en-US\PSModule-help.xml. Culture en-US Version 6.0
PS C:\WINDOWS\system32> Get-Help Find-Module -verbose
NAME
Find-Module
SYNTAX
Find-Module [[-Name] <string[]>] [-MinimumVersion <string>] [-MaximumVersion <string>] [-RequiredVersion <string>] [-AllVersions] [-IncludeDependencies] [-Filter <string>] [-Tag <string[]>] [-Includes {DscResource | Cmdlet | Function | RoleCapability}]
[-DscResource <string[]>] [-RoleCapability <string[]>] [-Command <string[]>] [-Proxy <uri>] [-ProxyCredential <pscredential>] [-Repository <string[]>] [-Credential <pscredential>] [-AllowPrerelease] [<CommonParameters>]
ALIASES
fimo
REMARKS
Get-Help cannot find the Help files for this cmdlet on this computer. It is displaying only partial help.
-- To download and install Help files for the module that includes this cmdlet, use Update-Help.
-- To view the Help topic for this cmdlet online, type: "Get-Help Find-Module -Online" or
go to https://go.microsoft.com/fwlink/?LinkID=398574. |
@bmanikm are you sure that is due to the refactoring? Where are these help files defined? I can't get to that url in the verbose output. How do these help files get referenced? The fact that Find-Module is loaded from the module and displayed means the refactoring into the separate folders is working and all the tests have passed etc. |
Please use below steps to reproduce the get-help issue. # Run as admin
Update-Help PowerShellGet -verbose -Force
Get-Help Find-Module -verbose |
@bmanikm the command works and doesn't report an error:- C:\windows\system32> update-help powershellget -verbose -force but yes it's missing extended info, how is this help constructed? Really I don't see how it could be affected with this PR, but nothing surprises me with this module. |
If the language specific module help is moved inside the /public folder the help is available so it looks like it has to be adjacent to the files which seems contrary to the advice here:- https://msdn.microsoft.com/en-us/library/dd878343(v=vs.85).aspx#Placement of Module Help and here:- Maybe raise case with MS? Splitting the functions out to separate files is the only way to keep this repo maintainable imo. Up to you how you want to play it. |
Removing this PR version of the module and installing 1.6.0 again. I still can't get the help to load. Windows PowerShell Switched to node version v8.9.4 NAME SYNTAX ALIASES REMARKS C:\windows\system32> get-module ModuleType Version Name ExportedCommands Binary 1.0.0.0 CimCmdlets {Export-BinaryMiLog, Get-CimAssociatedInstance, Get-CimClass, Get-CimInstance...} C:\windows\system32> gci 'C:\Program Files\WindowsPowerShell\Modules\PowerShellGet\1.6.0'
Mode LastWriteTime Length Name d----- 5/03/2018 9:30 PM en-US C:\windows\system32> |
@Benny1007 Yes, Update-Help issue is a known issue in the 1.6.0 version of PowerShellGet module on PSGallery. This issue got resolved with PR #215. Please note that I would like to take this PR so that community can contribute this repo, that's why I am providing the inputs, and also ensuring that some of the known scenarios are working fine. After further investigation, I found a solution for fixing the get-help scenario :). This is happening due to the new folder structure we have created for the public functions. We can fix this issue by below provided resolution. I have also tried on my local machine with this change, Get-Help is working fine. Current: <#
.ExternalHelp PSModule-help.xml
#> Resolution: <#
.ExternalHelp ..\PSModule-help.xml
#> |
I think it goes against best practices to ship a module that is a psm1 which dot-sources many files. That is, if you "compile" it into a single PSM1 for shipping, then I love this -- breaking it apart can make it a lot easier to work with as developers. However, dot-sourcing a bunch of files at run-time has some significant downsides.
Particularly on systems with slow disc IO, slow networks, or no access to the internet, having many files has a measurable impact on module load time. |
@Jaykul which best practice would this be? (Genuinely interested to know and read up). Although I am not sure how no access to the internet or slow disc IO would affect loading 80 odd files? |
@bmanikm I have updated the PR with the change to the ExternalHelp. It's a shame we can't use something like:-
in the psm1 and then just reference that (.ExternalHelp $ExternalHelpfile) but I tried and the variable is not expanded in the comment tags. With regard to the approach of creating the psm1 file at build time and incorporating all the private and public functions into it, I do like that however it needs to carefully evaluated for this repo given the amount of extra content in the .psm1 file in this situation, I am sure @Jaykul would agree in context of this. Anyway some good discussion by @Jaykul and others are in the following issues:- I think small steps first to untangle it a bit (split of the functions out to aid development), then moving forward addressing the other complexity within the psm1 would be a seem a good approach. |
@Benny1007 my personal solution for building files has not been modularized yet, but I updated the copy of Optimize-Module on gist if you want to see an example. The short answer to "extra content in the psm1" for me is that I create a file like |
@Benny1007 It looks like your change for updating the ExternalHelp file path is not yet pushed to this PR. |
@bmanikm oops, have just pushed the changes. About the single psm1 file if you read those comments in the PR's it's still a matter of opinion. Besides the script signing load time @Jaykul doesn't really provide any other argument, and I have read comments in other threads where it confused contributors. I wouldn't be able to get to this until after the weekend if you want it in this PR (I think it should be a separate build PR 'modify build to output to collate one distributable .psm1 file') , by your comment it would seem you would modify the build file to accommodate? |
…m/Benny1007/PowerShellGet into fix#37/Initial-split-of-functions
Load time performance with this change and without any script signingPS C:\> powershell -Command {Measure-Command {Import-Module PowerShellGet; Get-PackageProvider PowerShellGet}}
TotalSeconds : 13.5587634 Load time performance with single .psm1 file and without any script signingPS C:\> powershell -Command {Measure-Command {Import-Module PowerShellGet; Get-PackageProvider PowerShellGet}}
TotalSeconds : 2.9640948 We definitely need a separate PR for merging the content of multiple .ps1 files into a single .psm1 file during the build time. If there are no concerns, we will merge this PR. |