useRememberedArgumentsForUpgrades feature doesn't work when upgrading packages via the Lets.GetChocolatey() entry point (such as with Chocolatey GUI)

[Destroy666x]

What You Are Seeing?

As I mentioned on Discord, whenever I upgrade a package with Chocolatey GUI (e.g. VirtualBox, ImageMagick), the --params argument doesn't get remembered. I was requested to create an issue here.

What is Expected?

Parameters should be remembered.

How Did You Get This To Happen? (Steps to Reproduce)

ImageMagick is not installed.

1. choco install imagemagick -y -f --params "InstallDevelopmentHeader=true LegacySupport=true" --version 7.1.0
2. choco upgrade imagemagick -y (to version 7.1.0.51)
3. The parameters were preserved in both imagemagick and imagemagick.app. Tested also mogrify command that's part of legacy tools and it worked.

Then I tried this, after running choco uninstall imagemagick imagemagick.app:

1. choco install imagemagick -y -f --params "InstallDevelopmentHeader=true LegacySupport=true" --version 7.1.0
2. Upgraded ImageMagick through GUI
3. The only parameter shown in GUI was --cache-location

So I guess your 1.2.0 fix, that was mentioned on Discord, worked for choco itself but not for GUI for some reason? What does the GUI do when upgrading? I thought it would just run the choco command.

System Details

• OS Build (In PowerShell run [System.Environment]::OSVersion.version.tostring()): 10.0.19045.0
• Windows PowerShell version (Run: $PSVersionTable): 5.1.19041.1682
• Chocolatey version (Run choco --version): 1.2.0
• Chocolatey GUI version: 1.1.0

Output Log

https://gist.github.com/Destroy666x/b01e6c5435c92488d26cb7a3670d083f

[vexx32]

Looking through the log here, every mention of imagemagick that seems relevant here is also accompanied by this line:

[DEBUG] - imagemagick - Adding remembered arguments for upgrade:  --package-parameters="'InstallDevelopmentHeader=true LegacySupport=true'" --allow-downgrade --cache-location="'C:\Users\Destr\AppData\Local\Temp\chocolatey'"

I can see ChocolateyGUI potentially getting confused in this process and not looking for this log entry or not being given this information, but this does appear to be working correctly, and I don't see any instances in the log where the upgrade is occurring without these arguments being pulled in.

[Destroy666x]

Well, no clue about the log, but the software does install without the parameter working 100%

[vexx32]

I'm not sure exactly what you mean. Does the install work as you would expect if the remembered arguments were used, or does it seem to install the application with the incorrect configuration, not seeming to use the remembered arguments at all?

[Destroy666x]

As I stated above and in steps to reproduce, the parameters are not remembered. With the example I showed it's simple to test - mogrify command can be either used (if upgraded through CLI) or not (if upgraded through GUI). Parameters were also not kept for multiple other apps I upgraded through GUI - e.g. VIrtualBox, Tor Browser or PHP.

Also, I was told on Discord by the devs that they found the issue and I should report it here and not in GUI repo, so better to align with them, not me.

[vexx32]

Thanks!

After much digging and consulting with @gep13, we figured out what's causing this. To fix this would require a degree of re-designing how the Lets.GetChocolatey() entry point works, so it's not something we can quickly fix. For folks coming to examine this in future for a proper fix, the issue is as follows:

1. When choco.exe is run, the entrypoint gets to this call in ConsoleApplication.cs: https://github.com/chocolatey/choco/blob/305b5466900532c7bbf417addc6429bc82ccf4b9/src/chocolatey/infrastructure.app/runners/ConsoleApplication.cs#L63-L114

2. This call passes this config object through its many layers of delegates, which is how arguments are parsed as needed, and eventually update the configuration as a result via the configure_argument_parser method on the relevant ICommand class (in this case, ChocolateyUpgradeCommand).

3. For running from the CLI, this is fine and everything works more or less as intended. However, when upgrading a package that has remembered arguments, eventually this call will be reached, which parses the remembered arguments via the command-line parser again in order to update the configuration with the content of the remembered arguments: https://github.com/chocolatey/choco/blob/305b5466900532c7bbf417addc6429bc82ccf4b9/src/chocolatey/infrastructure.app/services/NugetService.cs#L1477-L1479

4. For this pattern to work, crucially that config reference in this method must be the same config object that was passed into the parse_arguments_and_update_configuration method from ConsoleApplication.cs (or more properly, must be the same config object that is eventually handed to the ChocolateyUpgradeCommand's configure_argument_parser method.

This pattern / code flow is disrupted in the case of using Lets.GetChocolatey() because it accepts a completely separate config item that it will happily pass along to upgrade_run in the NugetService and eventually the aforementioned method, but will never set the "global" configuration that is intended to work with the CLI arguments.

From what we can tell, this has been broken for a very long time, the breakage is just a little more visible now. The Lets.GetChocolatey() entry point in chocolatey.lib is fundamentally broken in that it does not work with this flow of how upgrade_run needs to parse remembered arguments.

This is not something we can do a quick fix for at this time, but will likely require a rethink and redesign of how the entire API entry points here work and how we can correctly handle argument parsing for package parameters via both CLI and Lets.GetChocolatey() entry points. So for now I'm taking it out of this milestone and we will document this as a known issue for chocolatey.lib and Chocolatey GUI.

This will need to be fixed thoroughly, it's a heck of a bug. 😁

[Destroy666x]

Thanks for the detailed explantation, does that mean that a fix isn't expectable anytime soon? Or are you working on some sort of a refactor/redesign? I'm surprised that the GUI doesn't just run choco commands but uses some sort of API in the 1st place, considering it's not a "speed demon".

[vexx32]

A proper fix for this would essentially mean redesigning the entire API surface for things to work correctly, yeah.

It's not currently being worked on, but it is something we'll need to fix sooner than later. I'm not sure when this is likely to get fixed, there are a few other other large scopes of work that need to be finished before we can realistically look at this properly.

[Destroy666x]

Ok, thanks for the info. I guess I'll have to pin all the packages with custom arguments for now then as I have accidentally upgraded some through the GUI and then needed to reinstall. Unless there's a better solution other than that or "just use CLI less comfortable for regular usage, especially if you don't want to upgrade everything" but I can't really see any.

[gep13]

Based on the changes in the associated PR's for this issue, I am going to have to bump this issue from the 2.3.0 release, and instead look at this again in the 2.4.0.

The suggested changes to the INuGetService mean that this would essentially be a breaking change, that would need a corresponding change in the Chocolatey Licensed Extension, which we haven't planned to do at the minute.

Instead of holding up the release of 2.3.0, we will look to do the work in this issue in a later release.

Apologies for any inconvenience that this causes.