In/outgoing Span<byte> support, linux net45 build #589
Conversation
|
It's worthwhile to mention it might appear that I've removed a couple of input validation checks in the generated code, but this is not really so. Those checks happen anyway inside the constructor of |
|
Any comments about this? |
|
My only question would be whether there was test cases for those exceptions?
Bill
…On Sun, 7 Oct 2018, 5:28 PM damageboy ***@***.***> wrote:
Any comments about this?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFI1pEyacdIkHwWpnrdIBQEwGWwajJlCks5uia0jgaJpZM4XCCgM>
.
|
|
I'll try figuring that out, I admin I worked under the assumption that tests run as part of gradle, I'm suddenly not so sure |
|
Thanks for the PR. Looks good to me, in general. :-) My understanding is that only the code generation happens inside the Gradle script not the building/testing of the C# solution. I have a few questions. I'm not sure who is best placed to answer them (maybe @billsegall ?).
|
|
Also...
|
|
We're currently targeting the nuget packages at netstandard2.0 and have
dropped 1.1. No-one complained but that doesn't mean they won't. In general
I'm happy to maintain a reasonable amount of backward compatibility but MS
made this very difficult pre 2.0.
Similarly 4.5 would seem to me to be a reasonable minimum at this point but
I'd be happy to address this if anyone really needed it. In the absence of
a demonstrated need I'd leave things at that,
Bill.
Bill.
…On Mon, Oct 8, 2018 at 7:54 PM Zach Bray ***@***.***> wrote:
Thanks for the PR. Looks good to me, in general. :-)
My understanding is that only the code generation happens inside the
Gradle script not the building/testing of the C# solution.
I have a few questions. I'm not sure who is best placed to answer them
(maybe @billsegall <https://github.com/billsegall> ?).
1.
Could the introduction of the System.Memory dependency affect anyone?
I.e., could users have been using .NET Framework < 4.5 to build the
SBE generated code prior to this change? I guess the lower limit is
currently whatever the sbe-tool package
<https://www.nuget.org/packages/sbe-tool/0.1.8.1-beta-1> targets. The
NuGet site doesn't make this obvious for this particular package.
2.
If users are affected, are we still happy to make the change? Or
should we introduce a switch into the code generation that enables Span
support? I think Olivier suggested this in #587
<#587>.
3.
Do we use any features in netstandard2.0 that aren't in netstandard1.1?
I ask as it looks like netstandard1.1 is supported by the
System.Memory package <https://www.nuget.org/packages/System.Memory/>
and according to the .NET Standard compatibility matrix
<https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support>
this is supported by .NET Framework v4.5. Would this mean we could simplify
the build somewhat?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFI1pL_l0ybxFqKV_0vR3czwF-Ee9fmvks5uiyDogaJpZM4XCCgM>
.
|
|
Release notes are a good idea. How do people feel about an additional
section in ~csharp/README.md?
Bill.
…On Mon, Oct 8, 2018 at 7:55 PM Zach Bray ***@***.***> wrote:
Also...
1. Do we keep release notes somewhere?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFI1pBTjbjtzQbYLbvG3tkN9eE-t4GhOks5uiyEBgaJpZM4XCCgM>
.
|
|
@billsegall @ZachBray I didn't think about the README.md inside the csharp folder, and see no reason not to amend this commit to reflect that dependency as well.. As for .NET 4.5 as minimum I think its a fair limitation given that official support is practically non existent for anything below 4.5.2 on the one hand, and .NET Framework 4.5 was released on 15 August 2012 on the other hand. This is especially true for a performance / latency oriented project like this where you practically get free perf boost just for moving to .NET Core. In other words, to me, it seems reasonable to say optimal perf is on .NET Core, compat for whatever reason you might need it goes back to 2012. |
|
The bulk of the notes can be in the csharp/README.md which can be referenced from the wiki. |
I didn't realise that. Fair enough! :-)
Assuming the consumers are not building from source (is this a fair assumption?), this means we support https://github.com/real-logic/simple-binary-encoding/blob/master/csharp/sbe-dll/sbe-dll.csproj#L4 I.e., only target |
|
@damageboy apologies for polluting your PR with these comments! They probably belong somewhere else. |
|
@ZachBray As I'm now knee deep in figuring out how the unit tests work, it's actually good timing I think. To be quite honest, I think I should simply run the tests as is manually on windows and give a scout's honor promise the tests are green. Other than that, I think the tests require their own major overhaul in a separate PR, |
damageboy
force-pushed
the
csharp/span
branch
from
1c25ed3
to
dd54c66
Compare
- Closes real-logic#587 - Add netfx.props and tweak sbxee-dll csproj to allow building sbe.dll for net45 on Linux using dotnet sdk following: https://andrewlock.net/building-net-framework-asp-net-core-apps-on-linux-using-mono-and-the-net-cli/ - Remove packages.config files from projects already converted to SDK style project since those files serve no purpose other than confusing potential contributors... - Intoroduce minimal Span<byte> GetBytes/SetBytes implementation for copying to/from Span<byte> to sbe.dll itself. - Change code generation for array types to use Span<byte> internally keeping the original functionality intact with no (hopefully!) visible user facing changes except for the addtitional Get/Set methods accepting Span<byte> being added to the mix. - Add release notes section to README.md and some more touch ups - Add sbe-tests.csproj netcoreapp2.1 test target
damageboy
force-pushed
the
csharp/span
branch
from
dd54c66
to
5223d74
Compare
|
Can @JPWatson, @odeheurles, or @billsegall let me know if this should be merged or not? |
|
I've completed a manual build/test with the patch and it's working for me
on Windows building with VS2017. I'll probably freshen the nuget package
following the release of 1.9.1. I didn't add any validation tests using
the the new code.
Bill.
…On Sat, Nov 10, 2018 at 7:10 AM Martin Thompson ***@***.***> wrote:
Can @JPWatson <https://github.com/JPWatson>, @odeheurles
<https://github.com/odeheurles>, or @billsegall
<https://github.com/billsegall> let me know if this should be merged or
not?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFI1pCUyKAObXW_lnaL-RrelMZtIn3Qwks5ute8_gaJpZM4XCCgM>
.
|
|
|
||
| sb.append(String.format("\n" + | ||
| indent + "public %1$s Get%2$s(int index)\n" + | ||
| indent + "{\n" + | ||
| indent + INDENT + "if (index < 0 || index >= %3$d)\n" + | ||
| indent + INDENT + "{\n" + | ||
| indent + INDENT + "if (index < 0 || index >= %3$d) {\n" + |
There was a problem hiding this comment.
Why change the generated format here? Will this not now be inconsistent?
for net45 on Linux using dotnet sdk following:
https://andrewlock.net/building-net-framework-asp-net-core-apps-on-linux-using-mono-and-the-net-cli/
those files serve no purpose other than confusing potential contributors...
to/from Span to sbe.dll itself.
original functionality intact with no (hopefully!) visible user facing changes except
for the addtitional Get/Set methods accepting Span being added to the mix.