Update to latest protogen

[xPaw]

I patched protogen with the following:

• Skip generating Extensions
• Always generate List<> instead of arrays types to match current behaviour
• Force proto2 if syntax is not specified

The patch file is attached in the PR.

Things that need to be done:

• Fix DefaultValue - DefaultValue() generated incorrectly for big ulong values protobuf-net/protobuf-net#495 Fix [DefaultValue(...)] with large uint64 value being interpreted as floating point protobuf-net/protobuf-net#516
• Generate interfaces for unified services like IPlayer - Service interfaces are not generated with protogen protobuf-net/protobuf-net#487 Poor implementation of service interfaces in c# generator protobuf-net/protobuf-net#492

[codecov-io]

Codecov Report

Merging #628 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
+ Coverage   23.59%   23.65%   +0.05%     
==========================================
  Files          85       85              
  Lines        8709     8718       +9     
  Branches      719      719              
==========================================
+ Hits         2055     2062       +7     
- Misses       6523     6525       +2     
  Partials      131      131

Impacted Files	Coverage Δ
SteamKit2/SteamKit2/Steam/CDNClient.cs	0% <0%> (ø)	⬆️
...mClient/Configuration/SteamConfigurationBuilder.cs	100% <0%> (ø)	⬆️
SteamKit2/SteamKit2/Util/HardwareUtils.cs	25.83% <0%> (+0.34%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec01c92...2886f64. Read the comment docs.

[yaakov-h]

I really don't like patching the binaries, and I don't think it will be maintainable going forwards.

Can we follow the original suggestion from the other issue and have our own generator frontend that's more tailored for our use?

[xPaw]

The patch is a proof of concept, as it currently compiles steamkit but throws during serialisation.

[xPaw]

Okay, so I figured out why it failed to serialize with new protogen. It generates the following attribute for some properties:

[global::System.ComponentModel.DefaultValue(18446744073709551615)]

And for whatever reason that makes protobuf-net fail to serialize it with Arithmetic operation resulted in an overflow.. I patched protogen so it no longer generates that and it appears to work now.

Old protogen didn't generate that, updating protobuf-net doesn't fix it, so I suspect it's a bug.

The following code throws, and removing ComponentModel.DefaultValue makes it work again.

        [global::ProtoBuf.ProtoMember(10, DataFormat = global::ProtoBuf.DataFormat.FixedSize)]
        [global::System.ComponentModel.DefaultValue(18446744073709551615)]
        public ulong jobid_source
        {
            get { return __pbn__jobid_source ?? 18446744073709551615; }
            set { __pbn__jobid_source = value; }
        }

@mgravell Would you have any idea what could be going on here?

[mgravell]

Oof, yeah, that's just a huge number overflow. I'm *guessing* that adding UL as a suffix to the number will fix it. Any chance you could test that hypothesis? I can update protogen if it is that.

…

On Wed, 23 Jan 2019, 13:30 Pavel Djundik ***@***.*** wrote: Okay, so I figured out why it failed to serialize with new protogen. It generates the following attribute for some properties: [global::System.ComponentModel.DefaultValue(18446744073709551615)] And for whatever reason that makes protobuf-net fail to serialize it with Arithmetic operation resulted in an overflow.. I patched protogen so it no longer generates that and it appears to work now. Old protogen didn't generate that, updating protobuf-net doesn't fix it, so I suspect it's a bug. The following code throws, and removing ComponentModel.DefaultValue makes it work again. [global::ProtoBuf.ProtoMember(10, DataFormat = global::ProtoBuf.DataFormat.FixedSize)] [global::System.ComponentModel.DefaultValue(18446744073709551615)] public ulong jobid_source { get { return __pbn__jobid_source ?? 18446744073709551615; } set { __pbn__jobid_source = value; } } @mgravell <https://github.com/mgravell> Would you have any idea what could be going on here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#628 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDsOMzQvpnXJNSQFvHP2tOYT714Npgks5vGGQRgaJpZM4aIfZp> .

[xPaw]

Adding UL does not fix it, and I just checked, it passes the value into DefaultValue(float) overload, as there doesn't appear to be any unsigned overloads: https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.defaultvalueattribute.-ctor?view=netframework-4.7.2

EDIT: Doing DefaultValue(typeof(ulong), "18446744073709551615") doesn't throw, but I'm unsure if that's correct.

[mgravell]

K, I'll need to take a look at options, then. I guess at a push the type+string overload might work. Sheesh, all more fun.

…

On Wed, 23 Jan 2019, 14:02 Pavel Djundik ***@***.*** wrote: Adding UL does not fix it, and I just checked, it passes the value into DefaultValue(float) overload, as there doesn't appear to be any unsigned overloads: https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.defaultvalueattribute.-ctor?view=netframework-4.7.2 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#628 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDsPMw3NRJVuid3sBNHCagxXZ2D5Obks5vGGtmgaJpZM4aIfZp> .

[xPaw]

Turns out protogen already had the underlying support for writing services, the c# generator simply didn't implement them. So I implemented the interface generation, it skips any options though, but it should be good enough for SteamKit.

Technically speaking this is now fully compatible with SteamKit (I'm not sure if skipping DefaultValue will have any issues or not).

[mgravell]

@xPaw I'd be happy to look at a PR, if the service code would be more generally useful

[xPaw]

Yeah I made a PR in the protobuf repo. protobuf-net/protobuf-net#492

[mgravell]

ah, k; I'll check that later; btw - if you don't want to keep having to upload binaries, all of the codegen code is available as a package ref to protobuf-net.Reflection - protogen is a trivial wrapper that just nudges the lib

[xPaw]

Well the Reflection binary is where all the patches are.

[yaakov-h]

Fixing [DefaultValue] seems like it should be a trivial PR to protobuf-net.

We can't easily implement service generation ourselves (by subclassing ProtoBuf.Reflection.CSharpCodeGenerator) because we need access to some private and internal members such as IType, NullIfInherit and FindNameFromCommonAncestor.

We can do it, but with reflection hackery and duplicated code copied from from protobuf-net.Reflection.

@mgravell Can we make those types/members part of public API, or do we have to upstream this, keeping in mind that it may conflict with / need to be switchable with gRPC as you mentioned in protobuf-net/protobuf-net#492?

In the meantime I'm tempted to proceed with reflection...

[xPaw]

Closing in favor of #686