(#1670) Add noshims options for install and upgrade #2003

johnstevenson · 2020-02-17T18:02:03Z

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

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

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.

ferventcoder · 2020-05-16T12:12:24Z

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

ferventcoder

Hi @johnstevenson! This is likely the most complete first time contribution I've ever seen coming to this codebase! This is awesome!

Can I ask that you take a look as a couple of minor things here?

ferventcoder · 2020-05-16T12:34:23Z

Scenarios.md

+#### when uninstalling a package that forgets to call uninstall bin file
+
+ * should have had a shim
+ * should have removed the shim


Awesome, thanks for doing this!

Of course, this only applies to packages that install into ..\chocolatey\lib\<package-name>, so shims targetting binaries somewhere else would not get removed.

If Shimgen accepted the package name as a parameter and stored this in the shim's FileVersionInfo data (together with the target binary, as suggested elsewhere), then this limitation disappears.

ferventcoder · 2020-05-16T12:35:23Z

Scenarios.md

@@ -933,6 +983,16 @@
 * should upgrade the package
 * should upgrade where install location reports

+#### when upgrading a package with shims


What about when upgrading a package that has existing shims with no shims in the upgrade? What's the expected behavior for that use case?

The default behaviour in install and upgrade scenarios is to remove all shims for the package first, then install any shims if not prohibited by --noshims or --noshimsglobal.

So the existing shims will be removed and the new shims will not be installed.

ferventcoder · 2020-05-16T12:36:59Z

src/chocolatey.resources/helpers/functions/Install-BinFile.ps1

+    Write-Debug "Removing any existing shim for `'$name`'."
+    Uninstall-BinFile $name $path
+    return
+  }


Following my last comment, I see you have code to remove shims if no shims is called. Might be good to add a scenario for that use case

Not quite sure what you want here, so I have added an extra upgrade scenario in b496390

This tests an upgrade to a package that has a shim, with the --noshims flag.

Is this the sort of thing you wanted?

src/chocolatey/infrastructure.app/domain/ShimRegistry.cs

ferventcoder · 2020-05-16T12:45:12Z

src/chocolatey/infrastructure.app/domain/ShimRegistry.cs

+            }
+
+            return target;
+        }


Ha! That's a very interesting way to get at the data! How does it handle with a large number of shims (300+)?

Guessing a lot better than running and text parsing for sure!

It's fine with a large number of shims (like mingw and other unixy packages), but as I mentioned in Concerns 1 it is hacky and brittle. Far better would be for shimgen to write the target file to a field in its FileVersionInfo data in a simple formatted way. The SpecialBuild field looks like it would be appropriate location.

We already use this data to check for a shimgen exe anyway:

choco/src/chocolatey/infrastructure.app/domain/ShimRegistry.cs

Lines 189 to 192 in 26ac2df

FileVersionInfo info = FileVersionInfo.GetVersionInfo(exeFile);

// note that this will not catch chocolatey specific shims

if (!info.ProductName.contains("shimgen")) return item;

So it would be even faster to grab the target file from info.SpecialBuild, for example.

ferventcoder · 2020-05-16T12:55:00Z

src/chocolatey/infrastructure.app/services/ShimGenerationService.cs

+                {
+                    this.Log().Error(() => " ShimGen cannot overwrite shim {0} in {1} package".format_with(_fileSystem.get_file_name(file), record.PackageName));
+                    this.Log().Debug(() => "  Shim: {0}{1}  Target: {2}{1}  New target: {3}{1}".format_with(shimLocation, Environment.NewLine, record.TargetFile, file));
+                    Environment.ExitCode = 1;


I'm not sure shim generation would be one of those things I would count as causing an exit code of an error. I would see if enhanced exit codes are turned on and let's specify an exit code specific for this. As in a type of inconclusive installation.

Okay. I'll use enhanced exit code 3 (since 2 has been used for no results)

On second thoughts (or rather getting my head around it again):

I'm not sure shim generation would be one of those things I would count as causing an exit code of an error.

I am actually copying existing behaviour, because an error exit code is specifically returned if shimgen.exe itself fails.

choco/src/chocolatey/infrastructure.app/services/ShimGenerationService.cs

Lines 173 to 176 in 26ac2df

if (exitCode != 0)

{

Environment.ExitCode = exitCode;

}

CLAassistant · 2020-06-08T14:48:41Z

All committers have signed the CLA.

ferventcoder · 2020-06-17T18:31:39Z

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 · 2020-06-17T18:32:11Z

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

johnstevenson · 2020-06-17T19:26:59Z

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 · 2020-06-17T22:00:56Z

@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 · 2020-06-17T22:28:09Z

@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 · 2020-06-18T16:02:32Z

@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 · 2020-06-19T17:21:22Z

Updated CLA signed.

github-actions · 2023-06-25T04:02:54Z

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 · 2023-06-25T10:31:51Z

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.

pauby · 2023-06-25T11:26:05Z

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

Let me speak to the team tomorrow and update.

gep13 · 2023-06-26T15:20:06Z

@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 · 2023-06-30T10:43:41Z

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.

ferventcoder requested changes May 16, 2020

View reviewed changes

ferventcoder added the 0 - Waiting on User label May 16, 2020

johnstevenson added 3 commits June 20, 2020 14:21

Remove unused variable

450f21f

Test upgrade with noshims on existing shim

b496390

Make upgrade scenarios more robust

f8a2bcf

gep13 changed the title ~~(GH-1670) Add noshims options for install and upgrade~~ (#1670) Add noshims options for install and upgrade Oct 14, 2021

gep13 linked an issue Oct 15, 2021 that may be closed by this pull request

install/upgrade: Add --no-shim option to allow skipping creation of shims from user side #1670

Open

github-actions bot added the Pending Closure This issue has been marked as having no response or is stale and will soon be closed. label Jun 11, 2023

TheCakeIsNaOH mentioned this pull request Jun 19, 2023

Chocolatey 2.0 creates shims for virtual/meta-pacakge #3204

Open

4 tasks

github-actions bot added the No Response / Stale label Jun 25, 2023

github-actions bot closed this Jun 25, 2023

gep13 reopened this Jun 26, 2023

github-actions bot removed No Response / Stale Pending Closure This issue has been marked as having no response or is stale and will soon be closed. labels Jun 27, 2023

github-actions bot added the Pending Closure This issue has been marked as having no response or is stale and will soon be closed. label Jul 31, 2023

TheCakeIsNaOH removed 0 - Waiting on User Pending Closure This issue has been marked as having no response or is stale and will soon be closed. labels Jul 31, 2023

	FileVersionInfo info = FileVersionInfo.GetVersionInfo(exeFile);

	// note that this will not catch chocolatey specific shims
	if (!info.ProductName.contains("shimgen")) return item;

(#1670) Add noshims options for install and upgrade #2003

Are you sure you want to change the base?

(#1670) Add noshims options for install and upgrade #2003

Uh oh!

Conversation

johnstevenson commented Feb 17, 2020

Uh oh!

ferventcoder commented May 16, 2020

Uh oh!

ferventcoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ferventcoder commented Jun 17, 2020

Uh oh!

ferventcoder commented Jun 17, 2020

Uh oh!

johnstevenson commented Jun 17, 2020

Uh oh!

AdmiringWorm commented Jun 17, 2020

Uh oh!

ferventcoder commented Jun 17, 2020

Uh oh!

AdmiringWorm commented Jun 18, 2020

Uh oh!

johnstevenson commented Jun 19, 2020

Uh oh!

github-actions bot commented Jun 25, 2023

Uh oh!

johnstevenson commented Jun 25, 2023

Uh oh!

pauby commented Jun 25, 2023

Uh oh!

gep13 commented Jun 26, 2023

Uh oh!

johnstevenson commented Jun 30, 2023

Uh oh!

Uh oh!

CLAassistant commented Jun 8, 2020 •

edited

Loading