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

(adobereader) Using /IgnoreInstalled reports matched packages multiple times #230

Open
pauby opened this issue Jul 18, 2024 · 5 comments
Open
Labels
enhancement An enhancement to the package functionality.

Comments

@pauby
Copy link
Owner

pauby commented Jul 18, 2024

See this Disqus comment:

There is a bug in # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled' that multiplies the found installs by the number of not matching IgnoreInstalled, e.g 8 matching installs instead of 2 (and will also prevent reaching 1 matchig installs after processing other configurations). I defects the result for all multiple IgnoreInstalled.

    /IgnoreInstalled package parameter was passed with these software names:
    - Adobe Acrobat X Pro*
    - Adobe Acrobat XI Pro
    - Adobe Acrobat DC (2015)
    - Adobe Acrobat 2017
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat X Pro*)'
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat XI Pro)'
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat DC (2015))'
    Keeping 'Adobe Acrobat 2020' as it does not match 'Adobe Acrobat 2017)'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat X Pro*)'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat XI Pro)'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat DC (2015))'
    Keeping 'Adobe Acrobat (64-bit)' as it does not match 'Adobe Acrobat 2017)'
    After processing, we will use this list of installed software:
    - Adobe Acrobat 2020
    - Adobe Acrobat 2020
    - Adobe Acrobat 2020
    - Adobe Acrobat 2020
    - Adobe Acrobat (64-bit)
    - Adobe Acrobat (64-bit)
    - Adobe Acrobat (64-bit)
    - Adobe Acrobat (64-bit)
    8 matching installs of Adobe Acrobat Reader DC found!
    To prevent accidental data loss, this install will be aborted.

Maybe modify the code to something like the following?

    # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'
    $key = for ($i = 0; $i -lt $installation.count; $i++) {
    $KEEP = $true
    for ($j = 0; $j -lt $matchInstallation.count; $j++) {
    if ($installation[$i].DisplayName -notlike $matchInstallation[$j]) {
    Write-Verbose "Keeping '$($installation[$i].DisplayName)' as it does not match '$($matchInstallation[$j]))'"
    }
    else {
    $KEEP = $false
    Write-Verbose "Removing '$($installation[$i].DisplayName)' from list of found software, as it matches '$($matchInstallation[$j]))'"
    }
    }
    If ($KEEP) {
    $installation[$i]
    }
    }
Copy link

Thanks for raising this issue!

The packages within this repository are maintained in my spare time. My spare time, like yours is important. Please help me not to waste it.

To help me, and to have the issue resolved more quickly, please see CONTRIBUTING for how to raise a pull request to resolve the issue yourself.

Thank you.

@sf0-k46
Copy link

sf0-k46 commented Jul 18, 2024

Suggestion:

# Since -notlike does not compare a single String to an array of patterns use -notmatch.
# Convert the $matchInstallation-array from a -like- to a -match-syntax in a single string to make it sematically identical by ...
#   - replacing '*' with '.*'
#   - frame the elements with '^' and '$'
#   - delimit elements with '|'
# E.g. 'Adobe Acrobat X Pro*', 'Adobe Acrobat XI Pro'
#   -> '^Adobe Acrobat X Pro.*$|^Adobe Acrobat XI Pro$'
$matchInstallationPattern = ('^' + (($matchInstallation -replace '\*','.*') -join '$|^') + '$')

$key = $installation `
| Where-Object `
	-FilterScript {
		$_.DisplayName -notmatch $matchInstallationPattern
	}

@pauby
Copy link
Owner Author

pauby commented Jul 18, 2024

What would be the purpose of using that code over what is there?

@sf0-k46
Copy link

sf0-k46 commented Jul 18, 2024

The code whats is there

  • Outputs the current $installation for each $matchInstallation that is not like it. When using at least two different elements in $matchInstallation this does lead to never removing any element of $installation since every element is outputted at least one time.
  • The Elements get outputted multiple times so ($key.Count -eq 1) in line 70 can never be true unless only one element is given in /IgnoreInstalled and can really ignore an entry.

So the code what is there does not ignore installations when multiple elements are given in /IgnoreInstalled despite the purpose of that block doing so.

The suggested code does # loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'.
Looping is done by $installation | Where-Object, ignoring by comparing the DisplayName with each of the values in /IgnoreInstalled by using -notmatch and a pattern that combines the values by using | to OR them.

@sf0-k46
Copy link

sf0-k46 commented Jul 19, 2024

The following code tests the results of the three code variants on a common installation set with a common /IgnoreInstalled configuration.

# <https://github.com/pauby/ChocoPackages/issues/230>

# Iterate over the three code versions
@(1, 2, 3) | ForEach-Object {

	# set up the used variables for each version

	# existent typed variables can influence the result
	Remove-Variable -Name @('installation', 'matchInstallation', 'key')

	# Test case: Current parallel installation
	[array]$installation = @(
		[PSCustomObject]@{
			DisplayName = 'Adobe Acrobat (64-bit)'
		}
		[PSCustomObject]@{
			DisplayName = 'Adobe Acrobat 2020'
		}
	)

	# Test case: More than one software to ignore
	$matchInstallation = @(
		'Adobe Acrobat 2020'
		'SomeOtherProduct'
	)

	"Running variant $_"

	switch ($_) {

		# the code that is there
		# <https://github.com/pauby/ChocoPackages/blob/4184359f216e4126ec054a84047c6451bad406c9/automatic/adobereader/tools/chocolateyinstall.ps1#L41>
		1 {
			# loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'
			$key = for ($i = 0; $i -lt $installation.count; $i++) {
				for ($j = 0; $j -lt $matchInstallation.count; $j++) {
					if ($installation[$i].DisplayName -notlike $matchInstallation[$j]) {
						$installation[$i]
						Write-Verbose "Keeping '$($installation[$i].DisplayName)' as it does not match '$($matchInstallation[$j])'"
					}
					else {
						Write-Verbose "Removing '$($installation[$i].DisplayName)' from list of found software, as it matches '$($matchInstallation[$j])'"
					}
				}
			}
		}

		# Disqus-comment
		# <http://disq.us/p/2zlyuib>
		2 {
			# loop over each found installation and ignore it if it matches a value in '/IgnoreInstalled'
			$key = for ($i = 0; $i -lt $installation.count; $i++) {
				$KEEP = $true
				for ($j = 0; $j -lt $matchInstallation.count; $j++) {
					if ($installation[$i].DisplayName -notlike $matchInstallation[$j]) {
						Write-Verbose "Keeping '$($installation[$i].DisplayName)' as it does not match '$($matchInstallation[$j])'"
					}
					else {
						$KEEP = $false
						Write-Verbose "Removing '$($installation[$i].DisplayName)' from list of found software, as it matches '$($matchInstallation[$j])'"
					}
				}
				If ($KEEP) {
					$installation[$i]
				}
			}

			Write-Verbose "After processing, we will use this list of installed software:"
			$key | ForEach-Object {
				Write-Warning "- $($_.DisplayName)"
			}
		}

		# GitHub-issue
		# <https://github.com/pauby/ChocoPackages/issues/230#issuecomment-2237235529>
		3 {
			# Since -notlike does not compare a single String to an array of patterns use -notmatch.
			# Convert the $matchInstallation-array from a -like- to a -match-syntax in a single string to make it sematically identical by ...
			#   - replacing '*' with '.*'
			#   - frame the elements with '^' and '$'
			#   - delimit elements with '|'
			# E.g. 'Adobe Acrobat X Pro*', 'Adobe Acrobat XI Pro'
			#   -> '^Adobe Acrobat X Pro.*$|^Adobe Acrobat XI Pro$'
			$matchInstallationPattern = ('^' + (($matchInstallation -replace '\*','.*') -join '$|^') + '$')

			[array]$key = $installation `
			| Where-Object `
				-FilterScript {
					$_.DisplayName -notmatch $matchInstallationPattern
				}
		}
	}

	$key
	"Type: $($key.GetType().BaseType.Name)"
	"Count: $($key.Count)"

	if ($key.Count -eq 1) {
		"CONTINUE"
	} else {
		'ABORT'
	}

	''

}

Variant 1 does not filter correctly, always aborts:

Running variant 1

DisplayName           
-----------           
Adobe Acrobat (64-bit)
Adobe Acrobat (64-bit)
Adobe Acrobat 2020    
Type: Array
Count: 3
ABORT

Variant 2 was a minimal quick-fix that filters correctly but test shows that it outputs the wrong type, so that it always aborts. Maybe this is due to the test code but as it mainly reordered the output of variant 1 that could be an indication that variant 1 is not type safe also. An explicit type case would correct this:

Running variant 2
Adobe Acrobat (64-bit)
Type: Object
Count: 
ABORT

Variant 3 is a code refacturing and does filter correctly. But to make it type safe an explicit typecast to [array] has been added compared to the previous suggestion:

Running variant 3
Adobe Acrobat (64-bit)
Type: Array
Count: 1
CONTINUE

@pauby pauby added the enhancement An enhancement to the package functionality. label Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement to the package functionality.
Projects
None yet
Development

No branches or pull requests

2 participants