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

(#2738) Check installed .NET version installed is at least 4.8 #2935

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

TheCakeIsNaOH
Copy link
Member

@TheCakeIsNaOH TheCakeIsNaOH commented Dec 14, 2022

Description Of Changes

This adds a registry check to chocolatey.console to validate that .Net 4.8 or later is installed. This check is added because Chocolatey CLI can start to execute but runs into errors when the system has an older version of .Net framework installed. The check is very early on in the execution so as to print out a helpful error before any errors due to incompatibilities with the older .Net version show up.

Motivation and Context

It is better to have a check and helpful message than various unhelpful error messages.

Testing

  1. Build PR
  2. On Server 2012 R2 (with the default .Net 4.5.2 installed), upgrade from Chocolatey CLI v1.2.1 via the nupkg built in step 1
  3. Validate that choco commands cause an error (which happens too early in execution for a check to be added)
  4. Update .Net to 4.6.0 and reboot
  5. Validate that choco commands print out the error messaged added in this PR
  6. Update .Net to 4.8.0 and reboot
  7. Validate that choco commands do not trigger the error messaged added in this PR

Additionally, test on Windows 10 with .Net 4.8 installed that choco commands do not trigger the error messaged added in this PR

Operating Systems Testing

  • Windows Server 2012R2
  • Windows 10 22H2

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Part of #2738
https://app.clickup.com/t/20540031/PROJ-368

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.

This looks great to me! I have left some comments/questions.

src/chocolatey.console/Program.cs Outdated Show resolved Hide resolved
src/chocolatey.console/Program.cs Outdated Show resolved Hide resolved
src/chocolatey.console/Program.cs Show resolved Hide resolved
src/chocolatey.console/Program.cs Show resolved Hide resolved
src/chocolatey.console/Program.cs Show resolved Hide resolved
@gep13
Copy link
Member

gep13 commented Dec 15, 2022

@TheCakeIsNaOH am I right in saying that even with this change in place, there is still an error when running on .NET Framework 4.5.1? i.e. we don't see this actual error that we want to throw?

@gep13
Copy link
Member

gep13 commented Dec 15, 2022

@TheCakeIsNaOH @AdmiringWorm are there any tests that we can do around this change? Or do we not think that they are required?

@TheCakeIsNaOH
Copy link
Member Author

am I right in saying that even with this change in place, there is still an error when running on .NET Framework 4.5.1? i.e. we don't see this actual error that we want to throw?

Correct, the error is about the System.Array.Empty() method not being found.

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the 2738-add-console-check branch 2 times, most recently from 63351b5 to 62d05c7 Compare December 15, 2022 16:04
@TheCakeIsNaOH TheCakeIsNaOH requested a review from gep13 December 15, 2022 16:05
@AdmiringWorm
Copy link
Member

@TheCakeIsNaOH @AdmiringWorm are there any tests that we can do around this change? Or do we not think that they are required?

Could perhaps be possible to do this as part of the E2E tests, but that would require manipulating the registry as part of the initialization (and restoring it once the test has run).

I think @corbob may be in a better position to answer that though since I think he has experience with manipulating the necessary part of the registry.

@corbob
Copy link
Member

corbob commented Dec 15, 2022

The NDP keys in the registry are owned by everyone's favorite account: TrustedInstaller. That means in order for us to change them, we need to change the owner away from TrustedInstaller, and then grant modify permissions. The last time I looked at automating something like that I think ended up finding regini as an option, but the documentation about the correct incantation is a little sparse, and it requires the Windows Server 2003 resource kit to actually get regini. We could possibly set the permissions through PowerShell, but I don't know of any resources showing how you might do that.

All of that to say: If we can get permissions to the registry keys needed, there's no reason we shouldn't have an E2E test to cover it, but I'm not aware of automated ways to set the permissions.

In case anyone decides to look at regini: https://learn.microsoft.com/en-US/troubleshoot/windows-client/application-management/change-registry-values-permissions

@TheCakeIsNaOH
Copy link
Member Author

We could possibly set the permissions through PowerShell, but I don't know of any resources showing how you might do that.

Yes, it is possible. I have some internal packages that take ownership from trusted installer to set registry keys.

The trick is adjust token privileges to add the SeTakeOwnershipPrivilege, and then try to take ownership.
The blog post to let powershell add token privileges is here:
https://www.leeholmes.com/adjusting-token-privileges-in-powershell/

And there is an example here of how to take ownership:
https://social.technet.microsoft.com/Forums/en-US/e718a560-2908-4b91-ad42-d392e7f8f1ad/take-ownership-of-a-registry-key-and-change-permissions?forum=winserverpowershell

This code has been bouncing around for a while, so I would expect there to be lots of other examples.

@choco-bot
Copy link

This adds a registry check to chocolatey.console to validate that .Net
4.8 or later is installed. This check is added because Chocolatey CLI
can start to execute but runs into errors when the system has an older
version of .Net framework installed. The check is very early on in the
execution so as to print out a helpful error before any errors due to
incompatibilities with the older .Net version show up.
@gep13 gep13 force-pushed the 2738-add-console-check branch from abeb893 to bfd4176 Compare December 20, 2022 07:46
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!

@gep13 gep13 merged commit eabb51a into chocolatey:develop Dec 20, 2022
@gep13
Copy link
Member

gep13 commented Dec 20, 2022

@TheCakeIsNaOH thanks for getting this added!

@TheCakeIsNaOH TheCakeIsNaOH deleted the 2738-add-console-check branch December 20, 2022 14:30
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.

5 participants