upstream: remove dependency on cfriedt/thrift and depend on apache/thrift directly #158
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Keeping this in draft, as it is blocked by |
cfriedt
changed the title
cfriedt
changed the title
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
5 times, most recently
from
490103b
to
7884727
Compare
|
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
5 times, most recently
from
6d5adc9
to
6d46498
Compare
|
Need to update git submodule commit after apache/thrift#2745 is merged, as it appears that TSocketServer.cpp (upstream) leaks file descriptors. For more details, see cc @stephanosio @SdtElectronics Young, you may have encountered this in your SSL work. |
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
from
4d7f766
to
75c32d9
Compare
I was wrong - it seems that custom deleters are not working in Zephyr's C++ runtime. If that were the case, then https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/transport/TServerSocket.cpp#L91 Kind of an interesting C++ quirk @stephanosio. Might be good to add a test for that. I'm sure it's a lot more involved under the hood, of course ;-) |
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
3 times, most recently
from
5e6f3a5
to
e8666fd
Compare
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
from
e8666fd
to
ae52a98
Compare
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
2 times, most recently
from
9435a56
to
184bcb3
Compare
The Thrift sources put includes directly in the source directory. We'll need to override some upstream headers to workaround Zephyr's lack of support for `std::thread` and `std::mutex`, so our local include paths should come first. Signed-off-by: Chris Friedt <[email protected]>
Zephyr does not yet support `std::thread` or `std::mutex`. Fixes #28 Signed-off-by: Chris Friedt <[email protected]>
Upstream does not have a separate include directory. Signed-off-by: Christopher Friedt <[email protected]>
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
from
acc6e57
to
81a053e
Compare
This file is not required. Signed-off-by: Christopher Friedt <[email protected]>
Just to keep things slightly separate. Signed-off-by: Christopher Friedt <[email protected]>
With this, along with upstream Zephyr POSIX fixes, we can run perfectly against upstream Thrift. Signed-off-by: Christopher Friedt <[email protected]>
This is no longer needed after upstream Zephyr POSIX fixes. Signed-off-by: Christopher Friedt <[email protected]>
Previously, there was an issue in the Zephyr SDK that prevented some targets from building properly. However, we do want to ensure that we have coverage for at least arm, aarch64, riscv, riscv64, x86, and x86_64, so re-enable those targets here. Signed-off-by: Christopher Friedt <[email protected]>
The system workqueue thread had experienced a stack overflow for BOARD=`qemu_riscv64_smp`. Bump it for now for all platforms. Signed-off-by: Chris Friedt <[email protected]>
This was fixed with release 0.15.2 of the SDK but somehow `qemu_x86` was not included. Once a newer version of the SDK is released with a fix, we can re-enable `qemu_x86` as an integration platform in Thrift. The exact details are highlighted in Zephyr SDK Issue 603. Signed-off-by: Chris Friedt <[email protected]>
Since the file descriptor leak was plugged in the previous commit, we can reduce memory requirements for this test. Also, we can keep the stack sentinal as default. Lastly, use `qemu_riscv64` instead of `qemu_riscv64_smp` for now, as the latter seems to be experiencing weird issues. Signed-off-by: Chris Friedt <[email protected]>
Upstream has an issue in that `TConnectedClient` has a non- virtual destructor. This results in a warning from GCC when compiling `TServerFramework.cpp`. Of course, in CI, all of Zephyr's warnings are promoted to errors, which will break the build. Tracking the issue upstream at the link below. https://issues.apache.org/jira/browse/THRIFT-5678 We'll keep this workaround in tree until the fix is merged upstream. Fixes #160 Signed-off-by: Christopher Friedt <[email protected]>
It would seem that Zephyr's C++ runtime was not properly executing custom deleters for `std::shared_ptr`. Originally, it seemed as though `TServerSocket.cpp` was leaking file descriptors, but there is a custom deleter used that is never called. Details at the URL below. https://issues.apache.org/jira/browse/THRIFT-5677 Once this is fixed in Zephyr's C++ runtime, this workaround should be reverted. Fixes #161 Signed-off-by: Christopher Friedt <[email protected]>
Tested all configurations against programes compiled for the host which are built with `.../hello_client/Makefile`. Signed-off-by: Christopher Friedt <[email protected]>
Tested all configurations against programes compiled for the host which are built with `.../hello_server/Makefile`. Signed-off-by: Christopher Friedt <[email protected]>
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
from
81a053e
to
4421837
Compare
Like Zephyr's CI, there are occasionally statistical outliers, race conditions, random timing changes, particularly on SMP platforms that can make a test fail. Retry a test failure up to 3 times before failing in CI. Signed-off-by: Chris Friedt <[email protected]>
cfriedt
force-pushed
the
switch-to-apache-thrift-master
branch
2 times, most recently
from
96f692f
to
9ce1006
Compare
|
jeremybettis
approved these changes
Jan 23, 2023
|
IIRC Stephanos is off for the next couple of days, so opening this up to other reviewers. |
SdtElectronics
approved these changes
Jan 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changeset actually has a fairly big impact - it removes the dependency on cfriedt/thrift (a fork) and enables the module to use apache/thrift directly 😀
With that, we will also be able to pin to a specific release of apache/thrift, likely the upcoming v0.18.0 release.
The only remaining issue now is to fix
std::threadandstd::mutexin Zephyr. As a temporary workaround, we've copied in the affected apache/thrift sources and headers and made some slight modifications. Eventually, when Zephyr supportsstd::threadandstd::mutex, we can remove those workarounds.See individual commit messages for details.