.net core binary serialization fails #95
Comments
|
It seems Lazy isn't serializable anymore in .NET Core. I'm not sure how useful |
|
So should we remove the seralization tests because we are already testing
going from class to bytes to class?
Chris
…On Fri, May 31, 2019 at 9:18 AM Phyxion ***@***.***> wrote:
It seems Lazy isn't serializable anymore in .NET Core. I'm not sure how
useful [Serializable] is in the library itself though (which should then
be removed from everything), as I think you only want to save the
underlying byte array (which can be used to reconstruct the packets again
anyway).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#95>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJH4AGHOFXXJ4OTNI4FKV3PYEQTVANCNFSM4HRZ6NTQ>
.
|
|
I was thinking about removing serialization tests and also the |
|
I don’t understand the consequence of dropping that attribute. I’ll keep it
but remove the tests only for now.
Did you want to do that? Otherwise I’ll do that tonight to unblock the
conversion to .net core I have almost completed.
Chris
…On Fri, May 31, 2019 at 10:36 AM Phyxion ***@***.***> wrote:
I was thinking about removing serialization tests and also the
Serializable attribute from all classes in main project. I don't really
see a good use case for that as the properties are based on byte array
anyway and all you really need is to serialize is that. The only exception
to the removal of all Serializable attributes is perhaps the
ByteArraySegment as I can see the benefit of saving Length/Offset.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#95>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJH4AAWRHSMUDY3YD475LTPYEZYDANCNFSM4HRZ6NTQ>
.
|
|
The tests are OK. Lazy just isn't serializable in .NET Core, which means that the packet serialization isn't going to work either (for the same reason). I think removing the Serializable attribute would be OK, I don't think many will use it as you're much better of serializing the ByteArraySegment or the underlying array. The downside of simply removing all binary serialization tests is that most of these test methods are the only thing kind of validating the pcap files and their contained packet. So perhaps these should be changed to assert the expected value rather than comparing serialized vs deserialized. For example, this https://github.com/chmorgan/packetnet/blob/2f2f4208f2ee5efd707360bae969f64e7f198116/Test/PacketType/IPPacketTest.cs#L74 could check if ip.Protocol is ProtocolType.IPv4 (didn't check what it actually is). This is of course more work than removing these tests completely, but then there's at least some tests for packet correctness (expected values can be found in Wireshark). I don't have a lot of free time at the moment, so I hope this makes sense 👍 |
|
Ahh ok. I can take a look at that. There are 13 failing which likely means
several hours of work. I should be able to get to it this weekend. Have
some other packetnet work held up by this and I’d prefer to be able to work
on Mac, which doesn’t have good support for .net framework.
Chris
…On Fri, May 31, 2019 at 12:54 PM Phyxion ***@***.***> wrote:
The tests are OK. Lazy just isn't serializable in .NET Core, which means
that the packet serialization isn't going to work either (for the same
reason). I think removing the Serializable attribute would be OK, I don't
think many will use it as you're much better of serializing the
ByteArraySegment or the underlying array.
The downside of simply removing all binary serialization tests is that
most of these test methods are the only thing kind of validating the pcap
files and their contained packet. So perhaps these should be changed to
assert the expected value rather than comparing serialized vs deserialized.
For example, this
https://github.com/chmorgan/packetnet/blob/2f2f4208f2ee5efd707360bae969f64e7f198116/Test/PacketType/IPPacketTest.cs#L74
could check if ip.Protocol is ProtocolType.IPv4 (didn't check what it
actually is). This is of course more work than removing these tests
completely, but then there's at least some tests for packet correctness
(excepted values can be found in Wireshark). I don't have a lot of free
time at the moment, so I hope this makes sense 👍
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#95>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJH4AGPHDXLGOJM4ZWNPYLPYFJ45ANCNFSM4HRZ6NTQ>
.
|
|
@Phyxion I took a look and didn't see anything that made sense to preserve. I did have one question. You flagged Echo as obsolete. Should we drop support for that option from TcpPacket.cs? Otherwise there are warnings printed during the compile. Thoughts? |
|
The Echo was already flagged as Obsolete. I've not removed it yet because while RFC states it's obsolete, I'm not sure if it needs to be removed from the library. Perhaps it would be better to drop the Obsolete attribute instead. Also created PR #96 for these changes so it's easier to keep track on and allow AppVeyor to show results :) |
I'm working to convert the tests from a .net framework target to .net core so it is compatible with windows, Mac, and linux again without using mono. Binary serialization is a lot different in .net core, see https://github.com/dotnet/corefx/issues/23213 and when running as a .net core executable there are several failures like:
should we drop these tests? What is the benefit/purpose of keeping these serialization tests working? @Phyxion any thoughts?
The text was updated successfully, but these errors were encountered: