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

Fixing SizeF, CookieContainer, CookieCollection serialization between Core <--> Desktop and adding full binary mode tests #21224

Merged
merged 1 commit into from
Jun 24, 2017
Merged

Fixing SizeF, CookieContainer, CookieCollection serialization between Core <--> Desktop and adding full binary mode tests #21224

merged 1 commit into from
Jun 24, 2017

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jun 19, 2017 •
edited
Loading

#21223

Requires dotnet/coreclr#12367

cc @danmosemsft

@ViktorHofer ViktorHofer self-assigned this Jun 19, 2017
@ViktorHofer ViktorHofer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 19, 2017
@ViktorHofer ViktorHofer force-pushed the release/2.0.0-CookieContainerSerializationFix branch from ce20af1 to b2a21e2 Compare June 19, 2017 15:30
@ViktorHofer ViktorHofer changed the title Adding TypeForward to PathListComparer to fix core --> desktop serialization (21223) Fixing CookieContainer, CookieCollection serialization between Core <--> Desktop and adding full binary mode tests Jun 19, 2017
@ViktorHofer ViktorHofer mentioned this pull request Jun 19, 2017
Merged
@ViktorHofer ViktorHofer force-pushed the release/2.0.0-CookieContainerSerializationFix branch from b2a21e2 to 4539cb9 Compare June 19, 2017 15:49
@ViktorHofer ViktorHofer changed the title Fixing CookieContainer, CookieCollection serialization between Core <--> Desktop and adding full binary mode tests Fixing SizeF, CookieContainer, CookieCollection serialization between Core <--> Desktop and adding full binary mode tests Jun 19, 2017
@karelz karelz modified the milestone: 2.0.0 Jun 20, 2017
@ViktorHofer ViktorHofer force-pushed the release/2.0.0-CookieContainerSerializationFix branch 2 times, most recently from 89c6492 to 53d11d0 Compare June 22, 2017 14:27
@ViktorHofer @danmoseley
Fixing SizeF, CookieContainer, CookieCollection serialization between…
3375d53 
… Core <--> Desktop and adding full binary mode tests (dotnet#21223)

* Adding TypeForward to PathListComparer, CookieCollection member, full-mode tests, SizeF publickeytoken fix

* Making FormatterAssemblyStyle explicit

* Update blobs after CompareInfo culture field added to serialization payload

* Added blob integrity check

* Remove unnecessary operation which has no impact on netfx serialization

* Putting pragma around the unused field...

* Setting m_version value to include it in the serialization payload. Remove static versions for blob comparison

* Update compare logic

* update blobs

* disable checking if blob has changed
@danmoseley danmoseley force-pushed the release/2.0.0-CookieContainerSerializationFix branch from 53d11d0 to 3375d53 Compare June 23, 2017 03:33
@danmoseley
Copy link
Member

Updated to match the master change.

@danmoseley danmoseley requested a review from krwq June 23, 2017 03:37
krwq
krwq reviewed Jun 23, 2017
View reviewed changes
src/System.Net.Primitives/src/System/Net/CookieContainer.cs
@@ -1048,6 +1048,7 @@ public IEnumerator GetEnumerator()
public object SyncRoot => m_list.SyncRoot;

[Serializable]
[System.Runtime.CompilerServices.TypeForwardedFrom("System, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
Copy link
Member

@krwq krwq Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Should this be b03f5f7f11d50a3a or the other one match this?

Copy link

Choose a reason for hiding this comment

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

b77 is correct

krwq
krwq approved these changes Jun 23, 2017
View reviewed changes
Copy link
Member

@krwq krwq left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM assuming pointed in the comment PublicKeyToken is correct

@ViktorHofer
Copy link
Member Author

Approved by shiproom. After new coreclr version is in release/2.0.0 branch I will take a look @krwq. Thanks!

@krwq
Copy link
Member

krwq commented Jun 23, 2017

@ViktorHofer could you take a look at the PublicKeyToken comment in src/System.Net.Primitives/src/System/Net/CookieContainer.cs, you got more context on this

@krwq
Copy link
Member

krwq commented Jun 23, 2017

@morganbr thanks for confirming!

@danmosemsft this is good to go assuming this is approved

@ViktorHofer
Copy link
Member Author

The thing is, the coreclr change isn't yet in. Without comparing blobs we have green CI but in fact it shouldn't be green... Waiting just to make sure

@krwq
Copy link
Member

krwq commented Jun 24, 2017

@ViktorHofer what scenarios are broken without coreclr changes?

@ViktorHofer
Copy link
Member Author

Serialization with full mode of types which depend on CompareInfo which itself depends on the introduced culture field. Should at least be a couple of types. Because we already updated the blobs and don't have a equals test for this field CI is green although it shouldn't be till the corevlr bits are in

@danmoseley
Copy link
Member

We have a new CLR.
@dotnet-bot test this plesae

@danmoseley
Copy link
Member

@dotnet-bot test this please (spell please right)

@danmoseley
Copy link
Member

@ViktorHofer good to merge?

@ViktorHofer ViktorHofer merged commit decbb45 into dotnet:release/2.0.0 Jun 24, 2017
@wfurt wfurt mentioned this pull request Jan 29, 2019
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@morganbr morganbr morganbr left review comments

@krwq krwq krwq approved these changes

@danmoseley danmoseley Awaiting requested review from danmoseley

Assignees

@ViktorHofer ViktorHofer

Labels
area-Serialization * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ViktorHofer @danmoseley @krwq @morganbr @karelz @dnfclas