[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 ...
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[33m2 Warning(s) [39;49m0 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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 ...
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 ...
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 ...
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 ...
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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 ...
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 ...
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 binarybuffered-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. ---
[GitHub] thrift issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 ...
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 ...
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 ...
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 ...
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 ...
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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 ...
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 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 ...
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 ...
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 ...
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 ...
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 ...
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 ...
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 ...
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 ...
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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 issue #1449: THRIFT-4434. Update .NET Core components, add tests for ...
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 ...
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 ...
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 ...
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 ...
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. ---
