(chocolatey-core.extension) Added simple wrapper around Install-ChocolateyInstallPackage #612

AdmiringWorm · 2017-03-05T17:33:44Z

This wrapper makes it easier to embedd both 32-bit and 64-bit
installers inside a package, extracting the needed check
to decide which installer is meant to be run.

I plan to make the same for Get-ChocolateyUnzip, but want to see if I'm moving in the right direction first.

majkinetor · 2017-03-05T17:58:14Z

LGTM

pascalberger

LGTM, other than the small intendation issue

pascalberger · 2017-03-05T18:05:51Z

extensions/chocolatey-core.extension/extensions/Install-ChocolateyInstallPackageEx.ps1

+  }
+
+  $packageArgs = @{
+    packageName    = $packageName


Remove whitespace before = or intend all of them to the same level

I've indented them this way on purpose, if I were to align them all to the same place as useOnlyPackageSilentArgs then that would be too much IMO.
Therefore I indented only the first 5 arguments.

It looks fine to me.

ferventcoder · 2017-03-05T18:42:35Z

Would it be simply better to just add this to 0.10.4?

ferventcoder · 2017-03-05T18:44:59Z

I'm just thinking about all of the things, like the package-validator and those similar that would now need to account for this. Not that excited about changing all of those systems so they can see this as well when we can simply fix Chocolatey.

ferventcoder · 2017-03-05T18:46:30Z

And yes, I do realize that means it would be a few months before we could account for it on the community repository (about 6 months to allow the community to fully upgrade). As we've seen, folks are still upgrading from 0.9.8.x (over 3 years old now!!!!) so that things will work with latest changes on the community repository.

AdmiringWorm · 2017-03-05T18:47:40Z

@ferventcoder Yeah it would, but that would require the user to run choco v 0.10.4+.

I'm just thinking about all of the things, like the package-validator and those similar that would now need to account for this. Not that excited about changing all of those systems so they can see this as well when we can simply fix Chocolatey.

I understand that, the reason I added this script is to have an option when users are running an older version of choco.
If there is a way to substitute the function when an older version is used, then I'm all ears.

ferventcoder · 2017-03-05T19:04:00Z

If there is a way to substitute the function when an older version is used, then I'm all ears.

@AdmiringWorm there actually is. It's actually somewhat simple and something we used a long time ago when we were building mocking for Pester.

You can look at the version of Chocolatey $env:CHOCOLATEY_VERSION - https://github.com/chocolatey/choco/wiki/HelpersReference#environment-variables

CHOCOLATEY_VERSION - The version of Choco you normally see. Use if you are 'lighting' things up based on choco version. (0.9.9+) - Otherwise take a dependency on the specific version you need.

If that version is less than version 0.10.4 (flip it to use System.Version I believe and compare the two).

Then use that to decide whether to load this or not - if you decide to load the function, capture the original Install-ChocolateyInstallPackage:

Rename-Item function:Install-ChocolateyInstallPackage Install-ChocolateyInstallPackageActual

And rename the Install-ChocolateyInstallPackageEx function to Install-ChocolateyInstallPackage. So it would look something like the following:

$currentChocoVersion = $env:CHOCOLATEY_VERSION
if ($currentChocoVersion -and [Version]$currentChocoVersion -ge [Version]'0.10.4'){
  return
}

Write-Debug "Loading Install-ChocolateyInstallPackage override"
Rename-Item function:Install-ChocolateyInstallPackage Install-ChocolateyInstallPackage-Actual

# docs here
function Install-ChocolateyInstallPackage {
  # code here
  Install-ChocolateyInstallPackage-Actual @packageArgs  
}

ferventcoder

Take a look at what has been requested as far as a really good way to handle this.

AdmiringWorm · 2017-03-05T19:08:55Z

@ferventcoder awesome, I'll definitely take a look at changing this function to something like that.
I'm not really a fan of just testing for a certain version though, but I did find a way to test if a certain parameter exists in a function.

ferventcoder · 2017-03-05T19:09:51Z

extensions/chocolatey-core.extension/extensions/Install-ChocolateyInstallPackageEx.ps1

+    [alias("installerType","installType")][string] $fileType = 'exe',
+    [parameter(Mandatory=$false, Position=2)][string[]] $silentArgs = '',
+    [alias("fileFullPath")][parameter(Mandatory=$false, Position=3)][string] $file,
+    [alias("fileFullPath64","file64Bit")][Parameter(Mandatory=$false, Position=4)][string] $file64,


Remove position. We are moving away from this. So never add new items with a position.

Also not a fan of $file64bit, can it be removed? I want this to have parity with function updates that will roll back into Chocolatey.

sure, it can be removed. I actually added it because of the use of -Url64bit in the Install-ChocolateyPackage

ferventcoder · 2017-03-05T19:12:46Z

extensions/chocolatey-core.extension/extensions/Install-ChocolateyInstallPackageEx.ps1

+    [parameter(Mandatory=$false, Position=1)]
+    [alias("installerType","installType")][string] $fileType = 'exe',
+    [parameter(Mandatory=$false, Position=2)][string[]] $silentArgs = '',
+    [alias("fileFullPath")][parameter(Mandatory=$false, Position=3)][string] $file,


Do you think we should leave this mandatory - This will either be a 64bit file or 32bit if more than one is sent.

I don't really think this should be mandatory, if a package only supports 64-bit the IMO the helper function should throw an exception when trying to install the 32-bit version

My point here is making this as well transitioned as possible.

The documentation leads me to believe this is 32-bit, when in fact I can pass a 64bit installer through $file. Think of how this would be used over time and transition.

Although I see the idea of making it optional so we can fail early if the file is only supported in 64bit scenarios.

The idea was to have it behave somewhat similiar to the Install-ChocolateyPackage urls parameters (which also gives the impression that Url parameter is only for 32bit).
I'll definitely need to improve the documentation though.
I don't really see the problem with the transition though, since it will still fail if both file and file64 is not set.

ferventcoder · 2017-03-05T19:14:21Z

I'm not really a fan of just testing for a certain version though, but I did find a way to test if a certain parameter exists in a function.

That's fine as well. And likely better.

ferventcoder · 2017-03-05T19:22:27Z

Filed chocolatey/choco#1187

AdmiringWorm · 2017-03-05T19:48:02Z

@ferventcoder I've added the check and renaming of the existing function.
I haven't yet tested it in POSH v2 though, but that as well as the other comments must unfortunately have to wait until tomorrow.

ferventcoder · 2017-03-05T19:50:51Z

extensions/chocolatey-core.extension/extensions/Install-ChocolateyInstallPackage.ps1

+}
+
+# We need to force the export of Install-ChocolateyInstall function
+Export-ModuleMember -Function Install-ChocolateyInstallPackage


Will that work from here?

And this is why I had a particular recommendation on letting each function decide whether to load up versus the universal one in the PowerShell module file. Sometimes I have a good reason that I can't always explain at the proper moment. Or it doesn't had those needs like this would have now.

I believe so yes (I did try it on my own computer, although only through importing chocolatey-core.psm1 manually).
Since the chocolatey-core.psm1 is sourcing all of the files, it just don't export it since it had found an existing function with the same name.

Before importing the chocolatey-core.psm1 file manually the following functions was found:

> Get-ChildItem function:\Install* CommandType Name Version Source ----------- ---- ------- ------ Function Install-BinFile 0.0 chocolateyInstaller Function Install-ChocolateyDesktopLink 0.0 chocolateyInstaller Function Install-ChocolateyEnvironmentVariable 0.0 chocolateyInstaller Function Install-ChocolateyExplorerMenuItem 0.0 chocolateyInstaller Function Install-ChocolateyFileAssociation 0.0 chocolateyInstaller Function Install-ChocolateyInstallPackage 0.0 chocolateyInstaller Function Install-ChocolateyPackage 0.0 chocolateyInstaller Function Install-ChocolateyPath 0.0 chocolateyInstaller Function Install-ChocolateyPinnedTaskBarItem 0.0 chocolateyInstaller Function Install-ChocolateyPowershellCommand 0.0 chocolateyInstaller Function Install-ChocolateyShortcut 0.0 chocolateyInstaller Function Install-ChocolateyVsixPackage 0.0 chocolateyInstaller Function Install-ChocolateyZipPackage 0.0 chocolateyInstaller Function Install-VisualStudio 0.0 chocolateyInstaller Function Install-Vsix 0.0 chocolateyInstaller Function Install-WindowsUpdate 0.0 chocolateyInstaller

After importing:

> Get-ChildItem function:\Install* CommandType Name Version Source ----------- ---- ------- ------ Function Install-BinFile 0.0 chocolateyInstaller Function Install-ChocolateyDesktopLink 0.0 chocolateyInstaller Function Install-ChocolateyEnvironmentVariable 0.0 chocolateyInstaller Function Install-ChocolateyExplorerMenuItem 0.0 chocolateyInstaller Function Install-ChocolateyFileAssociation 0.0 chocolateyInstaller Function Install-ChocolateyInstallPackage 0.0 chocolatey-core Function Install-ChocolateyPackage 0.0 chocolateyInstaller Function Install-ChocolateyPath 0.0 chocolateyInstaller Function Install-ChocolateyPinnedTaskBarItem 0.0 chocolateyInstaller Function Install-ChocolateyPowershellCommand 0.0 chocolateyInstaller Function Install-ChocolateyShortcut 0.0 chocolateyInstaller Function Install-ChocolateyVsixPackage 0.0 chocolateyInstaller Function Install-ChocolateyZipPackage 0.0 chocolateyInstaller Function Install-VisualStudio 0.0 chocolateyInstaller Function Install-Vsix 0.0 chocolateyInstaller Function Install-WindowsUpdate 0.0 chocolateyInstaller Function Install-ChocolateyInstallPackageOriginal 0.0 chocolatey-core

Ruhroh - looks like it is not picking up the original one.

Function Install-ChocolateyInstallPackageOriginal 0.0 chocolatey-core

That should probably be

Function Install-ChocolateyInstallPackageOriginal 0.0 chocolateyInstaller

I think it's a drawback of the global import file, and the use of rename-item.
Could be wrong though.

When importing with the latest commits,
the results is now this:

PS C:\Users\Administrator> Get-ChildItem function:\Install* CommandType Name ModuleName ----------- ---- ---------- Function Install-BinFile chocolateyInstaller Function Install-ChocolateyDesktopLink chocolateyInstaller Function Install-ChocolateyEnvironmentVariable chocolateyInstaller Function Install-ChocolateyExplorerMenuItem chocolateyInstaller Function Install-ChocolateyFileAssociation chocolateyInstaller Function Install-ChocolateyInstallPackage chocolateyInstaller Function Install-ChocolateyInstallPackageOriginal chocolateyInstaller Function Install-ChocolateyPackage chocolateyInstaller Function Install-ChocolateyPath chocolateyInstaller Function Install-ChocolateyPinnedTaskBarItem chocolateyInstaller Function Install-ChocolateyPowershellCommand chocolateyInstaller Function Install-ChocolateyShortcut chocolateyInstaller Function Install-ChocolateyVsixPackage chocolateyInstaller Function Install-ChocolateyZipPackage chocolateyInstaller Function Install-Vsix chocolateyInstaller Function Install-WindowsUpdate chocolateyInstaller

So now the opposite problem.

well, yes and no.
The reason I got chocolatey-core and chocolateyInstaller is becuase previously I imported both chocolateyInstaller.psm1 and chocolatey-core.psm1 separately.

Now I only imported chocolateyInstaller.psm1

ferventcoder · 2017-03-05T19:54:37Z

extensions/chocolatey-core.extension/extensions/Install-ChocolateyInstallPackage.ps1

+  return
+}
+
+Write-Debug "Loading Install-ChocolateyInstallPackage override"


And these debugging messages are necessary for determining issues - it may not be a bad idea to also have a debug message in the function itself.

ferventcoder · 2017-03-05T20:09:25Z

extensions/chocolatey-core.extension/extensions/Install-ChocolateyInstallPackage.ps1

+}
+
+Install-ChocolateyInstallPackageEx @packageArgs
+#>


Synopsis and docs should be right by function Install-ChocolateyInstallPackage {.

ferventcoder · 2017-03-05T20:27:29Z

extensions/chocolatey-core.extension/extensions/Install-ChocolateyInstallPackage.ps1

+    } else {
+      Write-Host "Installing $packageName 64-bit"
+    }
+    $fileFullPath = $file64


This looks incorrect.

This should be captured inside of the else statement above it.

AdmiringWorm · 2017-03-05T20:38:34Z

Looks like Rename-Item can't be used as it currently is.
The extensions are imported before chocos own extensions is imported, which results in the original Install-ChocolateyInstallPackage function isn't yet available 😢

ferventcoder · 2017-03-05T20:40:36Z

IIRC - Chocolatey loads its functions first, but it may not export everything until later, which results in a much less deterministic way of knowing what is loaded.

ferventcoder · 2017-03-05T20:41:04Z

However, I'm guessing you may be able to search for the function in chocolateyInstaller module namespace.

AdmiringWorm · 2017-03-05T20:43:07Z

Chocolatey loads its functions first, but it may not export everything until later

Sorry, that's what I meant.

However, I'm guessing you may be able to search for the function in chocolateyInstaller module namespace.

Yeah it could be, I'll need to have a look at that.

ferventcoder · 2017-03-06T15:01:07Z

@AdmiringWorm changed up the logic a bit - Let's ensure we match up on parameter names and aliases - chocolatey/choco@4cf2441

AdmiringWorm · 2017-03-07T04:21:49Z

@ferventcoder I'll update this one with the same logic.
Still struggling with the renaming the original function though, currently the only way I've found that works is to first manually source the original Install-ChocolateyInstallPackage script.

ferventcoder · 2017-03-13T22:45:44Z

@AdmiringWorm note I added chocolatey/choco#1200 to address the non-deterministic load (not that it is going to help here)

AdmiringWorm · 2017-03-14T03:39:31Z

@ferventcoder said
@AdmiringWorm note I added chocolatey/choco#1200 to address the non-deterministic load (not that it is going to help here)

yeah, that probably won't help in this case.
I honestly don't know how to proceed with this PR, as previously mentioned the only way that I found to work was to first source the Install-ChocolateyInstallPackage file which then made it discoverable.

AdmiringWorm · 2017-03-17T17:53:37Z

@ferventcoder any chance of getting another review on this PR? so it hopefully can be merged....

AdmiringWorm · 2017-04-08T18:14:04Z

@ferventcoder I know I'm nagging, but is this PR acceptable as it currently stands?
If not, this PR could probably just be closed as I'm not able to get it to work any other way.

majkinetor · 2017-04-11T18:47:51Z

Why do we need it since its added to chocolaty ? If you think older versions of choco, we can add dependency to chocolatey 0.10.5 instead of this ?

AdmiringWorm · 2017-04-12T20:14:42Z

@majkinetor while that is certainly possible, I believe there should be an option to keep backwards compatibility while still using new functions. Hence why this PR, as well as it was started before it was added to choco.

Like what is done with the Get-UninstallRegistryKey function.

majkinetor · 2017-04-13T06:44:30Z

Why promote bad behavior (keeping the old choco version) ? This way you force users to transparently update the choco by putting it as dependency.

…lateyInstallPackage This wrapper makes it easier to embedd both 32-bit and 64-bit installers inside a package, extracting the needed check to decide which installer is meant to be run.

AdmiringWorm · 2017-05-06T18:57:22Z

I'm closing this now, as there seem to be no interest in it.

ferventcoder · 2017-05-10T15:12:06Z

Apologies I didn't get back to this. Getting the functionality working properly with the same name was my biggest concern.

AdmiringWorm · 2017-05-10T20:52:28Z

Getting the functionality working properly with the same name was my biggest concern.

I understand that, unfortunately I wasn't able to get it to work with the methods you suggested.
The only way I could make it to work was with sourcing the actual script manually (semi-manually?), and although I'm not entirely happy with it I could see no other way.

However, I've dropped trying to get this into the extension. I haven't removed the branch though, as I'm hoping someone will pick it up and continue the work...

/CC @ferventcoder

AdmiringWorm · 2017-05-10T20:56:41Z

@majkinetor said...
Why promote bad behavior (keeping the old choco version) ? This way you force users to transparently update the choco by putting it as dependency.

The idea behind it wasn't really meant as to promote bad behavior, but rather to provide an alternative to users that can't update chocolatey (be it because of a bug, or company policy or something else).
As well as I don't really like having a dependency on chocolatey itself (thinks of it like this, the user only have 1 package installed that depends on choco, he removes the package using choco uninstall -x packageName, it would then also remove the lib directory of chocolatey making it not updating itself anymore, possibly stop working (happened during testing for me for a package))

majkinetor · 2017-05-11T06:25:27Z

