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

(#2345) Avoid any trailing spaces after silentArgs #3113

Merged
merged 6 commits into from
May 23, 2024

Conversation

blami
Copy link
Contributor

@blami blami commented Apr 12, 2023

Description Of Changes

Fix a bug where concatenating $silentArgs and $additionalInstallArgs adds a trailing space at the end of $silentArgs even when $additionalInstallArgs is empty. Such trailing space can change the meaning of the last positional argument (that may contain spaces).

Final arguments are now created as space joined string arrays filtered down to only non-empty items. Fix is applied to all installer types.

Motivation and Context

I have some installers doing similar thing as Total Commander (mentioned in #2345 ) - passing installation destination as last positional argument allowing spaces in it. Added trailing space caused by "$silentArgs $additionalInstallArgs" when $additionalInstallArgs is empty breaks this mechanism and attempts to install to path that ends with space.

NOTE: I am just an ordinary PowerShell user and not really skilled in it so any input on how to get this thing done is welcome. Also wasn't sure if its worth to apply this beyond EXE type as MSI has its own way of passing arguments. Motivation to fix all types is mostly uniformity.

Testing

  1. Tested installation of several .exe installers (made up mocks and Total Commander) on Windows 11
  2. @corbob did some additional testing: (#2345) Avoid any trailing spaces after silentArgs #3113 (comment)
  3. @corbob also added some Pester tests that were tested both in Team City, and by running through the testing vagrant environment.

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).
  • Documentation changes.
  • PowerShell code changes.

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?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Fixes: #2345

@blami blami changed the title Avoid any trailing spaces after silentArgs (#2345) Avoid any trailing spaces after silentArgs Apr 18, 2023
@corbob corbob requested a review from gep13 May 6, 2024 13:52
corbob
corbob previously approved these changes May 6, 2024
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.

Thank you for getting this fixed up @blami. I've taken the liberty of adding a Pester test to this PR, rebased off of develop, then force pushed the change up to your fork. As I've added the Pester tests, I've asked for a review from @gep13 as well.

@gep13: The new Pester tests will fail in Test Kitchen as I have not yet pushed the package to the repositories used by Test Kitchen. Once the PR is merged I'll get those other repositories updated.

@corbob
Copy link
Member

corbob commented May 14, 2024

@blami I've been trying to come up with a manual test to show that this fixes a broken package installation. Unfortunately, I haven't been able to find a version of TotalCommander on the Chocolatey Community Repository that exhibits this behaviour. Do you have a package available that I could use to demonstrate that this fix resolves the issue with that package? The pester tests I added show that the space is gone, but having another verification would be very helpful.

@blami
Copy link
Contributor Author

blami commented May 19, 2024

@corbob Thanks a lot for picking up this! I just looked at Total Commander package in community and it should be still broken. This bug only surfaces if you set install location parameter.

I cannot share our private repo but package excerpt that requires the fix is roughly:

$ErrorActionPreference = 'Stop' # stop on all errors
$toolsDir   = "$(Split-Path -parent $MyInvocation.MyCommand.Definition)"
$url        = 'https://totalcommander.ch/1052/tcmd1052x64.exe'

$packageArgs = @{
  packageName   = $env:ChocolateyPackageName
  unzipLocation = $toolsDir
  fileType      = 'EXE'
  url           = $url
  softwareName  = 'Total Commander*'

  # https://www.ghisler.ch/wiki/index.php?title=Installer#Description_of_switches_and_parameters
  silentArgs    = '/A1H1M0G1D0FN"*" U:\Program Files\TotalCmd'

  validExitCodes= @(0)
}

Install-ChocolateyPackage @packageArgs

Hope this helps.

@corbob
Copy link
Member

corbob commented May 21, 2024

@blami Thank you for that script. I was able to reproduce the issue, and confirm that the updates made here resolve the issue.

For completeness, my reproduction/verification steps:

  1. Create a package with the chocolateyInstall.ps1 script supplied.
  2. Modify the target directory to C:\Program TotalCmd as I don't have any drives other than C:.
  3. choco install tc -s c:\packages\tc
  4. Note that the output says it was installed to C:\Program TotalCmd \
  5. Check the directory created.
  6. Note that it doesn't have a space.
  7. Check the contents of the directory.
  8. Note that it is empty.
  9. Check the Registry for uninstall data for Total Commander.
  10. Note that all paths have the trailing space.
  11. choco uninstall tc
  12. Upgrade to Chocolatey CLI built off this PR.
  13. Repeat step 3
  14. Note the output now says C:\Program TotalCmd\
  15. Check the directory.
  16. Note it contains files.
  17. Check the registry entry for Total Commander again.
  18. Note that the space is gone.

Screenshot of the Chocolatey 2.2.2 behaviour:

image

blami and others added 3 commits May 22, 2024 10:05
Fix a bug where concatenating silentArgs and additionalInstallArgs adds
a trailing space at the end of silentArgs even when
additionalInstallArgs is empty. Such trailing space can change the
meaning of the last positional argument (that may contain spaces).

Final arguments are now created as space joined string arrays filtered
down to only non-empty items. Fix is applied to all installer types.

It is expected not to manipulate the arguments themselves in case there
are any spaces that the package maintainer has included.
This adds a set of tests to validate that the spaces that may be passed
in by the package are honoured, while not introducing erroneous spaces.
During pairing with gep13, discovered that some of the Pester tests
failed in Team City as they are likely waiting for the user to confirm
running the PowerShell scripts as the certificate used to sign choco.exe
differs from the ps1 files on development builds. This updates the tests
to trust the certificate of the ps1 files as well so that the prompt
doesn't happen.
Enhance the local testing process for developers. Instead of building
all nuspec files we find, filter out ones that may have been built as
part of previous tests.

Update the vagrantfile to copy over changed files when running the test
provisioner, but only purge the tests directory.

Update the vagrantfile to set a hostname on the VM to prevent on-screen
error messages that don't seem to affect anything.
As part of the recent renewal of the Chocolatey Certificate, the
Subject name changed, so capturing the change here as another possible
signature to check for.
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13
Copy link
Member

gep13 commented May 23, 2024

@blami thank you for raising this PR, your first to Chocolatey CLI, and thanks to @corbob thank you for getting this over the finish line!

@gep13 gep13 merged commit 872fe03 into chocolatey:develop May 23, 2024
5 checks passed
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.

Ensure Install-ChocolateyInstallPackage does not add a trailing space to silent arguments
3 participants