Skip to content
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

First attempt at using profile 259 for Google.Protobuf. #638

Merged
merged 4 commits into from Jul 29, 2015

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Jul 23, 2015

This requires .NET 4.5, and there are a few compatibility changes required around reflection.
Creating a PR from this to see how our CI systems handle it. Will want to add more documentation,
validation and probably tests before merging.

This is in aid of issue #590.

@jtattermusch FYI

@jtattermusch
Copy link
Contributor

Interesting stuff, let me know when this is ready to be reviewed. By dropping .NET 4 support, are we expecting do disqualify some user group from using C# protobufs?
Btw, how about Unity? I've never used it but I think it uses some ancient version of .NET.

@jskeet
Copy link
Contributor Author

jskeet commented Jul 25, 2015

I'm not worried by Windows desktop users - the vast, vast majority will be on 4.5 by now.
Unity is a good point though - will investigate over the weekend.

This requires .NET 4.5, and there are a few compatibility changes required around reflection.
Creating a PR from this to see how our CI systems handle it. Will want to add more documentation,
validation and probably tests before merging.

This is in aid of issue protocolbuffers#590.
@jskeet
Copy link
Contributor Author

jskeet commented Jul 27, 2015

I think it'll be worth tidying up this PR in terms of documentation, then merging it.

I suspect it won't work with Unity, but we can add another build target for that later when we find a need - we can create a new minor version of the NuGet package for that with no problems. My experience with the "old" port is that Unity on iOS has various issues, so I think we'll want to investigate them pretty thoroughly before trying to support it anyway - and we may need to conditionally compile out some features. We'll see... but I'd like to avoid having Unity support block a dotnetcore release.

I'll add the docs and tests to this PR tomorrow (UK time).

@jskeet
Copy link
Contributor Author

jskeet commented Jul 27, 2015

Having said that, this Travis failure is none too promising:

++mono ../../nuget.exe restore
Native stacktrace:
mono() [0x4b20bc]
mono() [0x5086ee]
mono() [0x428f7d]
/lib/x86_64-linux-gnu/libpthread.so.0(+0xfcb0) [0x7f6232ea7cb0]
mono() [0x517335]
mono() [0x524a00]
mono() [0x4b4898]
[0x41445046]

Debug info from gdb:

Got a SIGSEGV while executing native code. This usually indicates
a fatal error in the mono runtime or one of the native libraries

used by your application.

@jtattermusch
Copy link
Contributor

That's a flake in nuget + mono combo I've seen happening in the past, but it doesn't happen very often. So my guess is after rerunning the test you'll be fine (and this flake is probably something we'll have to live with).

@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="4.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<EnvironmentFlavor>CLIENTPROFILE</EnvironmentFlavor>
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need the CLIENTPROFILE declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think so. There may be all kinds of stuff in the build file that's left over from more complicated times. Will have a look at that at the same time.

@jtattermusch
Copy link
Contributor

Changes LGTM.
Some more nits:
-- one comment above
-- please also update the .nuspec file so that's it's in sync with the changes to profile
-- please update the list of supported platforms in the README.md file

After that, I think we should merge this.

@jtattermusch jtattermusch self-assigned this Jul 27, 2015
@jskeet
Copy link
Contributor Author

jskeet commented Jul 27, 2015

Will do. IIRC, the nuspec file is already in sync with this (and out of sync with the current profile) but I'll check tomorrow.

@jskeet
Copy link
Contributor Author

jskeet commented Jul 28, 2015

Hmm... http://blogs.msdn.com/b/lucian/archive/2015/07/27/writing-a-nuget-package-for-vs2015-rtm-and-uwp.aspx suggests there may be more answers in a few days.

- Fix nupec paths
- Remove an obsolete part of the JSON build
- Add documentation and tests to reflection extension methods, and improve implementations
@jskeet
Copy link
Contributor Author

jskeet commented Jul 28, 2015

Right, this is ready for merging after further review. (The implementation has changed, so it's worth a review.)

As preparation for checking that it's likely to work in terms of .NET Core, I'm going to make the same sorts of changes in Noda Time 2.0 and push an alpha release of that, then try using that in a .NET Core project.

@jskeet
Copy link
Contributor Author

jskeet commented Jul 28, 2015

Had missed the bit about supported platforms before - I've added an extra section in the csharp/README.md file; I'm not sure whether that's what you meant though... I couldn't see an existing supported platform list to update.

@Flavien
Copy link

Flavien commented Jul 29, 2015

Do you have an ETA of when this will get released to NuGet?

@@ -22,6 +22,17 @@ Use `protoc` with the `--csharp_out` option to generate C# files in the specifie
Include these in your C# project, and add a reference to the `Google.Protobuf` project. Currently
there is no NuGet package for this, but we will be building one as soon as the API is stable.

Supported platforms
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I suspected we don't have a supported platforms section, it's great you've added it. Thanks!

@jtattermusch
Copy link
Contributor

LGTM.

jtattermusch added a commit that referenced this pull request Jul 29, 2015
First attempt at using profile 259 for Google.Protobuf.
@jtattermusch jtattermusch merged commit 74810c6 into protocolbuffers:csharp-experimental Jul 29, 2015
@jtattermusch
Copy link
Contributor

@Flavien We don't have a specific ETA unfortunately, but we are hoping to release this as nuget package soon. Stay tuned.

@jskeet jskeet deleted the portable branch July 30, 2015 08:48
@Flavien
Copy link

Flavien commented Aug 9, 2015

Thanks. Do you know if that is a matter of days/weeks/months?

@jskeet
Copy link
Contributor Author

jskeet commented Aug 9, 2015

I would estimate a small number of weeks, but that's only an estimate.

@jtattermusch
Copy link
Contributor

@Flavien I've created an UNOFFICIAL preview nuget package on my personal fork of this repo https://github.com/jtattermusch/protobuf/releases/tag/unofficial20150808

You can download the preview nuget package there and install it locally. Feedback is welcome.

@Flavien
Copy link

Flavien commented Aug 20, 2015

I've tried it, the package runs smoothly with .NET Core. I haven't noticed any issue so far, apart from the fact that apparently Protobuf v2 is no longer supported so I had to migrate my schemas to version 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants