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

Suppress warnings about breaking changes to Newtonsoft.Json dependency in SignalR #9405

Merged
merged 3 commits into from Apr 16, 2019

Conversation

Projects
None yet
7 participants
@natemcmaster
Copy link
Member

natemcmaster commented Apr 16, 2019

Resolves aspnet/AspNetCore-Internal#1940

This adds support for listing a reference removal as an allowable breaking change.

@natemcmaster natemcmaster requested a review from BrennanConroy Apr 16, 2019

@natemcmaster natemcmaster requested review from anurse, halter73, mikaelm12 and aspnet/build as code owners Apr 16, 2019

@@ -27,4 +27,9 @@
<Reference Include="System.Runtime.CompilerServices.Unsafe" />
</ItemGroup>

<ItemGroup Condition="'$(AspNetCoreMajorMinorVersion)' == '3.0'">

This comment has been minimized.

Copy link
@BrennanConroy

BrennanConroy Apr 16, 2019

Member

This is technically a new package in 3.0.

We replaced the old Microsoft.AspNetCore.SignalR.Protocols.Json package with the Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson package and then removed the old one.

So this is now a new package and probably shouldn't have a baseline associated with it.

This comment has been minimized.

Copy link
@natemcmaster

natemcmaster Apr 16, 2019

Author Member

Even so, it's the same package ID and the build system (and users) see this as a breaking change. It's okay since this was planned. This documents that intention in the code.

@@ -0,0 +1,17 @@
Build Errors

This comment has been minimized.

Copy link
@anurse

anurse Apr 16, 2019

Member

👏 👏 👏 👏 👏 👏

@@ -28,4 +28,9 @@
<Reference Include="System.Threading.Channels" />
</ItemGroup>

<ItemGroup Condition="'$(AspNetCoreMajorMinorVersion)' == '3.0'">
<!-- This dependency was replaced by Protocols.NewtonsoftJson between 3.0 and 2.2. This suppression can be removed after 3.0 is complete. -->

This comment has been minimized.

Copy link
@halter73

halter73 Apr 16, 2019

Member

Should we file an issue to do this after branching for 3.0? I guess it's not a big deal if we don't remove it right away.

This comment has been minimized.

Copy link
@dougbu

dougbu Apr 16, 2019

Member

Well, such an issue is likely to get lost between now and RTM if it has to wait in the backlog. But, if you create a 4.0 milestone for it, that'll work.

This comment has been minimized.

Copy link
@BrennanConroy

BrennanConroy Apr 16, 2019

Member

We don't need to file an issue, this will go away when we make System.Text.Json the default.

This comment has been minimized.

Copy link
@natemcmaster

natemcmaster Apr 16, 2019

Author Member

It doesn't hurt if it stays around, but feel free to file an issue to clean up later. I'm writing these suppressions with the condition that checks version to makes sure the code doesn't become a problem later when we branch for 3.1 and beyond.

@dougbu

dougbu approved these changes Apr 16, 2019


> error BUILD002: Package references changed since the last release...
Similar to BUILD001, but this error not suppressable. This error only appears in servicing builds which should not change references between assemblies or packages.

This comment has been minimized.

Copy link
@dougbu

dougbu Apr 16, 2019

Member

Minor but this will slow readers: "error is not…"


## Example: make a breaking change to references

If Microsoft.AspNetCore.Banana in 2.1 had a reference to `Microsoft.AspNetCore.Orange`, but in 3.0 this reference is changing to `Microsoft.AspNetCore.BetterThanOrange`, you would need to make these changes to the .csproj file

This comment has been minimized.

Copy link
@dougbu

dougbu Apr 16, 2019

Member

🍌 is already better than orange (🔶)

@@ -28,4 +28,9 @@
<Reference Include="System.Threading.Channels" />
</ItemGroup>

<ItemGroup Condition="'$(AspNetCoreMajorMinorVersion)' == '3.0'">
<!-- This dependency was replaced by Protocols.NewtonsoftJson between 3.0 and 2.2. This suppression can be removed after 3.0 is complete. -->

This comment has been minimized.

Copy link
@dougbu

dougbu Apr 16, 2019

Member

Well, such an issue is likely to get lost between now and RTM if it has to wait in the backlog. But, if you create a 4.0 milestone for it, that'll work.

@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented Apr 16, 2019

Hard to tell but the build failures in attempt 1 are likely flakiness. I started attempt 2.

@aspnet-hello

This comment has been minimized.

Copy link

aspnet-hello commented Apr 16, 2019

This comment was made automatically. If there is a problem contact ryanbrandenburg.

I've triaged the above build. I've created/commented on the following issue(s)
aspnet/AspNetCore-Internal#2267

@Eilon Eilon added the area-signalr label Apr 16, 2019

@natemcmaster natemcmaster merged commit 607cbc3 into aspnet:master Apr 16, 2019

1 check passed

license/cla All CLA requirements met.
Details

@natemcmaster natemcmaster deleted the natemcmaster:warnings-about-changes branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.