users that can't update chocolatey (be it because of a bug, or company policy or something else).
You need to keep perspective. There will be very very very low amount of those users. And they are getting lower lower lower every day.

choco uninstall -x packageName, it would then also remove the lib directory of chocolatey

If this happens @ferventcoder , it probably shouldn't. Did you try it ? Chocolatey is added as first package during installation, it doesn't make sense it gets ever uninstalled.

AdmiringWorm requested review from ferventcoder, gep13 and pascalberger March 5, 2017 17:33

AdmiringWorm added the 4 - Done label Mar 5, 2017

AdmiringWorm force-pushed the 64bit-install-ext branch from 87fc41f to f260cf5 Compare March 5, 2017 17:49

pascalberger approved these changes Mar 5, 2017

View reviewed changes

ferventcoder requested changes Mar 5, 2017

View reviewed changes

AdmiringWorm added 2 - Working and removed 4 - Done labels Mar 5, 2017

ferventcoder reviewed Mar 5, 2017

View reviewed changes

ferventcoder mentioned this pull request Mar 5, 2017

Add File64 to Install-ChocolateyInstallPackage and Get-ChocolateyUnzip chocolatey/choco#1187

Closed

ferventcoder reviewed Mar 5, 2017

View reviewed changes

AdmiringWorm added 4 - Done and removed 2 - Working labels Mar 7, 2017

AdmiringWorm force-pushed the 64bit-install-ext branch 2 times, most recently from 03932aa to 89771f5 Compare March 15, 2017 19:12

AdmiringWorm mentioned this pull request Mar 15, 2017

(GH-621) PowerShell functions - allow for ignored parameters and splatting #622

Merged

AdmiringWorm force-pushed the 64bit-install-ext branch from 89771f5 to f1d7368 Compare March 31, 2017 14:12

AdmiringWorm mentioned this pull request Apr 12, 2017

(putty*) Adding support for 64bit putty installer/archive #671

Merged

AdmiringWorm added 2 commits April 28, 2017 09:49

(chocolatey-core.extension) Added simple wrapper around Install-Choco…

24dd780

…lateyInstallPackage This wrapper makes it easier to embedd both 32-bit and 64-bit installers inside a package, extracting the needed check to decide which installer is meant to be run.

(chocolatey-core.extension) Updated changelog

821d1e1

AdmiringWorm force-pushed the 64bit-install-ext branch from f1d7368 to 821d1e1 Compare April 28, 2017 07:49

AdmiringWorm closed this May 6, 2017

majkinetor deleted the 64bit-install-ext branch October 19, 2019 06:55

(chocolatey-core.extension) Added simple wrapper around Install-ChocolateyInstallPackage #612

(chocolatey-core.extension) Added simple wrapper around Install-ChocolateyInstallPackage #612

Conversation

AdmiringWorm commented Mar 5, 2017

majkinetor commented Mar 5, 2017

pascalberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferventcoder commented Mar 5, 2017

ferventcoder commented Mar 5, 2017

ferventcoder commented Mar 5, 2017

AdmiringWorm commented Mar 5, 2017

ferventcoder commented Mar 5, 2017 • edited

ferventcoder left a comment

Choose a reason for hiding this comment

AdmiringWorm commented Mar 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferventcoder commented Mar 5, 2017

ferventcoder commented Mar 5, 2017

AdmiringWorm commented Mar 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdmiringWorm commented Mar 5, 2017

ferventcoder commented Mar 5, 2017

ferventcoder commented Mar 5, 2017

AdmiringWorm commented Mar 5, 2017

ferventcoder commented Mar 6, 2017

AdmiringWorm commented Mar 7, 2017

ferventcoder commented Mar 13, 2017 • edited

AdmiringWorm commented Mar 14, 2017

AdmiringWorm commented Mar 17, 2017

AdmiringWorm commented Apr 8, 2017

majkinetor commented Apr 11, 2017

AdmiringWorm commented Apr 12, 2017

majkinetor commented Apr 13, 2017

AdmiringWorm commented May 6, 2017

ferventcoder commented May 10, 2017

AdmiringWorm commented May 10, 2017 • edited

AdmiringWorm commented May 10, 2017

majkinetor commented May 11, 2017

ferventcoder commented Mar 5, 2017 •

edited

ferventcoder commented Mar 13, 2017 •

edited

AdmiringWorm commented May 10, 2017 •

edited