(#1670) Add noshims options for install and upgrade

[johnstevenson]

The --noshims and --noshimsglobal options allow the user to stop shims being created for either the package, or the package and its dependencies.

In addition to this new behaviour, the implementation handles the creation and removal of shims more robustly, so that shims from other packages (or the same package) cannot be unintentionally overwritten or removed.

The new ShimRegistry class keeps track of the currently installed shims and their corresponding packages. It does this by examining all the exe files in the shim directory (so old-style .bat and unixy shims are not supported) and extracting the target file from the binary. If this target path contains the lib folder, then the package name can be obtained.

ShimRegistry is updated for each package, just before the Powershell scripts are run, using ShimGenerationService::take_snapshot. When ShimGenerationService installs any shims for the package, ShimRegistry provides its existing shim files (ie. those that were not modified or removed since the last update) so that they can be deleted prior to installing any new ones.

To stop shims being added from Install-BinFile, a new environment variable chocolateyNoShims is used when running the Powershell scripts. If a package sets a shim this way, but then forgets to call Uninstall-BinFile when uninstalling, the shim will be removed anyway if its target is in the package folder.

Concerns

1. The method to extract the target file from a shim is brittle because it relies on both the net compiler and shimgen.exe formatting its strings in the way it does now (and calling with --shimgen-noop is way too slow). Better would be if shimgen.exe wrote the data to the file version info: the SpecialBuild field looks like it would be appropriate.
2. If a package unintentionally overwrites an existing shim (from another package or its own) I have logged an error and set the exit code to 1. Depending on whether this behaviour is classed as a bug or not (which it certainly should be in the first instance) there may well be a BC break.
3. I'm bound to have done something wrong, or missed something I should have done, so please let me know (and sorry).

A bit of a moan I've spent far too much time fighting against trailing whitespace (and occasionally inconsistent line endings) in this repo. That aside, nice work - it's certainly a complex beast.

[ferventcoder]

@johnstevenson thanks for this - apologies we haven't reviewed it until just now. I'm going to take a look at this and start providing feedback.

[CLAassistant]

All committers have signed the CLA.

[ferventcoder]

A bit of a moan I've spent far too much time fighting against trailing whitespace (and occasionally inconsistent line endings) in this repo. That aside, nice work - it's certainly a complex beast.

Yeah, the resharper headers tend to put in spaces where none are necessary. That's something we are using a VS extension now to automatically correct for us.

The inconsistent line endings we try to fix as we find them. Certainly comes about with multiple editors and contributors bringing things in over the years. 😄

[ferventcoder]

NOTE: CLA hub was signed, so new CLA won't be necessary.

[johnstevenson]

Sorry, I haven't forgotten about this, but it got pushed to the back of a long list of stuff after the initial impetus was lost! Should get around to it later this week.

[AdmiringWorm]

@ferventcoder just a tip, you can upload a csv delimited file to cla-assistent with the names of each contributor that have already signed the cla using clahub (if I remember correctly, just put the name github handle of the user seperately on each line, and then run a recheck on the necessary PR's).

I think you can also add the date the user signed the cla as well (but that isn't something I have tested).

[ferventcoder]

@AdmiringWorm the old CLA agreement was with RealDimensions Software, LLC and the new on is with Chocolatey Software, Inc. Legally speaking, that would not be a good thing to do.

[AdmiringWorm]

@ferventcoder fair enough, but considering that this is then a new CLA, then legally speaking, I believe it would be appropriate to require users to sign the new CLA before any PR is pulled in.

Even in cases where there are open pull requests that had signed the old CLA, the CLA had been changed before the changes were pulled into the codebase.

That is just my take on it, though, and I can't speak of what would be required from a legal perspective.

But anyway, I was just giving a tip based on what I had done when it had been necessary to sign the CLA for a bot user, and thought it was still the same CLA.

[johnstevenson]

Updated CLA signed.

[github-actions[bot]]

Dear contributor,

As this PR seems to have been inactive for 30 days after changes / additional information was requested, it has been automatically closed. If you feel the changes are still valid, please re-open the PR once all changes or additional information that was requested has been added. Thank you for your contribution.

[johnstevenson]

Seems a shame that this has been automatically closed.

If you don't want to use it, then no problems at all. However, @ferventcoder did assign this to me: https://github.com/chocolatey/choco/issues/1670#issuecomment-629636489

I get that things have changed and this PR might no longer be relevant, but a more personalized closing comment would have been appreciated.

[pauby]

This was closed automatically. But I'm not entirely sure it should have been.

Let me speak to the team tomorrow and update.

[gep13]

@johnstevenson apologies again about this automatically getting closed, this was caused due to some new infrastructure we have put in place.

If you are interested, would you be willing to fix up this PR to re-target the develop branch, and we can look to include this change in an upcoming release of Chocolatey CLI?

[johnstevenson]

Many thanks for your swift response and re-opening this.

If you are interested, would you be willing to fix up this PR to re-target the develop branch, and we can look to include this change in an upcoming release of Chocolatey CLI?

I'm certainly interested, but it is a matter of having the time at the moment. I personally thought that this was a useful addition to the API, so I would appreciate some indication if this is something you would actually use.

[RogueScholar]

I don't have a dog in this fight except as a Chocolatey user that's become increasingly frustrated with the glacial pace of Chocolatey evolution, with well-recognized misbehaviors and acknowledged bugs seeing years go by without any discernible traction toward a resolution. This behavior in particular has had the status of "this project really eff-ing needs this feature like, yesterday" for me since COVID was in full swing. Apologies in advance for the incredulity that follows, but as an open source contributor and proponent, my blood pressure skyrockets to see a PR draw an initial reaction like…

https://github.com/chocolatey/choco/pull/2003#pullrequestreview-413073361 on May 16, 2020 Hi @johnstevenson! This is likely the most complete first time contribution I've ever seen coming to this codebase! This is awesome! …

…albeit after three months of silence, then go so horribly awry. The author seems to have responded to the subsequent review notes in a timely fashion, only to be met with *crickets* …for over three years is truly a travesty. The coup de grâce of the silence only being broken by—of all things—a cleanup bot, has all the social currency of a huge raised middle finger, especially in the context of the high praise given initially. There could scarcely be a more conclusive signal to potential contributors, especially experienced ones, that to involve themself here will almost certainly amount to a waste of valuable time.

My nature as one whose passions are often swiftly caught ablaze leaves scarce hope that I might one day match the beautifully measured and considerate tone found in this response to the automated closure that came over 40 months after the initial submission, with the last 37 passing silently.

https://github.com/chocolatey/choco/pull/2003#issuecomment-1606016162 on Jun 25, 2023 Seems a shame that this has been automatically closed.

If you don't want to use it, then no problems at all. However, @ferventcoder did assign this to me: #1670 (comment)

I get that things have changed and this PR might no longer be relevant, but a more personalized closing comment would have been appreciated.

I make these observations without any personal animus toward those involved and assume that they are good people who are likely falling short of their own expectations as much as they are of mine at the moment. This note is intended only in the hope of bringing into sharp focus how failures like this one typically become force divisors that haunt a project far into the future. To wit, the happenstance of my own presence in this thread is the product of a long-simmering odium for the CCR package creation documentation that today became catalyzed into a tentative desire to try to improve upon them through contributions of my own. It probably goes without saying that I have now reconsidered those impulses after reading what happened here. I do wish you all the best, in particular @johnstevenson for his impressive work on these desirable changes and sage aplomb to the lack of interest that followed.

---

“The best time to plant a tree is 20 years ago. The second best time is now.” — St. Louis Post Dispatch: April 2, 1902.

[pauby]

I don't have a dog in this fight except as a Chocolatey user that's become increasingly frustrated with the glacial pace of Chocolatey evolution, ...

As this isn't comment isn't helpful on this PR, but important to address, I have moved it here.