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

Remove explicit reference to System.Net.Http #531

Closed
wants to merge 1 commit into from
Closed

Remove explicit reference to System.Net.Http #531

wants to merge 1 commit into from

Conversation

verdie-g
Copy link
Contributor

The assembly is only used in CloudMetadataService for .NET Standard
builds but System.Net.Http is built-in in .NET Standard and doesn't
need an explicit reference.

Solution mixing nuget and built-in references to System.Net.Http leads
to runtime issues that we are trying to solve by removing the nuget
references.

dotnet/runtime#26131 (comment)

In most cases, we don't advise people use the separate System.Net.Http
NuGet package anymore

The assembly is only used in CloudMetadataService for .NET Standard
builds but System.Net.Http is built-in in .NET Standard and doesn't
need an explicit reference.

Solution mixing nuget and built-in references to System.Net.Http leads
to runtime issues that we are trying to solve by removing the nuget
references.

dotnet/runtime#26131 (comment)
> In most cases, we don't advise people use the separate System.Net.Http
> NuGet package anymore
@joao-r-reis
Copy link
Collaborator

joao-r-reis commented Jul 24, 2020

Hmm I think that we were required to add it for netstandard1.5 if I'm not mistaken but since we dropped support for that version we can probably remove it.

I want to run some more tests to make sure nothing breaks and I'll try to write an implementation of SimulacronManager that doesn't depend on System.Net.Http when built with .NET Framework so that we can remove that reference from the test project as well, otherwise we aren't really testing the scenario that we want.

I should have an update about this soon. Is this urgent or can you wait for the next release? (The next release should be out around end of August / early September).

@verdie-g
Copy link
Contributor Author

verdie-g commented Jul 24, 2020

Thanks for the quick response. We will be using my fork until the next release as this issue is in our critical path.

The jenkins pipeline was not successful. Is it something I should worry about?

@joao-r-reis
Copy link
Collaborator

No, the first run was green but this one ran the nightly suite and it had a couple of test failures which I will investigate later, they are not related to this change.

We can get out a patch release with just this change if this is blocking you, I'll run the tests that I need to run after the weekend and after that I will merge this PR and push a new patch release.

@verdie-g
Copy link
Contributor Author

I would be, indeed, very interested in a quick release with this change, thanks :)

@joao-r-reis
Copy link
Collaborator

I opened another PR #532 with all the necessary changes to remove the reference and I'm currently running tests using that branch. If everything goes smoothly I should be able to push a 3.15.1 release still today.

Sorry for not taking your commit here, I removed the reference as part of f48568d

@joao-r-reis joao-r-reis modified the milestone: 3.15.1 Jul 27, 2020
@joao-r-reis
Copy link
Collaborator

@verdie-g
Copy link
Contributor Author

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants