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

(#1443) Reset config via action after every package upgrade #2484

Merged
merged 6 commits into from
Sep 22, 2022

Conversation

TheCakeIsNaOH
Copy link
Member

@TheCakeIsNaOH TheCakeIsNaOH commented Dec 5, 2021

Description Of Changes

This adds a parameter to the upgrade_run method, which
is used to pass an action which resets the configuration in the
function that calls upgrade_run.

Motivation and Context

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.

Testing

  1. Build choco
  2. Run these setup steps:
.\choco.exe feature enable -n useRememberedArgumentsForUpgrades --allow-unofficial -y
.\choco.exe feature enable -n allowGlobalConfirmation --allow-unofficial -y
.\choco.exe install curl --package-parameters="'/CurlOnlyParam'" --version=7.77.0 --allow-unofficial -y -f --ia="'/CurlIAParam'" --forcex86
.\choco.exe install wget --version=1.21.1 --allow-unofficial -y -f

Note that curl has package parameters, install arguments and forcex86 set, while wget does not have any of those.

  1. Run this: .\choco.exe upgrade all --allow-unofficial -y --debug

The curl install script should be run with the package parameters, install arguments and forcex86 similar to this:

& '.\helpers\chocolateyScriptRunner.ps1' -packageScript '.\lib\curl\tools\chocolateyInstall.ps1' -installArguments '/CurlIAParam' -packageParameters '/CurlOnlyParam' -forceX86']

And the wget install script should be run without those arguments:

& '.\helpers\chocolateyScriptRunner.ps1' -packageScript '.\lib\Wget\tools\chocolateyinstall.ps1' -installArguments '' -packageParameters ''']

Change Types Made

  • Bug fix (non-breaking change)
  • Feature / Enhancement (non-breaking change)
  • Breaking change (fix or feature that could cause existing functionality to change)s

Related Issue

Fixes #1443

Change Checklist

  • Requires a change to the documentation
  • Documentation has been updated
  • Tests to cover my changes, have been added
  • All new and existing tests passed.
  • N/A PowerShell v2 compatibility checked.

@coveralls
Copy link

coveralls commented Jun 27, 2022

Coverage Status

Coverage decreased (-0.06%) to 27.583% when pulling 8f15337 on TheCakeIsNaOH:reset-config into 097b52f on chocolatey:develop.

@TheCakeIsNaOH
Copy link
Member Author

It seems like this only remembers bool switches (like --forcex86) for the first package that uses them.

.\choco.exe feature enable -n useRememberedArgumentsForUpgrades --allow-unofficial -y
.\choco.exe feature enable -n allowGlobalConfirmation --allow-unofficial -y
.\choco.exe install curl --package-parameters="'/CurlOnlyParam'" --version=7.77.0 --allow-unofficial -y -f --ia="'/CurlIAParam'" --forcex86
.\choco.exe install wget --version=1.21.1 --allow-unofficial -y -f --forcex86
.\choco.exe upgrade all --allow-unofficial -y --debug

And during that upgrade all, the curl package will extract it's 32 bit archive, but the wget package will extract it's 64 bit archive. The wget package should have the --forcex86 argument set, but it appears like that it was not set.

@AdmiringWorm
Copy link
Member

AdmiringWorm commented Sep 19, 2022

It seems like this only remembers bool switches (like --forcex86) for the first package that uses them.

.\choco.exe feature enable -n useRememberedArgumentsForUpgrades --allow-unofficial -y
.\choco.exe feature enable -n allowGlobalConfirmation --allow-unofficial -y
.\choco.exe install curl --package-parameters="'/CurlOnlyParam'" --version=7.77.0 --allow-unofficial -y -f --ia="'/CurlIAParam'" --forcex86
.\choco.exe install wget --version=1.21.1 --allow-unofficial -y -f --forcex86
.\choco.exe upgrade all --allow-unofficial -y --debug

And during that upgrade all, the curl package will extract it's 32 bit archive, but the wget package will extract it's 64 bit archive. The wget package should have the --forcex86 argument set, but it appears like that it was not set.

Hmm, that sounds like we would maybe want to wait to pull this PR into the repository.

I'll bring it up internally tomorrow to see where we want to go.

EDIT: I have a nagging feeling it is not just boolean switches, but rather everything once it goes to the second package.

@AdmiringWorm
Copy link
Member

I have had a chat internally about this, and we have come up with a solution that I will be trying out.

Expect this PR to be updated with the new code that I have in mind that should allow us to have a clean copy of the Configuration class, while still being able to retain the references that are required to be in place (the references are the identified cause of the issue that makes Chocolatey CLI forget about the needed arguments for further package executions).

@AdmiringWorm AdmiringWorm marked this pull request as draft September 20, 2022 16:32
@AdmiringWorm
Copy link
Member

I have moved this PR into draft, while the code and tests are being worked on to prevent any merges happening

@AdmiringWorm AdmiringWorm force-pushed the reset-config branch 3 times, most recently from 2edef72 to 70a2b07 Compare September 21, 2022 14:27
@AdmiringWorm AdmiringWorm marked this pull request as ready for review September 21, 2022 14:28
Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few spelling points in the comments.

Also a question: Does this need to be updated in chocolatey licensed extension as well?

@AdmiringWorm
Copy link
Member

Also a question: Does this need to be updated in chocolatey licensed extension as well?

No, there is no need to update anything in the Licensed codebase.

@AdmiringWorm AdmiringWorm force-pushed the reset-config branch 2 times, most recently from 4fa6640 to 8d0e7ed Compare September 22, 2022 15:40
AdmiringWorm and others added 5 commits September 22, 2022 17:55
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.
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 <TheCakeIsNaOH@gmail.com>
This commit updates other handlers in the Nuget Service to use
the same approach when resetting configuration class as was
added when upgrading packages.

This allows us to use the same approach in the relevant code paths
without having to rely on manually deep copying the reference and
thus loosing the actual reference to the configuration object.
corbob
corbob previously approved these changes Sep 22, 2022
Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This commit updates the vagrant image and Invoke-Tests to handle
scenarios where tests has been added that should not run outside of
one of the virtual machines that we have available.
@corbob
Copy link
Member

corbob commented Sep 22, 2022

Thanks for getting this added @AdmiringWorm. I'll let the GitHub Actions finish, then kick the one that failed and once that's all green I'll get this merged 😁

@corbob corbob merged commit 3892cfb into chocolatey:develop Sep 22, 2022
@TheCakeIsNaOH TheCakeIsNaOH deleted the reset-config branch January 27, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade all reuses overridden package parameters when useRememberedArgumentsForUpgrades feature is turned on
4 participants