Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Merge serialization changes to 2.0.0 - #20530

Merged
danmoseley merged 14 commits into
dotnet:release/2.0.0from
krwq:merge-serialization-changes-to-2.0.0
Jun 1, 2017
Merged

Merge serialization changes to 2.0.0#20530
danmoseley merged 14 commits into
dotnet:release/2.0.0from
krwq:merge-serialization-changes-to-2.0.0

Conversation

@krwq

@krwq krwq commented May 31, 2017

Copy link
Copy Markdown
Member

TODO:

@krwq

krwq commented May 31, 2017

Copy link
Copy Markdown
Member Author

Per my offline conversation with @mmitche Portable runs can be ignored as they should've not run.

@krwq

krwq commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

@stephentoub, could you please take another look? I've had some merge conflicts when cherry picking Viktor's PR.

Conflicting files:

  • src/System.Net.Primitives/src/System/Net/Cookie.cs
  • src/System.Runtime.Serialization.Json/tests/DataContractJsonSerializer.cs
  • src/System.Runtime.Serialization.Xml/tests/DataContractSerializer.cs

I took master changes in all 3 cases since that looked correct. It seems they were all related to UAP (ifdefs, test disable etc.).
First file is a product change so here is example thing which got changed:
https://github.com/dotnet/corefx/blame/release/2.0.0/src/System.Net.Primitives/src/System/Net/Cookie.cs#L661
vs
https://github.com/dotnet/corefx/blame/master/src/System.Net.Primitives/src/System/Net/Cookie.cs#L661

Is that ok or do we need to pick changes more granularily?

CI is expected to fail now without CoreCLR changes

@ViktorHofer

Copy link
Copy Markdown
Member

BinarySerializer change is in. Now we need an offline validation if tests are not working as expected in the release/2.0 branch.

@krwq

krwq commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

@danmosemsft - local run with coreclr pending changes is passing all tests - once coreclr is merged and this PR can pick up the official coreclr bits this PR is expected to pass

@ViktorHofer ViktorHofer added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 1, 2017
@weshaggard

Copy link
Copy Markdown
Member

Per my offline conversation with @mmitche Portable runs can be ignored as they should've not run

Can you please clarify this? There shouldn't be any legs in the CI that should be ignored here. Those look like correct failures that need to be understood. Perhaps those failures are because they need the corresponding coreclr changes.

@krwq

krwq commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

@weshaggard @mmitche said offline that those legs were not supposed to be run at all and that happened likely because I've opened this PR against master and closed it and then reopened another one against rel/2.0.0 with the same commit hash

@weshaggard

Copy link
Copy Markdown
Member

Looking at the CI legs that kicked off they are the expected set for release/2.0.0 branch (at least for now until we move the CI changes to the release branch). Please don't ignore the results for these CI legs make sure they are understood.

@krwq

krwq commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

@weshaggard I think current set looks good, previously it was trying to merge this PR with master branch causing merge conflicts

@mmitche

mmitche commented Jun 1, 2017

Copy link
Copy Markdown
Member

@krwq I was looking for the ones that said "Portable Linux x64 Debug Build"/"Portable Windows x64 Debug Build" etc. that happened to run on that one PR. Beyond that, everything else here should pass

@danmoseley

Copy link
Copy Markdown

Waiting on #20595 to reset CI here.

morganbr and others added 14 commits June 1, 2017 15:14
Fixes tests that depend on types in System.Private.CoreLib being serializable that won't be serializable soon.
* Changes to throw PlatformNotSupportedException from ISerializable.GetObjectData and serialization constructors on non-serializable types. Also removes private serialization constructors and some unneeded code that was used to support serializing non-serializable types. A few tests testing GetObjectData implementations are also removed.

* Address code review comments.

* Change exceptions' GetObjectData to just call base rather than throw. This makes them behave consistently with exceptions that didn't override GetObjectData.
Adds serialization support to BigInteger and Complex, which were inadvetently dropped in the previous cleanup. Also renames the private fields of Complex to match NetFX so that it will be serialization compatible.
Removes a couple more tests that depend on ISerializable implementations and fixes a diagnosability issue in DataContractSerializer tests
Disable lazy serialization tests on netcoreapp
Add tests for reflection based serialization of coretypes
@krwq
krwq force-pushed the merge-serialization-changes-to-2.0.0 branch from 949d810 to 4dc0237 Compare June 1, 2017 22:17
@danmoseley
danmoseley merged commit 21d7293 into dotnet:release/2.0.0 Jun 1, 2017
@karelz karelz modified the milestone: 2.0.0 Jun 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

* NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants