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

Ensure we only reference System.Net.Http v4.0.0.0 #2191

Merged
merged 4 commits into from Jan 23, 2019

Conversation

Projects
None yet
5 participants
@jcansdale
Copy link
Collaborator

jcansdale commented Jan 22, 2019

NOTE: Can't use extension when only .NET 4.7 or below is installed

What happended

When we changed to include the Octokit.GraphQL repository as a submodule in #2127 and reference its projects (rather than referencing it as a NugGet package), this had the unintended consequence of changing which version of System.Net.Http any consuming projects referenced.

For users who have a new version of .NET installed this didn't break anything because there is an automatic binding direct from System.Net.Http, v4.1.1.0 to System.Net.Http, v4.0.0.0. Unfortunately users with an older version of .NET installed would experience the following exception:

System.MissingMethodException: Method not found:
'System.Net.Http.HttpMessageHandler Octokit.Internal.HttpMessageHandlerFactory.CreateDefault()'.
at GitHub.Infrastructure.ExportedHttpClient..ctor()

What this PR does

This PR ensures that we reference the System.Net.Http, v4.0.0.0 assembly by changing the Octokit.GraphQL projects to compile against .NET 4.6 rather than .NET Standard 1.1 as they were before. This is a simple fix that should allow us to get a patch release out ASAP.

  • Add test to check that GitHub.* assemblies only reference System.Net.Http, v4.0.0.0
  • Compile against .NET 4.6 version of Octokit.GraphQL and Octokit.GraphQL.Core

How to test

  • Check that .vsix doesn't contain a System.Net.Http assembly.

Here is a screenshot of the .vsix contents from this PR:
image

More detail

On recent versions of .NET there is an automatic redirect from System.Net.Http, v4.1.1.0 to v4.1.1.0. Here is a binding log generated by fuslogvw:

image

Attempting to load System.Net.Http, v4.1.1.0 inside a docker image build from microsoft/dotnet-framework:4.6.2-runtime fails.

Step 20/23 : FROM microsoft/dotnet-framework:4.6.2-runtime AS runtime
 ---> 91af6975d04b
Step 21/23 : WORKDIR /app
 ---> Using cache
 ---> c3fc388c3cf6
Step 22/23 : COPY --from=publish /app/dotnetapp/out ./
 ---> 405821dc4a0d
Step 23/23 : ENTRYPOINT ["dotnetapp.exe"]
 ---> Running in 00c145e35a59
Removing intermediate container 00c145e35a59
 ---> 9f064e8ae099
Successfully built 9f064e8ae099
Successfully tagged foo3:latest
C:\Source\github.com\Microsoft\dotnet-framework-docker\samples\dotnetapp [master ≡ +0 ~4 -0 !]> docker run foo3
Attempting to load: System.Net.Http, Version=4.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a

Unhandled Exception: System.IO.FileNotFoundException: Could not load file or assembly 'System.Net.Http, Version=4.1.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.

I don't know when exactly this redirect was put in place, but it was somewhere between .NET 4.6.2 and .NET 4.7.2.

Fixes #2190
Fixes #2188
Fixes #2186
Fixes #2185
Fixes #2195

@jcansdale jcansdale requested review from grokys , shana and meaghanlewis Jan 22, 2019

@jcansdale jcansdale changed the title [wip] Ensure we only reference System.Net.Http v4.0.0.0 Ensure we only reference System.Net.Http v4.0.0.0 Jan 22, 2019

@shana

shana approved these changes Jan 22, 2019

Copy link
Contributor

shana left a comment

Nice touch with the tests there, good job! 👍

@grokys

grokys approved these changes Jan 22, 2019

Copy link
Collaborator

grokys left a comment

LGTM, if this works then we can try dual-targeting Octokit.GraphQL.

@jcansdale jcansdale force-pushed the fixes/2190-system-net-http-4-0 branch 2 times, most recently from 0fdfb12 to 9eb18d7 Jan 22, 2019

@jcansdale jcansdale referenced this pull request Jan 22, 2019

Closed

Patch release 2.7.1 #2192

@jcansdale jcansdale merged commit e98aa41 into master Jan 23, 2019

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jcansdale jcansdale deleted the fixes/2190-system-net-http-4-0 branch Jan 23, 2019

@jcansdale jcansdale referenced this pull request Jan 23, 2019

Open

Release Notes #1

@tsimmons1987

This comment has been minimized.

Copy link

tsimmons1987 commented Jan 23, 2019

@jcansdale @meaghanlewis This PR has fixed it my issues! Thanks!

One small nit I noticed, when I clicked on the "Release Notes" link in the VSIX installer, the notes indicated this update would be version 2.7.1, however after installed, Visual Studio shows the version as 2.8.0.xxxx

@tsimmons1987

This comment has been minimized.

Copy link

tsimmons1987 commented Jan 23, 2019

@jcansdale Not sure it's important, but to narrow the .Net version issue down abit... My issues occurred with .Net Version 4.7.02556 installed.

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Jan 24, 2019

One small nit I noticed, when I clicked on the "Release Notes" link in the VSIX installer, the notes indicated this update would be version 2.7.1, however after installed, Visual Studio shows the version as 2.8.0.xxxx

I did the initial fix on the master branch (which is now 2.8), but rebased back to 2.7 for the patch release. The 2.8.0.xxxx version you have installed should be identical to 2.7.1.

Not sure it's important, but to narrow the .Net version issue down abit... My issues occurred with .Net Version 4.7.02556 installed.

Thanks, this is useful (and worrying) information. The issue will have been more widespread that I expected. Am I correct in thinking you're not using a version of Windows 10 with auto-updates turned on?

@tsimmons1987

This comment has been minimized.

Copy link

tsimmons1987 commented Jan 25, 2019

I’ll have to look when I get to work tomorrow, but I think my laptop does NOT do auto updates. It is Windows 10 and it’s prettylocked down by my IT Dept.

@anonamystawakened666

This comment has been minimized.

Copy link

anonamystawakened666 commented Jan 25, 2019

@tsimmons1987

This comment has been minimized.

Copy link

tsimmons1987 commented Jan 25, 2019

@jcansdale Just to close the loop, I checked and my laptop is set to automatically update, not sure why it didn't update .Net to a version that doesn't exhibit the problem. It could be that my company limits the updates available. Also the auto update of Visual Studio is what caused the issue when I suddenly couldn't open my repos any more.

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