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

Upgrade all reuses overridden package parameters when useRememberedArgumentsForUpgrades feature is turned on #1443

Closed
dinfinity opened this issue Nov 6, 2017 · 44 comments · Fixed by #2484
Assignees
Labels
5 - Released Bug IN REGRESSION TEST SUITE These are things that are handled by tools like Test-Kitchen Priority_HIGH
Milestone

Comments

@dinfinity
Copy link

dinfinity commented Nov 6, 2017

What You Are Seeing?

I have a multitude of packages installed via chocolatey, a number of which I have set custom install directories for. Chocolatey regularly shows error messages that indicate that the overridden parameters for another package are used. For instance:

ERROR: Running ["C:\Users\Dual Infinity\AppData\Local\Temp\chocolatey\TotalCommander\9.10\INSTALL.exe" INSTALLDIR=C:\Frameworks\NodeJS] was not successful. Exit code was '1'. See log for possible error messages.
The upgrade of totalcommander was NOT successful.
Error while running 'C:\ProgramData\chocolatey\lib\TotalCommander\tools\chocolateyInstall.ps1'.
 See log for details.

It seems to use the last overridden parameters encountered.

Addendum 2017-11-07:
The issue is usually resolved by running the upgrade all command multiple times. It does not seem random, but it does seem that if a package with custom parameters was installed successfully its parameters are not read and do not conflict in the subsequent run of upgrade all.

What is Expected?

Chocolatey should (obviously) not reuse package parameters for other packages.

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

In essence, this would entail setting custom install dirs for multiple packages and running chocolatey upgrade all.

Output Log

See zip:
dinfinity.chocolatey.log.2017-11-06.zip

Related

@ferventcoder
Copy link
Member

ferventcoder commented Nov 6, 2017

This sounds like a bug, and a major one.

Search:
useRememberedArgumentsForUpgrades, useRememberedArguments, useRememberedArgs, useRemembered, use remembered args arguments

@ferventcoder ferventcoder added this to the 0.10.9 milestone Nov 6, 2017
@ferventcoder
Copy link
Member

This isn't going to be quite as easy to find the cause of as hoped. Thanks for the log file. The code in question doesn't reuse parameters (on first glance, there could be a small bug in there).

I think to really find this, we're going to need to understand how you installed everything and possibly take a much closer look (if you are up for it).

How did you install things? One at a time or did you run multiple choco.exe processes at the same time?

@dinfinity
Copy link
Author

Taking a closer look is no problem. I like chocolatey a lot.

I have been ignoring this issue for a couple of months now, so I can't say for sure I did not run multiple simultaneous choco commands, although I can't remember doing so and deem it quite unlikely.

I could provide a zip of (parts of) my ProgramData\chocolatey directory if that helps.

I'm adding the following to the issue description:
"The issue is usually resolved by running the upgrade all command multiple times. It does not seem random, but it does seem that if a package with custom parameters was installed successfully its parameters are not read and do not conflict in the subsequent run of upgrade all."

@ferventcoder
Copy link
Member

Without a repo, we'll need to bump this to vNext.

@Vankog
Copy link

Vankog commented Mar 7, 2018

As it turns out, this also happened to me:
http://disq.us/p/1qp8o56

VLC got "upgraded" from 64 to 32bit on upgrade all after reusing remembered arguments that forced the 32bit upgrade for notepadplusplus earlier.

@ferventcoder
Copy link
Member

I'm wondering if packages getting upgraded as a result of being a dependency of another package are getting hit by this...

@ferventcoder
Copy link
Member

Adding a partial adjustment for 0.10.11, but we'll need to see if it alleviates the bigger issue - I don't think it will until we isolate for each package.

ferventcoder added a commit that referenced this issue May 4, 2018
There may be a condition somewhere when the config gets changed each
time through for a package. Note this won't cover dependencies, they
will likely get those arguments passed to them.
ferventcoder added a commit that referenced this issue May 4, 2018
* stable:
  (version) 0.10.11
  (doc) update release notes for 0.10.11
  (GH-1540) Fix - AutoUninstaller can't find escaped quotes
  (GH-1543) Fix - log format error on install location with GUID
  (GH-1500) Fix - Wrap Write-Host if used in setup
  (GH-991) Install - Modify profile if file exists & empty
  (GH-1443) Set original config before loop
  (maint) add a note for GH-1548
  (GH-1557) Upgrade 7z to 18.5
@ferventcoder ferventcoder self-assigned this Dec 18, 2018
ferventcoder referenced this issue Dec 18, 2018
Ensure the config is reset to a known point with each package being run
in an operation. Previously it was thought this was happening, but the
way the reset occurred was actually changing the original configuration
and the current. Ensure that is getting reset accordingly by doing the
deep copy to set config each time through.
@dinfinity
Copy link
Author

In my case this seems very very similar to what is reported here: #797

There are 2 packages (out of many) that I installed with -ia "INSTALLDIR=C:\aaa\bbb" (Vagrant and NodeJS) and some other packages (Total Commander, Mysql Workbench, Google Earth Pro) want to install into the NodeJS and Vagrant directories.

I see a fix has been added in 347f09f
I am hoping that will solve this very, very annoying issue.

@gep13 gep13 added this to the 1.2.0 milestone Aug 11, 2022
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Aug 11, 2022
This adds a extra parameter to the upgrade_run method, which
is used to pass an action which resets the configuration in the
function that calls upgrade_run.

This fixes a bug when useRememberedArguments is enabled that causes
arguments to be reused in the chocolateyInstall.ps1 when multiple
packages are upgraded. It happens because OptionSet.parse() sets the
configuration via the actions specified in the UpgradeCommand optionSet
setup (and likewise in configuration builder as well). It only sets
options that are saved, so if a later package does not have an option,
it is not set, and the previous option is reused.

This fixes the bug because it resets the configuration at the
ChocolateyPackageService level, which is enough to reset the
configuration for action that runs the PowerShell as well.
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Aug 23, 2022
This adds a extra parameter to the upgrade_run method, which
is used to pass an action which resets the configuration in the
function that calls upgrade_run.

This fixes a bug when useRememberedArguments is enabled that causes
arguments to be reused in the chocolateyInstall.ps1 when multiple
packages are upgraded. It happens because OptionSet.parse() sets the
configuration via the actions specified in the UpgradeCommand optionSet
setup (and likewise in configuration builder as well). It only sets
options that are saved, so if a later package does not have an option,
it is not set, and the previous option is reused.

This fixes the bug because it resets the configuration at the
ChocolateyPackageService level, which is enough to reset the
configuration for action that runs the PowerShell as well.
@pauby pauby added the ADD AUTO TESTS Things that typically go to Test-Kitchen - once completed, IN REGRESSION TEST SUITE label added label Aug 25, 2022
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 16, 2022
This updates the action used when handling package results
during upgrade to reset the configuration class that is initially
set.

This is done as an alternative way other than updating the source
runners themself with a breaking change to the interfaces.

This fixes a bug when useRememberedArguments is enabled that causes
arguments to be reused in the chocolateyInstall.ps1 when multiple
packages are upgraded. It happens because OptionSet.parse() sets the
configuration via the actions specified in the UpgradeCommand optionSet
setup (and likewise in configuration builder as well). It only sets
options that are saved, so if a later package does not have an option,
it is not set, and the previous option is reused.

This fixes the bug because it resets the configuration at the
ChocolateyPackageService level, which is enaugh to reset the
configuration for action that runs the PowerShell as well.

Co-authored-by: TheCakeIsNaOH <[email protected]>
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 20, 2022
This commit adds two new methods to the Chocolatey Configuration class
that will be used to create a backup of the current values inside the Config
class, without these being able to be changed.

Once requested and the backup exists, we are then able to reset the Config
file without loosing the reference to the class.
This was something that is needed, as we rely in several places that
the reference is updatable throughout the entire Chocolatey CLI codebase.
This also means we can not replace the reference to the Config file itself
without loosing the ability to make use of remembered arguments when
multiple packages requires these to be used.
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 20, 2022
This updates the action used when handling package results
during upgrade to reset the configuration class that is initially
set.

This fixes a bug when useRememberedArguments is enabled that causes
arguments to be reused in the chocolateyInstall.ps1 when multiple
packages are upgraded. It happens because OptionSet.parse() sets the
configuration via the actions specified in the UpgradeCommand optionSet
setup (and likewise in configuration builder as well). It only sets
options that are saved, so if a later package does not have an option,
it is not set, and the previous option is reused.

This fixes the bug because it resets the configuration at the
ChocolateyPackageService level, which is enough to reset the
configuration for action that runs the PowerShell as well.

Co-authored-by: TheCakeIsNaOH <[email protected]>
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 20, 2022
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 21, 2022
This commit adds two new methods to the Chocolatey Configuration class
that will be used to create a backup of the current values inside the Config
class, without these being able to be changed.

Once requested and the backup exists, we are then able to reset the Config
file without loosing the reference to the class.
This was something that is needed, as we rely in several places that
the reference is updatable throughout the entire Chocolatey CLI codebase.
This also means we can not replace the reference to the Config file itself
without loosing the ability to make use of remembered arguments when
multiple packages requires these to be used.
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 21, 2022
This updates the action used when handling package results
during upgrade to reset the configuration class that is initially
set.

This fixes a bug when useRememberedArguments is enabled that causes
arguments to be reused in the chocolateyInstall.ps1 when multiple
packages are upgraded. It happens because OptionSet.parse() sets the
configuration via the actions specified in the UpgradeCommand optionSet
setup (and likewise in configuration builder as well). It only sets
options that are saved, so if a later package does not have an option,
it is not set, and the previous option is reused.

This fixes the bug because it resets the configuration at the
ChocolateyPackageService level, which is enough to reset the
configuration for action that runs the PowerShell as well.

Co-authored-by: TheCakeIsNaOH <[email protected]>
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 21, 2022
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 21, 2022
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 22, 2022
This commit adds two new methods to the Chocolatey Configuration class
that will be used to create a backup of the current values inside the Config
class, without these being able to be changed.

Once requested and the backup exists, we are then able to reset the Config
file without loosing the reference to the class.
This was something that is needed, as we rely in several places that
the reference is updatable throughout the entire Chocolatey CLI codebase.
This also means we can not replace the reference to the Config file itself
without loosing the ability to make use of remembered arguments when
multiple packages requires these to be used.
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 22, 2022
This updates the action used when handling package results
during upgrade to reset the configuration class that is initially
set.

This fixes a bug when useRememberedArguments is enabled that causes
arguments to be reused in the chocolateyInstall.ps1 when multiple
packages are upgraded. It happens because OptionSet.parse() sets the
configuration via the actions specified in the UpgradeCommand optionSet
setup (and likewise in configuration builder as well). It only sets
options that are saved, so if a later package does not have an option,
it is not set, and the previous option is reused.

This fixes the bug because it resets the configuration at the
ChocolateyPackageService level, which is enough to reset the
configuration for action that runs the PowerShell as well.

Co-authored-by: TheCakeIsNaOH <[email protected]>
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 22, 2022
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 22, 2022
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 22, 2022
This commit adds two new methods to the Chocolatey Configuration class
that will be used to create a backup of the current values inside the Config
class, without these being able to be changed.

Once requested and the backup exists, we are then able to reset the Config
file without loosing the reference to the class.
This was something that is needed, as we rely in several places that
the reference is updatable throughout the entire Chocolatey CLI codebase.
This also means we can not replace the reference to the Config file itself
without loosing the ability to make use of remembered arguments when
multiple packages requires these to be used.
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 22, 2022
This updates the action used when handling package results
during upgrade to reset the configuration class that is initially
set.

This fixes a bug when useRememberedArguments is enabled that causes
arguments to be reused in the chocolateyInstall.ps1 when multiple
packages are upgraded. It happens because OptionSet.parse() sets the
configuration via the actions specified in the UpgradeCommand optionSet
setup (and likewise in configuration builder as well). It only sets
options that are saved, so if a later package does not have an option,
it is not set, and the previous option is reused.

This fixes the bug because it resets the configuration at the
ChocolateyPackageService level, which is enough to reset the
configuration for action that runs the PowerShell as well.

Co-authored-by: TheCakeIsNaOH <[email protected]>
AdmiringWorm added a commit to TheCakeIsNaOH/choco that referenced this issue Sep 22, 2022
corbob added a commit that referenced this issue Sep 22, 2022
(#1443) Reset config via action after every package upgrade
@corbob corbob added 4 - Done IN REGRESSION TEST SUITE These are things that are handled by tools like Test-Kitchen and removed 3 - Review ADD AUTO TESTS Things that typically go to Test-Kitchen - once completed, IN REGRESSION TEST SUITE label added labels Sep 22, 2022
@AdmiringWorm
Copy link
Member

🎉 This issue has been resolved in version 1.2.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

@escalonn

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Released Bug IN REGRESSION TEST SUITE These are things that are handled by tools like Test-Kitchen Priority_HIGH
Projects
None yet
Development

Successfully merging a pull request may close this issue.