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

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

Open
Destroy666x opened this issue Nov 4, 2022 · 9 comments · May be fixed by #3003
Assignees
Milestone

Comments

@Destroy666x
Copy link

Destroy666x commented Nov 4, 2022

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

@pauby pauby added this to the 1.2.1 milestone Nov 7, 2022
@vexx32
Copy link
Member

vexx32 commented Nov 29, 2022

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
Copy link
Author

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

@vexx32
Copy link
Member

vexx32 commented Nov 30, 2022

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
Copy link
Author

Destroy666x commented Dec 1, 2022

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 vexx32 modified the milestones: 1.2.1, Future Dec 2, 2022
@vexx32
Copy link
Member

vexx32 commented Dec 2, 2022

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:

    runner.run(config, container, isConsole: true, parseArgs: command =>
    {
    ConfigurationOptions.parse_arguments_and_update_configuration(
    commandArgs,
    config,
    (optionSet) => command.configure_argument_parser(optionSet, config),
    (unparsedArgs) => {
    // if debug is bundled with local options, it may not get picked up when global
    // options are parsed. Attempt to set it again once local options are set.
    // This does mean some output from debug will be missed (but not much)
    if (config.Debug) Log4NetAppenderConfiguration.set_logging_level_debug_when_debug(config.Debug, "{0}LoggingColoredConsoleAppender".format_with(ChocolateyLoggers.Verbose.to_string()), "{0}LoggingColoredConsoleAppender".format_with(ChocolateyLoggers.Trace.to_string()));
    command.handle_additional_argument_parsing(unparsedArgs, config);
    if (!config.Features.IgnoreInvalidOptionsSwitches)
    {
    // all options / switches should be parsed,
    // so show help menu if there are any left
    foreach (var unparsedArg in unparsedArgs.or_empty_list_if_null())
    {
    if (unparsedArg.StartsWith("-") || unparsedArg.StartsWith("/"))
    {
    config.HelpRequested = true;
    config.UnsuccessfulParsing = true;
    }
    }
    }
    },
    () => {
    this.Log().Debug(() => "Performing validation checks.");
    command.handle_validation(config);
    var validationResults = new List<ValidationResult>();
    var validationChecks = container.GetAllInstances<IValidation>();
    foreach (var validationCheck in validationChecks)
    {
    validationResults.AddRange(validationCheck.validate(config));
    }
    var validationErrors = report_validation_summary(validationResults, config);
    if (validationErrors != 0)
    {
    // NOTE: This is intentionally left blank, as the reason for throwing is
    // documented in the report_validation_summary above, and a duplication
    // is not required in the exception.
    throw new ApplicationException("");
    }
    },
    () => command.help_message(config));
    });
    }

  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:

    var originalConfig = config.deep_copy();
    // this changes config globally
    ConfigurationOptions.OptionSet.Parse(packageArguments);

  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. 😁

@vexx32 vexx32 changed the title useRememberedArgumentsForUpgrades feature doesn't seem to work within Chocolatey GUI useRememberedArgumentsForUpgrades feature doesn't work when upgrading packages via the Lets.GetChocolatey() entry point (such as with Chocolatey GUI) Dec 2, 2022
@Destroy666x
Copy link
Author

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
Copy link
Member

vexx32 commented Jan 18, 2023

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
Copy link
Author

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.

TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jan 28, 2023
…ration

This renames set_package_config_for_upgrade to
get_package_config_from_remembered_args and changes how it operates.
First, a option set is set up, so only the config that is passed in is
modified, instead of the config that the global options parser was set
with with.
Second, it returns the modified configuration instead of the original
configuration, because it is the local configuration being modified.
Third, it renames the function and changes log messages to be more
general, so it later can more easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jun 16, 2023
…ration

This renames set_package_config_for_upgrade to
get_package_config_from_remembered_args and changes how it operates.
First, a option set is set up, so only the config that is passed in is
modified, instead of the config that the global options parser was set
with with.
Second, it returns the modified configuration instead of the original
configuration, because it is the local configuration being modified.
Third, it renames the function and changes log messages to be more
general, so it later can more easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jun 16, 2023
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jun 20, 2023
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jul 1, 2023
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Aug 9, 2023
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
@gep13 gep13 modified the milestones: Future, 2.3.0 Dec 14, 2023
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jan 9, 2024
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
@TheCakeIsNaOH TheCakeIsNaOH self-assigned this Jan 9, 2024
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jan 9, 2024
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jan 10, 2024
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jan 10, 2024
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jan 10, 2024
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Jan 10, 2024
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Apr 28, 2024
…ration

This adds a new method, GetPackageConfigFromRememberedArguments which
is similar to SetConfigFromRememberedArguments, but operates using a
different method. First, a OptionSet is set up, so only the config
that is passed in is modified, instead of the config that the global
options parser was set with with. Second, it returns the modified
configuration instead of the original configuration, because it is the
local configuration being modified. Third, it has a more general name
and changes log messages to be more general, so it later can more
easily be reused for uninstall and export.

This change fixes remembered arguments when Chocolatey is used as a
library, like in ChocolateyGUI, where the config that is passed to
the install_run method is not necessarily the same config object that
was used to set up the global argument parser.

The downside of using a new commandline parser is that it opens up the
possibility of drift between the upgrade/global arguments and this
added parser. However, this is not an issue because the format of the
saved arguments is known, and any added arguments there would not work
without being added here as well, which would be picked up during
development.
@gep13 gep13 modified the milestones: 2.3.0, 2.4.0 May 7, 2024
@gep13
Copy link
Member

gep13 commented May 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants