From 7e000cf77424411233038725dcd81826572167f9 Mon Sep 17 00:00:00 2001 From: AdmiringWorm Date: Tue, 20 Sep 2022 18:25:48 +0200 Subject: [PATCH 1/6] (#1443) Add ability to reset config with no ref lost 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. --- .../configuration/ChocolateyConfiguration.cs | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/src/chocolatey/infrastructure.app/configuration/ChocolateyConfiguration.cs b/src/chocolatey/infrastructure.app/configuration/ChocolateyConfiguration.cs index 5a36efb131..549ac89cb8 100644 --- a/src/chocolatey/infrastructure.app/configuration/ChocolateyConfiguration.cs +++ b/src/chocolatey/infrastructure.app/configuration/ChocolateyConfiguration.cs @@ -30,6 +30,9 @@ namespace chocolatey.infrastructure.app.configuration [Serializable] public class ChocolateyConfiguration { + [NonSerialized] + private ChocolateyConfiguration _originalConfiguration; + public ChocolateyConfiguration() { RegularOutput = true; @@ -58,6 +61,86 @@ public ChocolateyConfiguration() #endif } + /// + /// Creates a backup of the current version of the configuration class. + /// + /// One or more objects in the class or child classes are not serializable. + public void start_backup() + { + // We do this the easy way to ensure that we have a clean copy + // of the original configuration file. + _originalConfiguration = this.deep_copy(); + } + + /// + /// Restore the backup that has previously been created to the initial + /// state, without making the class reference types the same to prevent + /// the initial configuration class being updated at the same time if a + /// value changes. + /// + /// Whether a backup that was previously made should be removed after resetting the configuration. + /// No backup has been created before trying to reset the current configuration, and removal of the backup was not requested. + /// + /// This call may make quite a lot of allocations on the Gen0 heap, as such + /// it is best to keep the calls to this method at a minimum. + /// + public void reset_config(bool removeBackup = false) + { + if (_originalConfiguration == null) + { + if (removeBackup) + { + // If we will also be removing the backup, we do not care if it is already + // null or not, as that is the intended state when this method returns. + return; + } + + throw new InvalidOperationException("No backup has been created before trying to reset the current configuration, and removal of the backup was not requested."); + } + + var t = this.GetType(); + + foreach (var property in t.GetProperties(BindingFlags.Public | BindingFlags.Instance)) + { + try + { + var originalValue = property.GetValue(_originalConfiguration, new object[0]); + + if (removeBackup || property.DeclaringType.IsPrimitive || property.DeclaringType.IsValueType || property.DeclaringType == typeof(string)) + { + // If the property is a primitive, a value type or a string, then a copy of the value + // will be created by the .NET Runtime automatically, and we do not need to create a deep clone of the value. + // Additionally, if we will be removing the backup there is no need to create a deep copy + // for any reference types, as such we also set the reference itself so it is not needed + // to allocate more memory. + property.SetValue(this, originalValue, new object[0]); + } + else if (originalValue != null) + { + // We need to do a deep copy of the value so it won't copy the reference itself, + // but rather the actual values we are interested in. + property.SetValue(this, originalValue.deep_copy(), new object[0]); + } + else + { + property.SetValue(this, null, new object[0]); + } + } + catch (Exception ex) + { + throw new ApplicationException("Unable to restore the value for the property '{0}'.".format_with(property.Name), ex); + } + } + + if (removeBackup) + { + // It is enough to set the original configuration to null to + // allow GC to clean it up the next time it runs on the stored Generation + // Heap Table. + _originalConfiguration = null; + } + } + // overrides public override string ToString() { @@ -237,6 +320,7 @@ private void append_output(StringBuilder propertyValues, string append) [Obsolete("Side by Side installation is deprecated, and is pending removal in v2.0.0")] public bool AllowMultipleVersions { get; set; } + public bool AllowDowngrade { get; set; } public bool ForceDependencies { get; set; } public string DownloadChecksum { get; set; } From 1f06cd7f125ba66aff6402f186b1ab2ffb9c1777 Mon Sep 17 00:00:00 2001 From: AdmiringWorm Date: Fri, 16 Sep 2022 18:02:59 +0200 Subject: [PATCH 2/6] (#1443) Reset config after handling package results 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 --- .../services/ChocolateyPackageService.cs | 2 +- .../infrastructure.app/services/NugetService.cs | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs b/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs index b62d9c5106..8c08b1268c 100644 --- a/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs +++ b/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs @@ -1,4 +1,4 @@ -// Copyright © 2017 - 2022 Chocolatey Software, Inc +// Copyright © 2017 - 2022 Chocolatey Software, Inc // Copyright © 2011 - 2017 RealDimensions Software, LLC // // Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/src/chocolatey/infrastructure.app/services/NugetService.cs b/src/chocolatey/infrastructure.app/services/NugetService.cs index 64d3807e29..d738ffedcb 100644 --- a/src/chocolatey/infrastructure.app/services/NugetService.cs +++ b/src/chocolatey/infrastructure.app/services/NugetService.cs @@ -1,4 +1,4 @@ -// Copyright © 2017 - 2022 Chocolatey Software, Inc +// Copyright © 2017 - 2022 Chocolatey Software, Inc // Copyright © 2011 - 2017 RealDimensions Software, LLC // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -621,12 +621,13 @@ public virtual ConcurrentDictionary upgrade_run(Chocolate set_package_names_if_all_is_specified(config, () => { config.IgnoreDependencies = true; }); config.IgnoreDependencies = configIgnoreDependencies; - var originalConfig = config.deep_copy(); + config.start_backup(); foreach (string packageName in config.PackageNames.Split(new[] { ApplicationParameters.PackageNamesSeparator }, StringSplitOptions.RemoveEmptyEntries).or_empty_list_if_null()) { - // reset config each time through - config = originalConfig.deep_copy(); + // We need to ensure we are using a clean configuration file + // before we start reading it. + config.reset_config(); IPackage installedPackage = packageManager.LocalRepository.FindPackage(packageName); @@ -878,6 +879,11 @@ public virtual ConcurrentDictionary upgrade_run(Chocolate } } + // Reset the configuration again once we are completely done with the processing of + // configurations, and make sure that we are removing any backup that was created + // as part of this run. + config.reset_config(removeBackup: true); + return packageInstalls; } From 0b1cfafc8c35cb563163a31ccaf11d6acf4b7602 Mon Sep 17 00:00:00 2001 From: AdmiringWorm Date: Tue, 20 Sep 2022 18:27:08 +0200 Subject: [PATCH 3/6] (maint) Update install/uninstall to new reset handler 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. --- .../services/NugetService.cs | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/chocolatey/infrastructure.app/services/NugetService.cs b/src/chocolatey/infrastructure.app/services/NugetService.cs index d738ffedcb..736bad07d8 100644 --- a/src/chocolatey/infrastructure.app/services/NugetService.cs +++ b/src/chocolatey/infrastructure.app/services/NugetService.cs @@ -436,13 +436,13 @@ public virtual ConcurrentDictionary install_run(Chocolate uninstallSuccessAction: null, addUninstallHandler: true); - var originalConfig = config.deep_copy(); + config.start_backup(); foreach (string packageName in packageNames.or_empty_list_if_null()) { - // reset config each time through - config = originalConfig.deep_copy(); - + // We need to ensure we are using a clean configuration file + // before we start reading it. + config.reset_config(); //todo: #2577 get smarter about realizing multiple versions have been installed before and allowing that IPackage installedPackage = packageManager.LocalRepository.FindPackage(packageName); @@ -555,6 +555,11 @@ Version was specified as '{0}'. It is possible that version } } + // Reset the configuration again once we are completely done with the processing of + // configurations, and make sure that we are removing any backup that was created + // as part of this run. + config.reset_config(removeBackup: true); + return packageInstalls; } @@ -903,12 +908,13 @@ public virtual ConcurrentDictionary get_outdated(Chocolat set_package_names_if_all_is_specified(config, () => { config.IgnoreDependencies = true; }); var packageNames = config.PackageNames.Split(new[] { ApplicationParameters.PackageNamesSeparator }, StringSplitOptions.RemoveEmptyEntries).or_empty_list_if_null().ToList(); - var originalConfig = config.deep_copy(); + config.start_backup(); foreach (var packageName in packageNames) { - // reset config each time through - config = originalConfig.deep_copy(); + // We need to ensure we are using a clean configuration file + // before we start reading it. + config.reset_config(); var installedPackage = packageManager.LocalRepository.FindPackage(packageName); var pkgInfo = _packageInfoService.get_package_information(installedPackage); @@ -967,6 +973,11 @@ public virtual ConcurrentDictionary get_outdated(Chocolat } } + // Reset the configuration again once we are completely done with the processing of + // configurations, and make sure that we are removing any backup that was created + // as part of this run. + config.reset_config(removeBackup: true); + return outdatedPackages; } @@ -1380,12 +1391,13 @@ public virtual ConcurrentDictionary uninstall_run(Chocola config.ForceDependencies = false; }); - var originalConfig = config.deep_copy(); + config.start_backup(); foreach (string packageName in config.PackageNames.Split(new[] { ApplicationParameters.PackageNamesSeparator }, StringSplitOptions.RemoveEmptyEntries).or_empty_list_if_null()) { - // reset config each time through - config = originalConfig.deep_copy(); + // We need to ensure we are using a clean configuration file + // before we start reading it. + config.reset_config(); IList installedPackageVersions = new List(); if (string.IsNullOrWhiteSpace(config.Version)) @@ -1513,6 +1525,11 @@ public virtual ConcurrentDictionary uninstall_run(Chocola } } + // Reset the configuration again once we are completely done with the processing of + // configurations, and make sure that we are removing any backup that was created + // as part of this run. + config.reset_config(removeBackup: true); + return packageUninstalls; } From 3164f5edf6638781a885d22c7ab62951f76148e8 Mon Sep 17 00:00:00 2001 From: AdmiringWorm Date: Tue, 20 Sep 2022 18:29:48 +0200 Subject: [PATCH 4/6] (maint) Whitespace + Formatting fix --- .../services/ChocolateyPackageService.cs | 8 ++++---- .../infrastructure.app/services/NugetService.cs | 7 +++---- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs b/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs index 8c08b1268c..516768d5b6 100644 --- a/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs +++ b/src/chocolatey/infrastructure.app/services/ChocolateyPackageService.cs @@ -1,4 +1,4 @@ -// Copyright © 2017 - 2022 Chocolatey Software, Inc +// Copyright © 2017 - 2022 Chocolatey Software, Inc // Copyright © 2011 - 2017 RealDimensions Software, LLC // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -22,7 +22,6 @@ namespace chocolatey.infrastructure.app.services using System.IO; using System.Linq; using System.Text; - using builders; using commandline; using configuration; using domain; @@ -31,8 +30,8 @@ namespace chocolatey.infrastructure.app.services using infrastructure.events; using infrastructure.services; using logging; - using NuGet; using nuget; + using NuGet; using platforms; using results; using tolerance; @@ -91,6 +90,7 @@ system admins into something amazing! Singlehandedly solve your organization's struggles with software management and save the day! https://chocolatey.org/compare" }; + private const string PRO_BUSINESS_LIST_MESSAGE = @" Did you know Pro / Business automatically syncs with Programs and Features? Learn more about Package Synchronizer at @@ -742,7 +742,7 @@ private IEnumerable get_packages_from_config(string pac if (pkgSettings.IgnoreDependencies) packageConfig.IgnoreDependencies = true; if (pkgSettings.ApplyInstallArgumentsToDependencies) packageConfig.ApplyInstallArgumentsToDependencies = true; if (pkgSettings.ApplyPackageParametersToDependencies) packageConfig.ApplyPackageParametersToDependencies = true; - + if (!string.IsNullOrWhiteSpace(pkgSettings.Source) && has_source_type(pkgSettings.Source)) { packageConfig.SourceType = pkgSettings.Source; diff --git a/src/chocolatey/infrastructure.app/services/NugetService.cs b/src/chocolatey/infrastructure.app/services/NugetService.cs index 736bad07d8..651cd84e68 100644 --- a/src/chocolatey/infrastructure.app/services/NugetService.cs +++ b/src/chocolatey/infrastructure.app/services/NugetService.cs @@ -1,4 +1,4 @@ -// Copyright © 2017 - 2022 Chocolatey Software, Inc +// Copyright © 2017 - 2022 Chocolatey Software, Inc // Copyright © 2011 - 2017 RealDimensions Software, LLC // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -19,25 +19,24 @@ namespace chocolatey.infrastructure.app.services using System; using System.Collections.Concurrent; using System.Collections.Generic; - using System.Globalization; using System.IO; using System.Linq; using System.Net; - using NuGet; using adapters; + using chocolatey.infrastructure.app.utility; using commandline; using configuration; using domain; using guards; using logging; using nuget; + using NuGet; using platforms; using results; using tolerance; using DateTime = adapters.DateTime; using Environment = System.Environment; using IFileSystem = filesystem.IFileSystem; - using chocolatey.infrastructure.app.utility; //todo: #2575 - this monolith is too large. Refactor once test coverage is up. From b19e02e0045a5e235cc8b33b64e40a6c6ed62190 Mon Sep 17 00:00:00 2001 From: AdmiringWorm Date: Tue, 20 Sep 2022 18:30:20 +0200 Subject: [PATCH 5/6] (#1443) Add E2E tests for remember upgrade args --- .../chocolatey-tests/choco-upgrade.Tests.ps1 | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/tests/chocolatey-tests/choco-upgrade.Tests.ps1 b/tests/chocolatey-tests/choco-upgrade.Tests.ps1 index 269ee5f0de..92ff4a403c 100644 --- a/tests/chocolatey-tests/choco-upgrade.Tests.ps1 +++ b/tests/chocolatey-tests/choco-upgrade.Tests.ps1 @@ -119,4 +119,92 @@ Describe "choco upgrade" -Tag Chocolatey, UpgradeCommand { $Output.Lines | Should -Contain "Chocolatey upgraded 1/1 packages." -Because $Output.String } } + + Context "Upgrading packages while remembering arguments with only one package using arguments" { + BeforeAll { + Restore-ChocolateyInstallSnapshot + + $null = Enable-ChocolateyFeature useRememberedArgumentsForUpgrades + $null = Invoke-Choco install curl --package-parameters="'/CurlOnlyParam'" --version="7.77.0" --ia="'/CurlIAParam'" --x86 -y + $null = Invoke-Choco install wget --version=1.21.1 -y + + $Output = Invoke-Choco upgrade all -y --debug + } + + It 'Exits with Success (0)' { + $Output.ExitCode | Should -Be 0 + } + + It 'Outputs running curl script with correct arguments' { + $line = $Output.Lines | Where-Object { $_ -match "packageScript.*curl\\tools" } | Select-Object -Last 1 + + $line | Should -Not -BeNullOrEmpty + $line | Should -MatchExactly "\/CurlIAParam" + $line | Should -MatchExactly "\/CurlOnlyParam" + $line | Should -Match "-forceX86" + } + + It 'Outputs running wget script with correct arguments' { + $line = $Output.Lines | Where-Object { $_ -match "packageScript.*wget\\tools" } + + $line | Should -Not -BeNullOrEmpty + $line | Should -Match "installArguments:? ''" + $line | Should -Match "packageParameters:? ''" + $line | Should -Not -Match "-forceX86" + } + } + + # We exclude this test when running CCM, as it will install and remove + # the firefox package which is used through other tests that will be affected. + Context "Upgrading packages while remembering arguments with multiple packages using arguments" -Tag CCMExcluded { + BeforeAll { + Restore-ChocolateyInstallSnapshot + + $null = Enable-ChocolateyFeature useRememberedArgumentsForUpgrades + $null = Invoke-Choco install curl --package-parameters="'/CurlOnlyParam'" --version="7.77.0" --ia="'/CurlIAParam'" --forcex86 -y + $null = Invoke-Choco install wget --version=1.21.1 -y --forcex86 + $null = Invoke-Choco install firefox --version=99.0.1 --package-parameters="'/l=eu'" -y --ia="'/RemoveDistributionDir=true'" + + $Output = Invoke-Choco upgrade all -y --debug + } + + AfterAll { + $null = Invoke-Choco uninstall firefox -y + } + + It 'Exits with Success (0)' { + $Output.ExitCode | Should -Be 0 + } + + It 'Outputs running curl script with correct arguments' { + $line = $Output.Lines | Where-Object { $_ -match "packageScript.*curl\\tools" } | Select-Object -Last 1 + + $line | Should -Not -BeNullOrEmpty + $line | Should -Match "installArguments:? '/CurlIAParam'" + $line | Should -Match "packageParameters:? '/CurlOnlyParam'" + $line | Should -Match "-forceX86" + } + + It 'Outputs running wget script with correct arguments' { + $line = $Output.Lines | Where-Object { $_ -match "packageScript.*wget\\tools" } | Select-Object -Last 1 + + $line | Should -Not -BeNullOrEmpty + $line | Should -Match "installArguments:? ''" + $line | Should -Match "packageParameters:? ''" + $line | Should -Match "-forceX86" + } + + It 'Outputs firefox using eu as language locale' { + $Output.Lines | Should -Contain "Using locale 'eu'..." + } + + It 'Outputs running firefox script with correct arguments' { + $line = $Output.Lines | Where-Object { $_ -match "packageScript.*firefox\\tools" } + + $line | Should -Not -BeNullOrEmpty + $line | Should -Match "installArguments:? '\/RemoveDistributionDir=true'" + $line | Should -Match "packageParameters:? '\/l=eu'" + $line | Should -Not -Match "-forceX86" + } + } } From 8f153377768b765ecad2b7a1879909afdfc79d36 Mon Sep 17 00:00:00 2001 From: AdmiringWorm Date: Thu, 22 Sep 2022 17:39:41 +0200 Subject: [PATCH 6/6] (maint) Add ability to skip tests when not running in VM 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. --- Invoke-Tests.ps1 | 12 +++++++++++- tests/Vagrantfile | 1 + tests/chocolatey-tests/choco-upgrade.Tests.ps1 | 6 +++--- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/Invoke-Tests.ps1 b/Invoke-Tests.ps1 index 95fc45f412..b11fb8524c 100644 --- a/Invoke-Tests.ps1 +++ b/Invoke-Tests.ps1 @@ -93,7 +93,17 @@ try { Verbosity = 'Minimal' } Filter = @{ - ExcludeTag = 'Background', 'Licensed', 'CCM', 'WIP', 'NonAdmin', 'Internal' + ExcludeTag = @( + 'Background' + 'Licensed' + 'CCM' + 'WIP' + 'NonAdmin' + 'Internal' + if (-not $env:VM_RUNNING -and -not $env:TEST_KITCHEN) { + 'VMOnly' + } + ) } } diff --git a/tests/Vagrantfile b/tests/Vagrantfile index 2b2e4b2149..fec687b61a 100644 --- a/tests/Vagrantfile +++ b/tests/Vagrantfile @@ -60,6 +60,7 @@ Vagrant.configure("2") do |config| Write-Host "Build complete. Executing tests." # $env:TEST_KITCHEN = 1 + $env:VM_RUNNING = 1 ./Invoke-Tests.ps1 SHELL end diff --git a/tests/chocolatey-tests/choco-upgrade.Tests.ps1 b/tests/chocolatey-tests/choco-upgrade.Tests.ps1 index 92ff4a403c..25b8d785fc 100644 --- a/tests/chocolatey-tests/choco-upgrade.Tests.ps1 +++ b/tests/chocolatey-tests/choco-upgrade.Tests.ps1 @@ -120,7 +120,7 @@ Describe "choco upgrade" -Tag Chocolatey, UpgradeCommand { } } - Context "Upgrading packages while remembering arguments with only one package using arguments" { + Context "Upgrading packages while remembering arguments with only one package using arguments" -Tag Internal { BeforeAll { Restore-ChocolateyInstallSnapshot @@ -156,7 +156,7 @@ Describe "choco upgrade" -Tag Chocolatey, UpgradeCommand { # We exclude this test when running CCM, as it will install and remove # the firefox package which is used through other tests that will be affected. - Context "Upgrading packages while remembering arguments with multiple packages using arguments" -Tag CCMExcluded { + Context "Upgrading packages while remembering arguments with multiple packages using arguments" -Tag CCMExcluded, Internal, VMOnly { BeforeAll { Restore-ChocolateyInstallSnapshot @@ -195,7 +195,7 @@ Describe "choco upgrade" -Tag Chocolatey, UpgradeCommand { } It 'Outputs firefox using eu as language locale' { - $Output.Lines | Should -Contain "Using locale 'eu'..." + $Output.Lines | Should -Contain "Using locale 'eu'..." -Because $Output.String } It 'Outputs running firefox script with correct arguments' {