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

Fix GetClientUserAgentString() #1151

Merged
merged 1 commit into from
Apr 6, 2018
Merged

Fix GetClientUserAgentString() #1151

merged 1 commit into from
Apr 6, 2018

Conversation

ob-stripe
Copy link
Contributor

Tentative fix for #990.

Copy link
Contributor

@jaymedavis jaymedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost sure I tried this, but can’t remember entirely. Maybe it worked but didn’t provide the right information? As long as it runs on Mac too, it should be fine!

@ob-stripe ob-stripe force-pushed the ob-fix-990 branch 3 times, most recently from 7239436 to 531e9a1 Compare April 6, 2018 14:51
@ob-stripe
Copy link
Contributor Author

After a lot of efforts, I was able to run the XUnit test suite using Visual Studio for Mac and .NET Framework 4.6! AFAICT it works fine.

For reference, here are the differences in the X-Stripe-Client-User-Agent string:

  • OS X, target net46 (from Visual Studio for Mac):
"lang_version": ".NET Framework 4.5+",
"os_version": "Unix 17.4.0.0",
"mono_version": "5.8.1.0 (2017-10/6bf3922f3fd Thu Mar  8 16:47:23 EST 2018)"
  • OS X, target netcoreapp1.1:
"lang_version": ".NET Core 4.6.26011.01",
"os_version": "Darwin 17.4.0 Darwin Kernel Version 17.4.0: Sun Dec 17 09:19:54 PST 2017; root:xnu-4570.41.2~1/RELEASE_X86_64"
  • Windows, target net46 (from Appveyor):
"lang_version": ".NET Framework 4.5+",
"os_version": "Microsoft Windows 10.0.14393 "
  • Windows, target netcoreapp1.1 (from Appveyor):
"lang_version": ".NET Core 4.6.25211.01",
"os_version": "Microsoft Windows 10.0.14393 "

I've removed the registry lookup entirely -- while it was possible to simply add a check for non-Windows platform, I'm fairly sure the registry lookup was ~useless because it was not returning the current runtime, but rather the most recent framework version available on the machine. (E.g. on Appveyor, it reported "4.7.1 or later" even when targeting 4.6.)

As far as I can tell, there's no good way of checking the actual .NET Framework version at runtime -- there's System.Environment.Version, but it only lets you distinguish between 4.5 and 4.6, and Microsoft actually recommends using the registry instead. At that point I threw my hands in the air and decided "4.5+" was good enough.

r? @brandur-stripe
cc @stripe/api-libraries

@brandur-stripe
Copy link
Contributor

Wow, you are a hero for tracking this one down @ob-stripe! Thanks.

I'll admit up front that I've never looked at these internals before, but this diff and your rationale look correct. I can't believe how difficult it is to do this!

LGTM.

@ob-stripe ob-stripe merged commit ba4c031 into master Apr 6, 2018
@ob-stripe ob-stripe deleted the ob-fix-990 branch April 6, 2018 18:36
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.

3 participants