Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add new System.Net.Http.Json project/namespace #42889

Merged
merged 5 commits into from
Mar 26, 2020

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Mar 25, 2020

Porting #42879 to new blazor branch. The 3.1 PR was already approved for servicing. It includes all commits from PR dotnet/runtime#33459 up to dotnet/runtime@4970a53

cc: @ericstj @jozkee @jeffhandley @stephentoub @terrajobst @mkArtakMSFT @safern @GrabYourPitchforks

@safern
Copy link
Member

safern commented Mar 25, 2020

Could you also add this package to the package list in packages.builds so that it gets built?

<ItemGroup Condition="'$(BuildAllPackages)' == 'false' AND '$(SkipManagedPackageBuild)' != 'true'" >
<Project Include="$(MSBuildThisFileDirectory)..\pkg\Microsoft.Private.PackageBaseline\Microsoft.Private.PackageBaseline.builds">
<AdditionalProperties>$(AdditionalProperties)</AdditionalProperties>
</Project>
<Project Include="$(MSBuildThisFileDirectory)..\pkg\Microsoft.Private.CoreFx.NETCoreApp\Microsoft.Private.CoreFx.NETCoreApp.builds">
<AdditionalProperties>$(AdditionalProperties)</AdditionalProperties>
</Project>
<!-- add specific builds / pkgproj's here to include in servicing builds -->
</ItemGroup>

@jozkee
Copy link
Member

jozkee commented Mar 25, 2020

CI failure reason is that tests using LoopbackServer were not compatible with .NET Framework; compatibility was recently added by dotnet/runtime#33158.

'SslProtocols' does not contain a definition for 'Tls13' and no accessible extension method 'Tls13' accepting a first argument of type 'SslProtocols' could be found (are you missing a using directive or an assembly reference?)

@joperezr
Copy link
Member Author

Yup, I just moved them to the netcoreapp-specific configuration

<Project DefaultTargets="Build">
<Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.props))" />
<ItemGroup>
<ProjectReference Include="..\ref\System.Net.Http.Json.csproj">
Copy link
Member

Choose a reason for hiding this comment

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

We should omit the ref unless we have a good reason to have it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Our model until release/3.1 has been to always have the ref unless it causes issues (like hiding dependencies to RAR when targetting desktop) which this package isn't doing, so in my point of view it would be better to not special-case it and keep it consistent unless we think it would hurt. That said, I'm totally open for suggestions here so I can remove it too if you think we should.

Copy link
Member

Choose a reason for hiding this comment

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

We previously did a pass to try to limit the number of places where we expose refs to desktop. System.Text.Json, for example, does not expose a reference assembly. All new packages we try not to include refs unless we must have them for some reason.

<Project>
<PropertyGroup>
<BuildConfigurations>
netstandard;
Copy link
Member

Choose a reason for hiding this comment

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

should we also have a net461 configuration?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could add a net461 config, but that would still mean we will need the facades here, since we depend on SYstem.Text.Json which will pull them in. I was planning on adding net461 config here with my configuration changes wave comming next so that we have one PR doing it for every package that needs it. If you still think I should add it here now, I can do that too.

Copy link
Member

Choose a reason for hiding this comment

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

Go ahead and do it now. I think the timing of the next wave will land before we stabalize this, so I'd prefer to have this done WRT coding and just need to pick up package dependency updates when we ship those.

Copy link
Member

Choose a reason for hiding this comment

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

(assuming this is a small task, if anything gets complicated we can postpone this until after the first preview)

@joperezr
Copy link
Member Author

Seems like OSX Helix machines are hanging which is keeping the OSX leg running. The idea is to merge this in soon in order to start working on making sure the official build of this branch is working as expected. If there isn't more feedback I would love to merge this now to work on the official build and then put up a new PR here addressing any new feedback that comes up.

@joperezr joperezr force-pushed the 3.1-blazor/HttpJson branch from d04e772 to 799aa67 Compare March 25, 2020 23:09
@@ -1,10 +1,13 @@
<Project DefaultTargets="Build">
<Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.props))" />
<ItemGroup>
<ProjectReference Include="..\ref\System.Net.Http.Json.csproj">
<SupportedFramework>net461;netcoreapp2.0;uap10.0.16299;$(AllXamarinFrameworks)</SupportedFramework>
<ProjectReference Include="..\ref\System.Net.Http.Json.csproj" />
Copy link
Member

Choose a reason for hiding this comment

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

You should just omit the projectreference to ref.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM modulo the comment on simplifying the pkgproj

@joperezr
Copy link
Member Author

joperezr commented Mar 26, 2020

Osx test failure is unrelated to this pr so I’ll go ahead and merge this.

@joperezr joperezr merged commit 57f547c into dotnet:release/3.1-blazor Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants