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

(#89) Remove cpack shim for choco pack #2551

Merged
merged 2 commits into from
Mar 14, 2022
Merged

Conversation

gep13
Copy link
Member

@gep13 gep13 commented Jan 22, 2022

There are known incompatibilities with this shim when other applications
are installed on the machine, for example cmake. Since we are moving
towards usage of a ubiquitous choco command, removing this shim makes
a lot of sense.

Fixes #89

@corbob
Copy link
Member

corbob commented Feb 1, 2022

Should there be an upgrade path that removed the cpack.exe shim?

@gep13
Copy link
Member Author

gep13 commented Feb 1, 2022

@corbob I am not sure if I follow what you mean here. Can you elaborate?

@corbob
Copy link
Member

corbob commented Feb 1, 2022

So if I take the nupkg to a fresh system and install it, cpack.exe is not there. But if I already have Chocolatey on the box and do choco upgrade chocolatey cpack.exe is still on the path.

@TheCakeIsNaOH
Copy link
Member

cpack.exe is still on the path.

What if a user replaced the cpack.exe that points to choco pack and instead has a shim that points to the cmake cpack?

Or what if a user manually created a cpack.exe that points to choco pack, since they wanted the functionality back? If there was code that removed the shim, then that manually created shim might be removed on upgrade.

Although removing the shim still might be the best course of action, there are lots of edge cases.

@gep13
Copy link
Member Author

gep13 commented Feb 4, 2022

@TheCakeIsNaOH said...
Although removing the shim still might be the best course of action, there are lots of edge cases.

Agreed. There is a long standing "rule" that Chocolatey package installations shouldn't directly we adding anything into the Chocolatey bin folder, and the same rule should apply to folks directly adding files in there. Obviously we can't stop someone doing this if they really want to, but they can't expect that Chocolatey (in future versions) won't do something that results in something that they have done manually stops working.

We have discussed this within the team, and the plan going forward would be to check whether the cpack shim exists, if it does, if it is signed with either the Chocolatey Software or Real Dimensions Authenticode signature, we will delete it, as we know that we placed it there, otherwise, we will leave it alone.

@gep13 gep13 marked this pull request as draft February 4, 2022 09:44
@gep13
Copy link
Member Author

gep13 commented Feb 4, 2022

Converting to to draft until an addition to the chocolateyInstall.ps1 file is made to affect the removal of the cpack shim from the bin folder on upgrade to 1.0.0.

@gep13 gep13 requested a review from AdmiringWorm March 3, 2022 11:16
@AdmiringWorm AdmiringWorm marked this pull request as ready for review March 11, 2022 12:45
Copy link
Member

@AdmiringWorm AdmiringWorm 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 I added the changes in the PowerShell file I'll need a second pair of eyes to ensure it both works as expected and looks fine.

@AdmiringWorm AdmiringWorm force-pushed the issue-89 branch 2 times, most recently from da5f4a4 to a91cf3f Compare March 11, 2022 18:34
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 the one comment from my perspective.

nuget/chocolatey/tools/chocolateysetup.psm1 Outdated Show resolved Hide resolved
Copy link
Member Author

@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.

Small change...

nuget/chocolatey/tools/chocolateysetup.psm1 Show resolved Hide resolved
There are known incompatibilities with this shim when other applications
are installed on the machine, for example cmake. Since we are moving
towards usage of a ubiquitous choco command, removing this shim makes
a lot of sense.
@AdmiringWorm AdmiringWorm force-pushed the issue-89 branch 2 times, most recently from 2a9526f to 1816879 Compare March 14, 2022 11:24
This commit updates the installation/upgrading of
the package to remove the cpack shim if they are
signed with the an authenticode signature with
the subject set to one of our previously used
authenticode signatures.

The code added makes it easy to extend it when needed
for removal of other shims as well.
@gep13 gep13 merged commit 9a61954 into chocolatey:develop Mar 14, 2022
@gep13
Copy link
Member Author

gep13 commented Mar 14, 2022

@AdmiringWorm thanks for getting this over the line!

@gep13 gep13 deleted the issue-89 branch March 14, 2022 13:28
@AdmiringWorm
Copy link
Member

@gep13 it was a pretty big line, but finally got there eventually 😆

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.

Remove the cpack command alias
4 participants