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

[Firefox] The 64bits detection is overridden in all cases #364

Merged
merged 1 commit into from
Nov 4, 2016

Conversation

wget
Copy link
Member

@wget wget commented Oct 30, 2016

The variable $uninstallPaths gets overridden whatever the 64bits detection could be. Prevent that.

The variable `$uninstallPaths` gets overridden whatever the 64bits detection could be. Prevent that.
@wget wget changed the title The 64bits detection is overridden in all cases [Firefox] The 64bits detection is overridden in all cases Oct 30, 2016
@ferventcoder
Copy link
Contributor

LGTM

@wget
Copy link
Member Author

wget commented Oct 31, 2016

@ferventcoder Obviously, it depends what the maintainer wanted to do here with the first match: either take the first match from the standard location (x86 location on a 32 bits system, 64 bits location on a 64bits system) or take the match from the 32 bits location on a 64 bits system (the SysWoW64 location).

Otherwise we could have just let the line 15 as it and only replace the = by +=. But again, this depends what the maintainer wanted to do. Especially as from the tests I've made:

  • the non SysWow64 location is used
    • on a 32bits system when using the 32 bits version of Firefox;
    • on a 64 bits system when using the 64 bits version of Firefox.
  • the SysWow64 location is used
    • only on a 64 bits system when using the 32 version of Firefox.

I realized if we install firefox on a 64 bits system using Chocolatey, the 64 bits version is installed, and if we install manually the 32 bits version afterwards, Chocolatey using the Auto Uninstaller feature is removing the 64 bits version correctly.

However for some packages I'm currently updating, how can I force my uninstall script to be run when the user left the Auto Uninstaller feature enabled? This feature is rather annoying since the customizations I've made in my install scripts are NOT reverted when the Auto Uninstaller feature is run :-/

Any idea?

@majkinetor
Copy link
Contributor

However for some packages I'm currently updating, how can I force my uninstall script to be run when the user left the Auto Uninstaller feature enabled? This feature is rather annoying since the customizations I've made in my install scripts are NOT reverted when the Auto Uninstaller feature is run :-/

AFAIK, chocolateyUninstall.ps1 runs before auto uninstaller. So do you talk about packages that do not have it ?

@wget
Copy link
Member Author

wget commented Oct 31, 2016

@majkinetor From the tests I performed on my side, it looks like this is not what is happening. Actually I'm still testing locally using cpack then choco uninstall -fdv .\openvpn.nuspec --yes, so I don't know if this changes anything :-/

@majkinetor
Copy link
Contributor

Is there any such package in this repo ? If not, I suggest you to PR one so we can take a look.

Actually I'm still testing locally using cpack then choco uninstall -fdv .\openvpn.nuspec --yes, so I don't know if this changes anything :-/

You should take a look at AU's Test-Package that can test both locally and inside VM which is good for reproducibility on this referent machine.

@wget
Copy link
Member Author

wget commented Oct 31, 2016

@majkinetor Actually this is the package OpenVPN I'm talking about. A dev version of the new version I want to publish is available on my GitHub repo

@gep13
Copy link
Member

gep13 commented Nov 4, 2016

@wget @ferventcoder @majkinetor so, what was the consensus on this? Happy to merge?

@ferventcoder
Copy link
Contributor

@wget I think we determined that chocolateyUninstall.ps1 always runs before autoUninstaller in our conversation. We also found there may be a small bug in autoUninstaller as well. chocolatey/choco#1035

@gep13 This PR LGTM!

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 as well 😄

@gep13 gep13 merged commit 6d5f45b into chocolatey-community:master Nov 4, 2016
@wget
Copy link
Member Author

wget commented Nov 5, 2016

Damn. I had to RTFM to understand what LGTM means :D

@gep13
Copy link
Member

gep13 commented Nov 6, 2016

@wget said...
Damn. I had to RTFM to understand what LGTM means :D

Ha ha! 😄

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.

4 participants