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

Codegen with ProtoGen v2.3.4 ONLY support C# 6+ #343

Closed
huangqingcheng opened this Issue Jan 23, 2018 · 21 comments

Comments

Projects
None yet
2 participants
@huangqingcheng

huangqingcheng commented Jan 23, 2018

I use v2.3.4 ProtoGen to convert one proto3 file to .cs, then it report that only support C# 6+.

Each string field have a '= ""' string, like:

public string Build { get; set; } = "";

This language feature only work at C# 6+.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 23, 2018

This isn't a .NET version thing - it is a C# version thing; newer C# compilers can target older .NET frameworks without difficulty.

If you're saying "we should allow for down-level compilers as an option"; we can consider that - but: how low do we go? should we support not using automatically implemented properties (C# 3), for example?

I would also want to ask: is there a specific reason you can't use (or don't want to use) a more recent compiler? This might significantly influence the priority.

@mgravell mgravell changed the title from Codegen with ProtoGen v2.3.4 ONLY support .Net 6+ to Codegen with ProtoGen v2.3.4 ONLY support C# 6+ Jan 23, 2018

@huangqingcheng

This comment has been minimized.

huangqingcheng commented Jan 23, 2018

I use protobuf-net in Unity 2017.03 and use .net 4. Because of APK size, I need to use low-level .Net. If it can support C# 3.5+, must be a good news.

Are there any other people have the same problems?

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 23, 2018

k; so: what is the maximum language version supported by that? (not the .NET version - what matters is the C# version; there is no such thing as C# 3.5, for example).

@huangqingcheng

This comment has been minimized.

huangqingcheng commented Jan 24, 2018

According to wikipedia, .NET Framework 3.5 related to C# 3.0.

If C# 3.0 is too low, I think C# 4.0 is enough.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 24, 2018

@huangqingcheng

This comment has been minimized.

huangqingcheng commented Jan 24, 2018

I admire you for the rigorous programming.

If a library claims to support .NET Framework 4.0, for others, it means that it supports compiling with the C# 4 compiler or higher.

What I need is that .cs file can supports compiling with the C# 4 compiler, if this is reasonable.

For Unity developers, everyone can choose to use many versions of .NET themselves, but given the size of the binary package, he may be wise to choose a higher version.

I'm a Unity newbie and I'm not very familiar with how to set the C # compiler version, and I'll be actively trying to raise the compiler version if I can.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 24, 2018

Are you using the online generator (https://protogen.marcgravell.com)? or have you compiled the standalone exe?

If you are using the online generator, you can try this now; at the moment it requires specification inside the proto file, but I need to change how it works because obviously a single "langver" isn't useful for x-plat purposes; so: expect this to change, but for today at least, you can add:

and

import "protobuf-net/protogen.proto";
option (.protobuf_net.fileopt).langver = "3";

to a schema in the online version, and it should emit C# 3 compatible code. I'd be interested in your results - i.e. does it compile correctly for you. Note that the generator doesn't make any use of C# 4 or C# 5 features, so the only real question at the moment is "C# 6 vs C# 3".

You can see an example of this here: https://protogen.marcgravell.com/#gbc9d82f8397fa022286f15a2247f2d4c

(which is just the default page, loading this gist)

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 24, 2018

Small disagreement on one thing though: I fundamentally disagree with the following statement:

If a library claims to support .NET Framework 4.0, for others, it means that it supports compiling with the C# 4 compiler or higher.

I don't want to belabour the point, but: the language version and the C# version are IMO fundamentally disconnected. It isn't an unreasonable request to support earlier compiler versions - and as you can see: I'm not against that in any way. But: they are separate questions.

For most purposes, perhaps this is a moot point...

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 24, 2018

Oh; if you cannot edit your existing .proto, I'd be interested in hearing that too - it means I need to add additional override flags to the web UI and console CLI. Again: let me know how you get on, and whether this is going to be problematic.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 24, 2018

(if compiling from source: see ^^^, or the langver branch)

@huangqingcheng

This comment has been minimized.

huangqingcheng commented Jan 24, 2018

My .proto file look like:

syntax = "proto3";

package demo;

message Error {
    uint32 code = 1;
    string text = 2;
}

message Server {
    uint32 id = 1;
    string addr = 2;
}

message ExampleResp {
    Error err = 1;
    repeated Server servers = 2;
}
// This file was generated by a tool; you should avoid making direct changes.
// Consider using 'partial classes' to extend these types
// Input: my.proto

#pragma warning disable 1591, 0612, 3021

namespace Demo
{

    [global::ProtoBuf.ProtoContract()]
    public partial class Error
    {
        [global::ProtoBuf.ProtoMember(1, Name = @"code")]
        public uint Code { get; set; }

        [global::ProtoBuf.ProtoMember(2, Name = @"text")]
        [global::System.ComponentModel.DefaultValue("")]
        public string Text { get; set; } ~~= "";~~

    }

    [global::ProtoBuf.ProtoContract()]
    public partial class Server
    {
        [global::ProtoBuf.ProtoMember(1, Name = @"id")]
        public uint Id { get; set; }

        [global::ProtoBuf.ProtoMember(2, Name = @"addr")]
        [global::System.ComponentModel.DefaultValue("")]
        public string Addr { get; set; } ~~= "";~~

    }

    [global::ProtoBuf.ProtoContract()]
    public partial class ExampleResp
    {
        [global::ProtoBuf.ProtoMember(1, Name = @"err")]
        public Error Err { get; set; }

        [global::ProtoBuf.ProtoMember(2, Name = @"servers")]
        public global::System.Collections.Generic.List<Server> Servers { get; } ~~= new global::System.Collections.Generic.List<Server>();~~

    }

}

#pragma warning restore 1591, 0612, 3021

Then I need to manually remove those code snippets enclosed in ~~.

Other solutions are being explored.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 24, 2018

If I run your .proto through the new code, with the:

import "protobuf-net/protogen.proto";
option (.protobuf_net.fileopt).langver = "3";

then I get:

        [global::ProtoBuf.ProtoMember(2, Name = @"servers")]
        public global::System.Collections.Generic.List<Server> Servers { get; private set; }

with:

        public ExampleResp()
        {
            Servers = new global::System.Collections.Generic.List<Server>();
            OnConstructor();
        }

        partial void OnConstructor();

Likewise:

        [global::ProtoBuf.ProtoMember(2, Name = @"addr")]
        [global::System.ComponentModel.DefaultValue("")]
        public string Addr { get; set; }

with:

        public Server()
        {
            Addr = "";
            OnConstructor();
        }

        partial void OnConstructor();

which ... looks good to me; so: does the change deployed fix the issue for you?

@mgravell

This comment has been minimized.

@huangqingcheng

This comment has been minimized.

huangqingcheng commented Jan 24, 2018

Using the the online generator, everything looks well. However, x-plat issues are also a problem. I have enough time and patience to wait for the ultimate solution. If there is anything I can help, please let me know.

I use a standalone exe to generate group files. At the same time, also use protoc to generate Go files for server side.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 24, 2018

k; do you see it as an issue having the hints for this in the .proto? i.e. if this became:

option (.protobuf_net.fileopt).csharp_langver = "3";

would that be usable for you? My main concern with langver as a name is simply: it doesn't make sense when it could target multiple platrforms. The above approach has precedent with csharp_namespace from Google's base code, etc.

@huangqingcheng

This comment has been minimized.

huangqingcheng commented Jan 25, 2018

protoc will still report Option "(.protobuf_net.fileopt)" unknown if only option is used.

If possible, I think it would be more appropriate to put langver on protogen startup parameters.

protogen.exe --langver=3 --csharp_out=.  demo.proto

I'm not sure protogen supports generating other languages, which may not be appropriate if supported.

Of course, if put the langver in .proto file, does not affect the use of protoc, it is ok so.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 25, 2018

Yes, it is entirely expected that you'd still have to use the import; I was mainly talking about the naming. I'll have a think about command-line options.

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 25, 2018

k; I've pushed a change to the "langver" branch; if you pull from that, you should find that:

+langver=3.0

works at the command-line. More generally, +OPTION=VALUE allows arbitrary custom options to be provided through to the code-generator, so we have a consistent API for adding any other future needs.

(as opposed to -/-- switches which mimic the google protoc CLI)

mgravell added a commit that referenced this issue Jan 25, 2018

Langver (#344)
* #343 - support earlier compiler versions (incomplete)

* rename langver => csharp_langver

* update site to protoc 3.5.1

* update samples to current protoc

* allow command-line and API usage to specify custom options; "langver" and "names" supported directly
@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 25, 2018

btw; protogen is now available as a download on https://protogen.marcgravell.com/ - you are of course entirely welcome to self-build if you prefer

@mgravell

This comment has been minimized.

Owner

mgravell commented Jan 25, 2018

setting a vague and imprecise timer: I'm going to assume this is now resolved unless you get back to me in a few days

@huangqingcheng

This comment has been minimized.

huangqingcheng commented Jan 26, 2018

It works well now, nice.

@mgravell mgravell closed this Jan 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment