You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by vgotra <gi...@git.apache.org> on 2017/12/25 23:38:55 UTC
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
GitHub user vgotra opened a pull request:
https://github.com/apache/thrift/pull/1449
THRIFT-4434. Update .NET Core components, add tests for .Net Core library and .Net Core compiler, fix bugs and build process
This pull request should:
- highly improve the current version of .Net Core library and .Net Core compiler and quality of code
- improve and simplify build process
- improve documentation related to .Net Core library and compiler
- fix found bugs (some of bugs can be clarified like major - they are related to porting of protocols from Java version and can be present in C# library)
- add important unit tests for .Net Core library and .Net Core compiler
- add possibility to easy add unit tests for compiler for other languages
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vgotra/thrift netcore-standard2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1449.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1449
----
commit e3c255ff30d7aa6bcfc3fbaa5e3127698998a3d7
Author: Volodymyr Gotra <vo...@...>
Date: 2017-12-18T00:08:09Z
Initial merge of changes without fixes of builds
commit f6ced18b7021b72679af04b54da3019b34208a90
Author: Volodymyr Gotra <vg...@...>
Date: 2017-12-25T03:58:12Z
Fixed code generation from thrift files (for CLI and VS), fixed other issues
commit 3ad1dd065e6b71d69d583884c441f3a9e550dc0a
Author: Volodymyr Gotra <vg...@...>
Date: 2017-12-25T16:48:21Z
Updated build, readme, cmake files for tests
commit abc788df104ba6818fe5b9aaf13e2e5714fe791c
Author: Volodymyr Gotra <vg...@...>
Date: 2017-12-25T17:07:38Z
Updated copyrights
commit 2cf70c58d3f3081b026a852e2ab4a391297ed06d
Author: Volodymyr Gotra <vo...@...>
Date: 2017-12-25T21:52:40Z
Fixes for header file, updated project files, updated Makefiles
commit f55e4d9bd085fb5a8ecbc9b228d9a233bb9c4931
Author: Volodymyr Gotra <vg...@...>
Date: 2017-12-25T23:06:48Z
Added public interfaces generation for testing, updated readme
commit a8ad6ec61c9b6c4c56476e2bcd2aa484b4f0e298
Author: Volodymyr Gotra <vg...@...>
Date: 2017-12-25T23:24:03Z
Updated readme
commit d1d96857a72f26f840707807393260f5fc1face5
Author: Volodymyr Gotra <vo...@...>
Date: 2017-12-25T23:36:50Z
Added detection of Windows platform
----
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
I changed the lib/netcore/Makefile.am to this, maybe it's enough to do both:
```
SUBDIRS = .
all-local: \
$(DOTNETCORE) build
check-local: \
$(DOTNETCORE) test Tests\Thrift.Tests\Thrift.Tests.csproj
${DOTNETCORE} test Tests\Thrift.IntegrationTests\Thrift.IntegrationTests.csproj
clean-local:
$(RM) -r Thrift/bin
$(RM) -r Thrift/obj
EXTRA_DIST = \
README.md \
Tests \
Thrift \
Thrift.sln \
build.cmd \
build.sh \
runtests.cmd \
runtests.sh
```
I will test locally.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
The biggest fun - that csharp code for server tests are the same as it was regarding the Tls (it was simple Tls1 and it is Tls1). I will try to remove ((sender, certificate, chain, errors) => true, ) locally and test - if it will work then commit to check at build machines.
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1449
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161375213
--- Diff: lib/netcore/Makefile.am ---
@@ -19,86 +19,25 @@
SUBDIRS = .
-THRIFT = $(top_builddir)/compiler/cpp/thrift
-
-TESTDIR = Tests/Thrift.PublicInterfaces.Compile.Tests
-GENDIR = $(TESTDIR)/gen-netcore
-
THRIFTCODE = \
- Thrift/Thrift.csproj \
--- End diff --
.Net Core cli should automatically handle that - if file didn't change - it will not trigger the build - hope this will help you to spend less time on adding every file to list (autotools as I understand doesn't support wildcards)
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
I'll cleanup the autoconf issues I see here, and probably submit another PR for the squash and fixup to make sure it still works properly. This will probably take a little while though.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
@jeking3 - https://blogs.msdn.microsoft.com/vcblog/2017/11/02/visual-studio-build-tools-now-include-the-vs2017-and-vs2015-msvc-toolsets/ - hope this will help to move easier to VS2017 and with builds
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Hard to say - this can cause some time to make a single commit in manual way - if you know how I can make it fast and in simple way - then I can try to do it. Or we can simply re-run CI tests and make cherry-pick. Previously, it took half of day to understand where are problems during merge and what should be fixed - don't want to spend another day on resolving issues of merge in manual way (we can get some stupid errors and then spend some time to fix them).
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
Sure thing, I will take a look.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
Try this:
```
struct member_mapping_scope
{
public:
member_mapping_scope() : scope_member(0) { }
void* scope_member;
map<string, string> mapping_table;
};
```
```
// begin new scope
member_mapping_scopes.push_back(member_mapping_scope());
```
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
No it cannot. Few different tries - it seems that we will have few fixes and few more builds for only MSVC2010
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
How we can trigger checks? Is it possible to trigger them manually?
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
Currently the Ubuntu Xenial image which runs our cross tests in CI is installing dotnet-sdk-2.0.3, as is our Ubuntu Artful image. The latest is dotnet-sdk-2.1.4. Normally we like to have an older language level in the mix somewhere; so if your changes work properly on 2.0.3 that's great; we should update the artful Dockerfile to 2.1.4 in that case, so we have a "make check" environment on the latest SDK.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Thank you - updated :)
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Something at https://travis-ci.org/apache/thrift/jobs/328143179 - but locally I cannot reproduce this problem - also no logs at Travis CI.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
@jeking3 - It seems that fix for csharp lib tests with Tls1.2 can take more time and investigation because of possible situation that mono implements Tls1 - 1.2 in very specific way.
Few issues related to that:
- https://bugzilla.xamarin.com/show_bug.cgi?id=42805
- https://forums.xamarin.com/discussion/57683/tls-1-2-support
- http://www.mono-project.com/docs/about-mono/releases/4.8.0/#tls-12-support
- https://boringssl.googlesource.com/boringssl/
So, the simplest fix - I just reused Tls1 for .Net Core tests (by default for .Net Core implementation uses Tls 1.2).
At least, .Net Core client will be able to connect to old csharp server. But to fix a csharp client, a lot of checks should be done, also maybe some specific modification of build process (building of boringssl, etc.) should be done too.
Updated known_failures_Linux.json with issues related to described problem.
I think checking fix for this bug in csharp/mono is for another task and time.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
What do you mean - pass as client but hang as a server - do you mean - client can be like Python and server as .Net Core ?
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Thank you - maybe I missed that during fixes for pull request (also - I changed csproj files to run generation of code during a some build targets for all platforms) - so now we can skip execution of thrift in autotools to generate code - dotnet will do that automatically for csproj files for netcore library.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
See: https://github.com/apache/thrift/blob/master/build/docker/README.md
Also, I am testing Ubuntu Artful with dotnet-sdk-2.1.4 which pulls in netcore-2.0.5 (we're on 2.0.2).
We may be able to pull this dotnet sdk into the xenial image just as easily, especially if it fixes netcore server cross tests.
I'd like to continue to run the cross test suite on the LTS release. Fortunately there's another coming out in a few months.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Didn't imagine that we still support MSVC2010 - almost every compiler was able to build the source code but MSVC2010 cannot - hope people will move from this to something more modern and improved. Let check results of build for MSVC2010.
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161375244
--- Diff: lib/netcore/Makefile.am ---
@@ -19,86 +19,25 @@
SUBDIRS = .
-THRIFT = $(top_builddir)/compiler/cpp/thrift
-
-TESTDIR = Tests/Thrift.PublicInterfaces.Compile.Tests
-GENDIR = $(TESTDIR)/gen-netcore
-
THRIFTCODE = \
- Thrift/Thrift.csproj \
- Thrift/ITAsyncProcessor.cs \
- Thrift/ITProcessorFactory.cs \
- Thrift/SingletonTProcessorFactory.cs \
- Thrift/TApplicationException.cs \
- Thrift/TBaseClient.cs \
- Thrift/TException.cs \
- Thrift/TMultiplexedProcessor.cs \
- Thrift/Collections/TCollections.cs \
- Thrift/Collections/THashSet.cs \
- Thrift/Properties/AssemblyInfo.cs \
- Thrift/Protocols/ITProtocolFactory.cs \
- Thrift/Protocols/TAbstractBase.cs \
- Thrift/Protocols/TBase.cs \
- Thrift/Protocols/TBinaryProtocol.cs \
- Thrift/Protocols/TCompactProtocol.cs \
- Thrift/Protocols/TJSONProtocol.cs \
- Thrift/Protocols/TMultiplexedProtocol.cs \
- Thrift/Protocols/TProtocol.cs \
- Thrift/Protocols/TProtocolDecorator.cs \
- Thrift/Protocols/TProtocolException.cs \
- Thrift/Protocols/Entities/TField.cs \
- Thrift/Protocols/Entities/TList.cs \
- Thrift/Protocols/Entities/TMap.cs \
- Thrift/Protocols/Entities/TMessage.cs \
- Thrift/Protocols/Entities/TMessageType.cs \
- Thrift/Protocols/Entities/TSet.cs \
- Thrift/Protocols/Entities/TStruct.cs \
- Thrift/Protocols/Entities/TType.cs \
- Thrift/Protocols/Utilities/TBase64Utils.cs \
- Thrift/Protocols/Utilities/TProtocolUtil.cs \
- Thrift/Server/AsyncBaseServer.cs \
- Thrift/Server/TBaseServer.cs \
- Thrift/Server/TServerEventHandler.cs \
- Thrift/Transports/TClientTransport.cs \
- Thrift/Transports/TServerTransport.cs \
- Thrift/Transports/TTransportException.cs \
- Thrift/Transports/TTransportFactory.cs \
- Thrift/Transports/Client/TBufferedClientTransport.cs \
- Thrift/Transports/Client/TFramedClientTransport.cs \
- Thrift/Transports/Client/THttpClientTransport.cs \
- Thrift/Transports/Client/TMemoryBufferClientTransport.cs \
- Thrift/Transports/Client/TNamedPipeClientTransport.cs \
- Thrift/Transports/Client/TSocketClientTransport.cs \
- Thrift/Transports/Client/TStreamClientTransport.cs \
- Thrift/Transports/Client/TTlsSocketClientTransport.cs \
- Thrift/Transports/Server/THttpServerTransport.cs \
- Thrift/Transports/Server/TNamedPipeServerTransport.cs \
- Thrift/Transports/Server/TServerFramedTransport.cs \
- Thrift/Transports/Server/TServerSocketTransport.cs \
- Thrift/Transports/Server/TTlsServerSocketTransport.cs
+ Thrift/Thrift.csproj
all-local: \
Thrift.dll
Thrift.dll: $(THRIFTCODE)
- $(MKDIR_P) $(GENDIR)
- $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(TESTDIR)/CassandraTest.thrift
- $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/test/ThriftTest.thrift
- $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/contrib/fb303/if/fb303.thrift
- $(DOTNETCORE) --info
- $(DOTNETCORE) restore
+# $(MKDIR_P) $(GENDIR)
+# $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(TESTDIR)/CassandraTest.thrift
+# $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/test/ThriftTest.thrift
+# $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/contrib/fb303/if/fb303.thrift
$(DOTNETCORE) build
clean-local:
- $(RM) Thrift.dll
--- End diff --
Nice comment - maybe we should remove auto-generated files
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
If you want to force another CI build without code changes, you can:
$ git commit --amend --no-edit
$ git push (you may need to force push?)
That will trigger a new commit and therefore a new build.
If you want help with getting MSVC2010 building I can help with that, by pulling your changes into my windows system and using cmake to generate a visual studio 2010 solution, and getting it to build. The I can post the diff back here. Let me know if you want me to do that.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
I'll squash it then. For future reference there are instructions on squashing here:
https://thrift.apache.org/docs/HowToContribute
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
@jeking3 - pls check what version of mono is used for building csharp - it seems it very old (because it doesn't contain Tls11 and Tls12 in enumeration - error `System.Security.Authentication.SslProtocols' does not contain a definition for `Tls11')
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161375224
--- Diff: lib/netcore/Makefile.am ---
@@ -19,86 +19,25 @@
SUBDIRS = .
-THRIFT = $(top_builddir)/compiler/cpp/thrift
-
-TESTDIR = Tests/Thrift.PublicInterfaces.Compile.Tests
-GENDIR = $(TESTDIR)/gen-netcore
-
THRIFTCODE = \
- Thrift/Thrift.csproj \
- Thrift/ITAsyncProcessor.cs \
- Thrift/ITProcessorFactory.cs \
- Thrift/SingletonTProcessorFactory.cs \
- Thrift/TApplicationException.cs \
- Thrift/TBaseClient.cs \
- Thrift/TException.cs \
- Thrift/TMultiplexedProcessor.cs \
- Thrift/Collections/TCollections.cs \
- Thrift/Collections/THashSet.cs \
- Thrift/Properties/AssemblyInfo.cs \
- Thrift/Protocols/ITProtocolFactory.cs \
- Thrift/Protocols/TAbstractBase.cs \
- Thrift/Protocols/TBase.cs \
- Thrift/Protocols/TBinaryProtocol.cs \
- Thrift/Protocols/TCompactProtocol.cs \
- Thrift/Protocols/TJSONProtocol.cs \
- Thrift/Protocols/TMultiplexedProtocol.cs \
- Thrift/Protocols/TProtocol.cs \
- Thrift/Protocols/TProtocolDecorator.cs \
- Thrift/Protocols/TProtocolException.cs \
- Thrift/Protocols/Entities/TField.cs \
- Thrift/Protocols/Entities/TList.cs \
- Thrift/Protocols/Entities/TMap.cs \
- Thrift/Protocols/Entities/TMessage.cs \
- Thrift/Protocols/Entities/TMessageType.cs \
- Thrift/Protocols/Entities/TSet.cs \
- Thrift/Protocols/Entities/TStruct.cs \
- Thrift/Protocols/Entities/TType.cs \
- Thrift/Protocols/Utilities/TBase64Utils.cs \
- Thrift/Protocols/Utilities/TProtocolUtil.cs \
- Thrift/Server/AsyncBaseServer.cs \
- Thrift/Server/TBaseServer.cs \
- Thrift/Server/TServerEventHandler.cs \
- Thrift/Transports/TClientTransport.cs \
- Thrift/Transports/TServerTransport.cs \
- Thrift/Transports/TTransportException.cs \
- Thrift/Transports/TTransportFactory.cs \
- Thrift/Transports/Client/TBufferedClientTransport.cs \
- Thrift/Transports/Client/TFramedClientTransport.cs \
- Thrift/Transports/Client/THttpClientTransport.cs \
- Thrift/Transports/Client/TMemoryBufferClientTransport.cs \
- Thrift/Transports/Client/TNamedPipeClientTransport.cs \
- Thrift/Transports/Client/TSocketClientTransport.cs \
- Thrift/Transports/Client/TStreamClientTransport.cs \
- Thrift/Transports/Client/TTlsSocketClientTransport.cs \
- Thrift/Transports/Server/THttpServerTransport.cs \
- Thrift/Transports/Server/TNamedPipeServerTransport.cs \
- Thrift/Transports/Server/TServerFramedTransport.cs \
- Thrift/Transports/Server/TServerSocketTransport.cs \
- Thrift/Transports/Server/TTlsServerSocketTransport.cs
+ Thrift/Thrift.csproj
all-local: \
--- End diff --
Sure
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Cross langs tests passed fully - some of other builds seems just a curl problems (maybe because of impossibility to download dart signing keys).
So everything is good now :)
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Hm - for csharp for tests I enabled validation of any certificate to be successful
``` csharp
((sender, certificate, chain, errors) => true, )
```
in server tests code
Do you think I should revert it to check ?
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
To get netcore cross test running as a server you need to change "server-disabled" to "server" in test/tests.json, by the way...
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
It's already enabled - I removed disabled for netcore from tests.json
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Thanks - as I remember - I already merged (not re-based) some changes from master to pull request branch (in one or few previous commits) - not sure that this will correctly work with squash.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
One of the problems I had moving to netcore 2.0 was that the cross tests would pass as a client, but hang as a server, so I disabled them. If the cross tests pass that will go a long way to proving the benefit of these changes.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Yes, please - I tried almost everything to make it compiling (it seem that it doesn't support modern styles of initialization and instead of warning generates an error). It's hard to reproduce this locally - I need to create a lot of environments to check all compilations locally - maybe we can improve it later :) Just want to merge working changes to main repo - a lot of improvements in this pull request for .Net Core version and still a lot of things to do (integration and unit tests, improvement of CMake scripts, etc.)
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
@jeking3 - It seems that CLang doesn't recognize "-Wno-error=maybe-uninitialized". Do you know how to add some switches for different compilers ? - Error mostly because of one struct in header file (but to normally fix it it will take some time - unit tests for IDL much more important - already fixed few important bugs because of unit tetst for .Net Core generator) - easier to add suppression correctly and then fix it correctly.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
Okay, well, I squashed it all and found the following issues:
1. I don't see any netcore tests being run in the autotools build (make check). It builds some things but I don't see any test output?
```
Making check in netcore
make[2]: Entering directory '/thrift/src/test/netcore'
Making check in .
make[3]: Entering directory '/thrift/src/test/netcore'
/usr/bin/dotnet build
[?1h=[?1h=[?1h=[?1h=[?1h=[?1h=Microsoft (R) Build Engine version 15.4.8.50001 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.
[?1h=[?1h=[?1h=[?1h=[?1h= Thrift -> /thrift/src/lib/netcore/Thrift/bin/Debug/netstandard2.0/Thrift.dll
[39;49m[?1h=[33mThriftTest/Xception.cs(50,29): warning CS0114: 'Xception.Message' hides inherited member 'Exception.Message'. To make the current member override that implementation, add the override keyword. Otherwise add the new keyword. [/thrift/src/test/netcore/Server/Server.csproj]
[39;49m [WARNING:/thrift/src/test/ThriftTest.thrift:45] No generator named 'noexist' could be found!
[WARNING:/thrift/src/test/ThriftTest.thrift:47] cpp generator does not accept 'noexist' as sub-namespace!
[?1h=[39;49m[33mThriftTest/Xception.cs(50,29): warning CS0114: 'Xception.Message' hides inherited member 'Exception.Message'. To make the current member override that implementation, add the override keyword. Otherwise add the new keyword. [/thrift/src/test/netcore/Client/Client.csproj]
[?1h=[39;49m[?1h= [WARNING:/thrift/src/test/ThriftTest.thrift:45] No generator named 'noexist' could be found!
[WARNING:/thrift/src/test/ThriftTest.thrift:47] cpp generator does not accept 'noexist' as sub-namespace!
Server -> /thrift/src/test/netcore/Server/bin/Debug/netcoreapp2.0/Server.dll
Client -> /thrift/src/test/netcore/Client/bin/Debug/netcoreapp2.0/Client.dll
[39;49m[32m
Build succeeded.
[39;49m
[39;49m[33mThriftTest/Xception.cs(50,29): warning CS0114: 'Xception.Message' hides inherited member 'Exception.Message'. To make the current member override that implementation, add the override keyword. Otherwise add the new keyword. [/thrift/src/test/netcore/Server/Server.csproj]
ThriftTest/Xception.cs(50,29): warning CS0114: 'Xception.Message' hides inherited member 'Exception.Message'. To make the current member override that implementation, add the override keyword. Otherwise add the new keyword. [/thrift/src/test/netcore/Client/Client.csproj]
[39;49m[39;49m[33m 2 Warning(s)
[39;49m 0 Error(s)
[39;49m
Time Elapsed 00:00:10.96
make[3]: Leaving directory '/thrift/src/test/netcore'
make[2]: Leaving directory '/thrift/src/test/netcore'
```
2. The Makefile.am in lib/netcore, test/netcore, and tutorial/netcore have issues. They should always be running DOTNETCORE build instead of having any dependencies, and the EXTRA_DIST lists are incomplete.
I can clean up item #2 easily enough but I will need you to fix up item #1 or prove to me the unit tests are being executed.
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161374818
--- Diff: test/tests.json ---
@@ -1,689 +1,686 @@
-[
--- End diff --
Here's the diff
```
diff tests.json.old.normalized tests.json.new.normalized
222a223
> "header",
440c441
< "server-disabled": {
---
> "server": {
445,446c446
< "--no-restore",
< "--",
---
> "--project=Server\/Server.csproj",
456,457c456
< "--no-restore",
< "--",
---
> "--project=Client\/Client.csproj",
461c460
< "workdir": "netcore\/ThriftTest"
---
> "workdir": "netcore"
```
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
I work on a project that still uses 2010, but we're moving to 2017. So there are still folks using it. Boost supports back to Visual Studio 2008 in some cases, for example. I think Boost is going to tie support to EOL or EOS of the compiler. Extended support on Visual Studio lasts 10 years, so if we follow that we can reasonably drop 2010 support in 2020. :)
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
In the work I had done, the cross tests would pass when netcore was the client and another language was the server; but if I tried to run netcore server based cross tests they would randomly hang.
What I would recommend is that you remove all the netcore lines from the known failing linux tests file in test/ and see what passes and what fails once your changes are applied, and only add items back if absolutely necessary.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
I noticed that we can update the Dockerfile for artful to use dotnet-sdk-2.1.4 and it will pull in netcore 2.0.5, as opposed to 2.0.2 which is currently used, however we use the xenial image for cross tests right now. I am working on rebuilding the artful image and re-enabling haxe to see if we can get as many languages into artful as in xenial, then we can cut the cross test over to artful for CI.
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161373936
--- Diff: test/tests.json ---
@@ -1,689 +1,686 @@
-[
--- End diff --
By changing the indentation you have made it a very difficult task to merge this change. I have to find a way to diff these two sets and figure out what they are and apply them to the original.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Yes - it can be (if such self-signed certs are not trusted and if it not possible to build and validate the chain - but we cannot change that, because this will cause security problems at productions)
I'm waiting for the results of cross tests - if they will be successful - then we can say that everything is good.
Locally, I cannot reproduce any problem with set of few languages (cpp, c, csharp, rust, netcore, ...) for cross tests.
And you can approve current pull request :) Will be a good gift for people which want to reuse .Net Core Thrift and migrate their infrastructures to the latest stable stack for .Net
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161375191
--- Diff: test/features/known_failures_Linux.json ---
@@ -1,46 +1,48 @@
[
- "c_glib-limit_container_length_binary_buffered-ip",
--- End diff --
It's auto-formatting in vscode maybe
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
Would it be possible for you to squash this to a single commit, or does the commit history contain rebases?
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161372982
--- Diff: test/features/known_failures_Linux.json ---
@@ -1,46 +1,48 @@
[
- "c_glib-limit_container_length_binary_buffered-ip",
--- End diff --
In the future try not to reformat these files indentation; it makes it more difficult to understand what changed.
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161373426
--- Diff: lib/netcore/Makefile.am ---
@@ -19,86 +19,25 @@
SUBDIRS = .
-THRIFT = $(top_builddir)/compiler/cpp/thrift
-
-TESTDIR = Tests/Thrift.PublicInterfaces.Compile.Tests
-GENDIR = $(TESTDIR)/gen-netcore
-
THRIFTCODE = \
- Thrift/Thrift.csproj \
- Thrift/ITAsyncProcessor.cs \
- Thrift/ITProcessorFactory.cs \
- Thrift/SingletonTProcessorFactory.cs \
- Thrift/TApplicationException.cs \
- Thrift/TBaseClient.cs \
- Thrift/TException.cs \
- Thrift/TMultiplexedProcessor.cs \
- Thrift/Collections/TCollections.cs \
- Thrift/Collections/THashSet.cs \
- Thrift/Properties/AssemblyInfo.cs \
- Thrift/Protocols/ITProtocolFactory.cs \
- Thrift/Protocols/TAbstractBase.cs \
- Thrift/Protocols/TBase.cs \
- Thrift/Protocols/TBinaryProtocol.cs \
- Thrift/Protocols/TCompactProtocol.cs \
- Thrift/Protocols/TJSONProtocol.cs \
- Thrift/Protocols/TMultiplexedProtocol.cs \
- Thrift/Protocols/TProtocol.cs \
- Thrift/Protocols/TProtocolDecorator.cs \
- Thrift/Protocols/TProtocolException.cs \
- Thrift/Protocols/Entities/TField.cs \
- Thrift/Protocols/Entities/TList.cs \
- Thrift/Protocols/Entities/TMap.cs \
- Thrift/Protocols/Entities/TMessage.cs \
- Thrift/Protocols/Entities/TMessageType.cs \
- Thrift/Protocols/Entities/TSet.cs \
- Thrift/Protocols/Entities/TStruct.cs \
- Thrift/Protocols/Entities/TType.cs \
- Thrift/Protocols/Utilities/TBase64Utils.cs \
- Thrift/Protocols/Utilities/TProtocolUtil.cs \
- Thrift/Server/AsyncBaseServer.cs \
- Thrift/Server/TBaseServer.cs \
- Thrift/Server/TServerEventHandler.cs \
- Thrift/Transports/TClientTransport.cs \
- Thrift/Transports/TServerTransport.cs \
- Thrift/Transports/TTransportException.cs \
- Thrift/Transports/TTransportFactory.cs \
- Thrift/Transports/Client/TBufferedClientTransport.cs \
- Thrift/Transports/Client/TFramedClientTransport.cs \
- Thrift/Transports/Client/THttpClientTransport.cs \
- Thrift/Transports/Client/TMemoryBufferClientTransport.cs \
- Thrift/Transports/Client/TNamedPipeClientTransport.cs \
- Thrift/Transports/Client/TSocketClientTransport.cs \
- Thrift/Transports/Client/TStreamClientTransport.cs \
- Thrift/Transports/Client/TTlsSocketClientTransport.cs \
- Thrift/Transports/Server/THttpServerTransport.cs \
- Thrift/Transports/Server/TNamedPipeServerTransport.cs \
- Thrift/Transports/Server/TServerFramedTransport.cs \
- Thrift/Transports/Server/TServerSocketTransport.cs \
- Thrift/Transports/Server/TTlsServerSocketTransport.cs
+ Thrift/Thrift.csproj
all-local: \
Thrift.dll
Thrift.dll: $(THRIFTCODE)
- $(MKDIR_P) $(GENDIR)
- $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(TESTDIR)/CassandraTest.thrift
- $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/test/ThriftTest.thrift
- $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/contrib/fb303/if/fb303.thrift
- $(DOTNETCORE) --info
- $(DOTNETCORE) restore
+# $(MKDIR_P) $(GENDIR)
+# $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(TESTDIR)/CassandraTest.thrift
+# $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/test/ThriftTest.thrift
+# $(THRIFT) -gen netcore:wcf -r -out $(GENDIR) $(top_srcdir)/contrib/fb303/if/fb303.thrift
$(DOTNETCORE) build
clean-local:
- $(RM) Thrift.dll
--- End diff --
Why aren't we cleaning up Thrift.dll as part of clean?
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
All our tests use self-signed certificates... so validating the chain may result in a certificate error?
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
@jeking3 - updated header file to fix warnings, also reverted Autotools files to check builds
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161373415
--- Diff: lib/netcore/Makefile.am ---
@@ -19,86 +19,25 @@
SUBDIRS = .
-THRIFT = $(top_builddir)/compiler/cpp/thrift
-
-TESTDIR = Tests/Thrift.PublicInterfaces.Compile.Tests
-GENDIR = $(TESTDIR)/gen-netcore
-
THRIFTCODE = \
- Thrift/Thrift.csproj \
--- End diff --
I don't think this is correct, removing all the dependencies here. This means if a source code file changes but the project stays the same, make won't trigger a netcore build - it will think the target is up to date. In the merge I am including both, and adding three missing source code files. Because of this, I am going to have to submit a CI for the merge to make sure it works.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Updated tests, fixed some problems (added framed correct support at server side, updated tests and test known failures, etc.), reverted one line in netcore*.h file (instead added suppression of one warning to automake files).
It seems, that at Ubuntu 17.10, it can build and run all tests for .net core (for server side and client side) for all pre-configured languages with automake (make cross).
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on the issue:
https://github.com/apache/thrift/pull/1449
Will try to quickly find the problem with old .Net tests - maybe version of Tls in implementation of tests and will update pull request if this is possible to do that quickly
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161373681
--- Diff: lib/netcore/Makefile.am ---
@@ -19,86 +19,25 @@
SUBDIRS = .
-THRIFT = $(top_builddir)/compiler/cpp/thrift
-
-TESTDIR = Tests/Thrift.PublicInterfaces.Compile.Tests
-GENDIR = $(TESTDIR)/gen-netcore
-
THRIFTCODE = \
- Thrift/Thrift.csproj \
- Thrift/ITAsyncProcessor.cs \
- Thrift/ITProcessorFactory.cs \
- Thrift/SingletonTProcessorFactory.cs \
- Thrift/TApplicationException.cs \
- Thrift/TBaseClient.cs \
- Thrift/TException.cs \
- Thrift/TMultiplexedProcessor.cs \
- Thrift/Collections/TCollections.cs \
- Thrift/Collections/THashSet.cs \
- Thrift/Properties/AssemblyInfo.cs \
- Thrift/Protocols/ITProtocolFactory.cs \
- Thrift/Protocols/TAbstractBase.cs \
- Thrift/Protocols/TBase.cs \
- Thrift/Protocols/TBinaryProtocol.cs \
- Thrift/Protocols/TCompactProtocol.cs \
- Thrift/Protocols/TJSONProtocol.cs \
- Thrift/Protocols/TMultiplexedProtocol.cs \
- Thrift/Protocols/TProtocol.cs \
- Thrift/Protocols/TProtocolDecorator.cs \
- Thrift/Protocols/TProtocolException.cs \
- Thrift/Protocols/Entities/TField.cs \
- Thrift/Protocols/Entities/TList.cs \
- Thrift/Protocols/Entities/TMap.cs \
- Thrift/Protocols/Entities/TMessage.cs \
- Thrift/Protocols/Entities/TMessageType.cs \
- Thrift/Protocols/Entities/TSet.cs \
- Thrift/Protocols/Entities/TStruct.cs \
- Thrift/Protocols/Entities/TType.cs \
- Thrift/Protocols/Utilities/TBase64Utils.cs \
- Thrift/Protocols/Utilities/TProtocolUtil.cs \
- Thrift/Server/AsyncBaseServer.cs \
- Thrift/Server/TBaseServer.cs \
- Thrift/Server/TServerEventHandler.cs \
- Thrift/Transports/TClientTransport.cs \
- Thrift/Transports/TServerTransport.cs \
- Thrift/Transports/TTransportException.cs \
- Thrift/Transports/TTransportFactory.cs \
- Thrift/Transports/Client/TBufferedClientTransport.cs \
- Thrift/Transports/Client/TFramedClientTransport.cs \
- Thrift/Transports/Client/THttpClientTransport.cs \
- Thrift/Transports/Client/TMemoryBufferClientTransport.cs \
- Thrift/Transports/Client/TNamedPipeClientTransport.cs \
- Thrift/Transports/Client/TSocketClientTransport.cs \
- Thrift/Transports/Client/TStreamClientTransport.cs \
- Thrift/Transports/Client/TTlsSocketClientTransport.cs \
- Thrift/Transports/Server/THttpServerTransport.cs \
- Thrift/Transports/Server/TNamedPipeServerTransport.cs \
- Thrift/Transports/Server/TServerFramedTransport.cs \
- Thrift/Transports/Server/TServerSocketTransport.cs \
- Thrift/Transports/Server/TTlsServerSocketTransport.cs
+ Thrift/Thrift.csproj
all-local: \
--- End diff --
Perhaps this part of the build would be better off if it always ran the $(DOTNETCORE) build command instead od using dependencies handled out here... I'll do that.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
I saw a dart signing key issue about 30 minutes ago, so yes it could be that.
---
[GitHub] thrift pull request #1449: THRIFT-4434. Update .NET Core components, add tes...
Posted by vgotra <gi...@git.apache.org>.
Github user vgotra commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1449#discussion_r161375270
--- Diff: test/tests.json ---
@@ -1,689 +1,686 @@
-[
--- End diff --
As usually - some settings of different editors. Maybe you can provide some settings for VSCode in future - this should help to use this standard for Thrift. Can be useful for everyone and for you too.
---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1449
The log is there:
```
===============================================================================
*** Following 1 failures were unexpected ***:
If it is introduced by you, please fix it before submitting the code.
===============================================================================
server-client: protocol: transport: result:
csharp-tls binary buffered-ip-ssl failure(3)
===============================================================================
```
There is a security feature test in test/features called tls and it verifies that the server accepts a TLS connection. It verifies that a TLSv1.0, TLSv1.1, or TLSv1.2 client can connect successfully. If this is failing then something changed in the code that restricted the TLS to one level.
We currently use SSLv23 handshake and disable the use of SSLv2 and SSLv3, so that the client can connect at TLSv1.0 or higher.
There's a story in the backlog to move all languages by default up to TLSv1.2 to improve security.
https://issues.apache.org/jira/browse/THRIFT-3165
Implementing that across the board would be verified by modifying tls.sh in test/features so that TLSv1.0 and TLSv1.1 was NOT expected to allow a connection by default.
What I have found is that if you specify a handshake at TLSv10, then ONLY TLSv1.0 is allowed to connect. It doesn't allow TLSv1.1 or TLSv1.2 to connect. If you specify a SSLv23 handshake, but disable SSLv2 and SSLv3 negotiation, then any client can connect and they can only negotiate at TLSv1.0 or later, which is the current behavior we have.
---