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

Fix StartupTests.StartsWithDotnetInstallLocation #6589

Merged
merged 5 commits into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@jkotalik
Copy link
Contributor

jkotalik commented Jan 11, 2019

The reg key flag RRF_SUBKEY_WOW6432KEY doesn't evaluate to WOW6432Node on Win8 and below. Therefore we need to insert it ourselves.

StartsWithDotnetInstallLocation is failing on CI currently.

@jkotalik jkotalik requested a review from pakrym Jan 11, 2019

@pakrym

This comment has been minimized.

Copy link
Contributor

pakrym commented Jan 11, 2019

WOW6432Node doesn't exist on 32bit windows and for 32bit apps AFAIK. You might need to make it conditional on running inside 64bit process

Remove flag parameter from TryGetString

@halter73

This comment has been minimized.

Copy link
Member

halter73 commented Jan 11, 2019

Another option could be to open the HKEY_LOCAL_MACHINE section using RegOpenKeyEx with the KEY_WOW64_32KEY flag which seems defined all the way back to Windows XP on both the 32-bit and 64-bit versions. Since it's a predefined registry key, there's no need to close it.

Then you could use the newly opened section/key in place of HKEY_LOCAL_MACHINE when you call TryGetString without any need to include WOW6432Node in the subsection name since the KEY_WOW64_32KEY flag should do that implicitly for you that on 64-bit systems.

@halter73
Copy link
Member

halter73 left a comment

Needs to work on 32 bit windows

@jkotalik

This comment has been minimized.

Copy link
Contributor

jkotalik commented Jan 11, 2019

🆙 📅

@pakrym

This comment has been minimized.

Move this logic into the caller

@pakrym

This comment has been minimized.

Why do you need this?

This comment has been minimized.

Copy link
Contributor

jkotalik replied Jan 11, 2019

You need to pass in that key on 32 bit, right?

This comment has been minimized.

Copy link
Contributor

pakrym replied Jan 11, 2019

Nope

@pakrym

This comment has been minimized.

Copy link
Contributor

pakrym commented on src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp in 01515e7 Jan 11, 2019

You can remove std::wstring, I think

jkotalik added some commits Jan 11, 2019

Fb
@pakrym

pakrym approved these changes Jan 11, 2019

@jkotalik

This comment has been minimized.

Copy link
Contributor

jkotalik commented Jan 12, 2019

I'm still going to play around with calling RegOpenKeyEx to get an HKEY that already contains WOW6432Node. Would be a bit cleaner than this.

@pakrym

This comment has been minimized.

Copy link
Contributor

pakrym commented Jan 14, 2019

I think this looks good enough.

@jkotalik

This comment has been minimized.

Copy link
Contributor

jkotalik commented Jan 14, 2019

Discussed offline with @pakrym, we are going to stick with this rather than calling RegOpenKeyEx as they are functionally the same.

Resolved.

@jkotalik jkotalik merged commit 3f4622f into master Jan 14, 2019

1 check passed

license/cla All CLA requirements met.
Details

@jkotalik jkotalik deleted the jkotalik/regKey branch Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment