Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New PSGalleryModule with support for semver/VersionFilter/Pre-Release #96

Closed

Conversation

gaelcolas
Copy link
Contributor

Backward compatible.

This should fix #68 adding Pre-release option
Fix #65 by adding a filter like: -gt 1.2 -and (-le 1.7 -or gt 2.0 )
Also supports SemVer1/2/NuGet2 versions and filter
Should fix #44 by piping the module to Ipmo -force -scope global

@RamblingCookieMonster might not be 100% for merging, but asking for feedback

This should fix RamblingCookieMonster#68 adding Pre-release option
Fix RamblingCookieMonster#65 by adding a filter like: `-gt 1.2 -and (-le 1.7 -or gt 2.0 )`
Also supports SemVer1/2/NuGet2 versions and filter
Should fix RamblingCookieMonster#44 by piping the module to Ipmo -force -scope global
@gaelcolas
Copy link
Contributor Author

I've changed and simplified the Get-SemVerFilter and added relevant tests.
The Version Filter provided in the PSDepend PSD1s need to have quotes around their versions otherwise the PowerShell parser can't get the long versions correctly (and I don't want to write a parser).
So it should be -gt "1.2" -and (-le "1.7" -or -gt "2.0.0-beta")

I think I'm now happy with this to be merged if you can confirm it's working as it should for you.

@RamblingCookieMonster
Copy link
Owner

Nice! I'll try to give this a quick review over the weekend, if all looks good will merge it in - thanks : D

@JustinGrote
Copy link

JustinGrote commented Dec 11, 2018

@gaelcolas, per #97, would recommend that if the version is pinned (not 'latest') and it already exists locally (checked with get-module), then skip the find-module step for a performance improvement.
EDIT: I misread it, looks like it should do as expected. I'll test! 👍

@JustinGrote
Copy link

JustinGrote commented Dec 11, 2018

@gaelcolas Line 174 has Get-Module -Listavailable -All. It should just be Get-Module -Listavailable.

-All returns every single module in the system (and loads all their DLLs) and it takes forever on a system with a lot of modules with no benefit.

Here's the performance testing script I'm using, run it from the directory of your "new" test module

remove-module psdepend -erroraction SilentlyContinue
$oldmodule = (get-module psdepend -listavailable | sort version -Descending)[0]
$psdependparams = @{
    InputObject = @{
        PSDependOptions = @{
            Target = 'CurrentUser'
        }
        Pester                          = @{
            Version     = '4.4.2'
            Parameters  = @{
                SkipPublisherCheck = $true
            }
        }
        PowershellGet                   = @{
            Version     = '2.0.3'
            Parameters  = @{
                SkipPublisherCheck = $true
            }
        }
        BuildHelpers                    = '2.0.1'
        'powershell-yaml'               = '0.3.7'
        'Microsoft.Powershell.Archive'  = '1.2.2.0'
        PSScriptAnalyzer                = '1.17.1'
        Plaster                         = '1.1.3'
    }
    Install = $true
    Verbose = $true
}

$confirmpreference = 'none'

#Warmup
$m = ipmo $oldmodule -force -passthru
$cmd = get-command -module $m Invoke-psdepend
& $cmd @psdependparams
remove-module $m

#Test Old Module
$om = ipmo $oldmodule -force -PassThru -erroraction stop
$cmdold = get-command -module $om Invoke-psdepend
$oldtime = measure-command {& $cmdold @psdependparams} | % totalseconds
remove-module $om

#Test New Module (should be faster)
$nm = ipmo .\PSDepend.psd1 -force -PassThru -erroraction stop
$cmdnew = get-command -module $nm Invoke-psdepend
$newtime = measure-command {& $cmdnew @psdependparams} | % totalseconds
remove-module $nm

write-host -fore green "$($om.name) $($om.Version) (Old) Runtime - $oldtime"
write-host -fore green "$($nm.name) $($nm.Version) (New) Runtime - $newtime"

@gaelcolas
Copy link
Contributor Author

Yeah, not on a PC ATM but iirc that is because only last version is returned with get-module -ListAvailable, so when you have a specific rule/filter on the version you accept it's not enough.
Might need to be a bit more clever/restrictive as to when the -All is summoned!

@gaelcolas
Copy link
Contributor Author

And by 'last version' I think it doesn't even comply with semver iirc... 🙄 Just doing a string compare... Like 1.2.3-rc1.12 < 1.2.3-rc1.2 (wrong 12 > 2).

@JustinGrote
Copy link

Hi Gael,

Latest version is only returned if you don't use -listavailable. -listavailable returns all modules with the name in $psmodulepath

-All looks for submodules, etc. which is not the point of PSDepend (for instance if a module has a submodule the same version I want, it will report as "installed" even if I can't call that directly). Thats why it has to load the module first and takes forever (try having all the Az modules on your system and running it)

But yes, get-module in its current state in both WPS5.1 and PSCore6 is not prerelease-aware, you have to get the prerelease tag with

(get-module powercd).privatedata.psdata.prerelease

I'll take a look at your source branch and PR my recommendations

@ChrisLGardner
Copy link

ChrisLGardner commented Dec 11, 2018 via email

@gaelcolas
Copy link
Contributor Author

Worse than just this... In 5.1 (haven't checked for 6.x yet) you can't install a pre-release side by side with a normal release. You have to -force over existing pre-release... (same for Save-Module). Never realised the limitation.
Then as @JustinGrote wrote, Get-Module does not give you the pre-release in the version object, need to grab it from .PrivateData.PSData.Prerelease...

Will send an update soon-ish.

@tmeckel
Copy link

tmeckel commented Jan 31, 2020

@RamblingCookieMonster when will PSDepnd finally support prerelease versions of Powershell modules? @gaelcolas

@gaelcolas
Copy link
Contributor Author

This should be closed. Was going the wrong way. Jaykul's approach is better

@gaelcolas gaelcolas closed this Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants