thrift: module integration, samples, and tests #54013

cfriedt · 2023-01-23T12:57:55Z

Note: Compliance failures are false positives

TSC vote to fork thrift
fork https://github.com/apache/thrift into zephyrproject/thrift
base: add thrift dependencies docker-image#133
update sample docs to include e.g. using Python on the host
TSC vote to allow merging PR for v3.3 after feature freeze

zephyrbot · 2023-01-23T12:58:53Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
thrift	N/A	zephyrproject-rtos/thrift@`1002364` (zephyr)	N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

west.yml

cfriedt · 2023-01-24T04:39:00Z

All samples & tests passing with the addition of the following in ../modules/lib/thrift/zephyr/module.yml:

name: thrift
build:
  cmake-ext: True
  kconfig-ext: True

#
# Run Test
#
twister --retry-failed 3 -G -T tests/lib/thrift/
ZEPHYR_BASE unset, using "/home/cfriedt/zephyrproject/zephyr"
Renaming output directory to /home/cfriedt/zephyrproject/zephyr/twister-out.2
INFO    - Using Ninja..
INFO    - Zephyr version: zephyr-v3.2.0-3787-gb22347e7b20e
INFO    - Using 'zephyr' toolchain.
INFO    - Selecting default platforms per test case
INFO    - Building initial testsuite list...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/testplan.json
INFO    - JOBS: 32
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - Total complete:   36/  36  100%  skipped:    0, failed:    0
INFO    - 7 test scenarios (36 test instances) selected, 0 configurations skipped (0 by static filter, 0 at runtime).
INFO    - 36 of 36 test configurations passed (100.00%), 0 failed, 0 skipped with 0 warnings in 235.66 seconds
INFO    - In total 538 test cases were executed, 0 skipped on 7 out of total 535 platforms (1.31%)
INFO    - 36 test configurations executed on platforms, 0 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

#
# Build samples
#
twister --build-only -G -T samples/modules/thrift/
INFO    - Using Ninja..
INFO    - Zephyr version: zephyr-v3.2.0-3787-gb22347e7b20e
INFO    - Using 'zephyr' toolchain.
INFO    - Selecting default platforms per test case
INFO    - Building initial testsuite list...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/testplan.json
INFO    - JOBS: 64
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - Total complete:    8/   8  100%  skipped:    0, failed:    0
INFO    - 8 test scenarios (8 test instances) selected, 0 configurations skipped (0 by static filter, 0 at runtime).
INFO    - 8 of 8 test configurations passed (100.00%), 0 failed, 0 skipped with 0 warnings in 29.26 seconds
INFO    - 0 test configurations executed on platforms, 8 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

nordicjm

A very quick cursory glance

modules/thrift/include/muzic/uzlib.h

modules/thrift/misc/hello.thrift

modules/thrift/src/thrift/config.h

modules/thrift/src/thrift/server/TFDServer.cpp

samples/modules/thrift/hello_common/native-cert.pem

samples/modules/thrift/hello_server/CMakeLists.txt

samples/modules/thrift/hello_server/Kconfig

samples/modules/thrift/hello_server/Makefile

tests/lib/thrift/ThriftTest/src/client.cpp

doc/develop/getting_started/index.rst

doc/develop/getting_started/installation_linux.rst

modules/thrift/Kconfig

samples/modules/thrift/hello_client/prj.conf

doc/develop/getting_started/index.rst

cmake/util/thrift.cmake

cfriedt · 2023-02-01T22:02:19Z

Note: TSC vote is not about code review, it's about allowing the exception for being past feature-freeze

Ready again
This module has been more or less ready to merge since October but was delayed on reviews for fixes to other subsystems
Also again, just to clarify, some Thrift sources are re-implemented in Zephyr as a workaround for Zephyr's lack of support for std::thread, std::mutex, etc. Those are on the C++ Roadmap
Once Zephyr has support for std::thread, std::mutex, etc, we can drop the workarounds to use unmodified upstream sources.
Yes, upstream Thrift header files end in .h and not in .hpp. Yes, the copies of those files with slight workarounds need to keep the .h extension. Yes, that has already been discussed here. No, they will not be renamed, because it is not part of Zephyr's C++ coding style to force external projects to name C++ header files with the extension .hpp.
Mainly, the only sources that will persist in the modules/ directory are related to SSL / TLS and require some finesse due to Zephyr's TLS socket implementation

modules/thrift/src/thrift/protocol/TBinaryProtocol.h

tests/lib/thrift/ThriftTest/src/context.hpp

samples/modules/thrift/hello/client/src/main.cpp

samples/modules/thrift/hello/doc/index.rst

samples/modules/thrift/hello/server/src/main.cpp

modules/thrift/src/thrift/protocol/TBinaryProtocol.h

modules/thrift/src/thrift/server/TConnectedClient.h

nordicjm

Final change and +1 from me

tests/lib/thrift/ThriftTest/src/server.hpp

stephanosio · 2023-02-09T01:21:55Z

#54626, which includes thrift-compiler has been merged.

Rebasing

stephanosio · 2023-02-09T01:23:12Z

@cfriedt FYI, this PR is currently pointing to add-zephyr-module-dir branch in the module.

cfriedt · 2023-02-09T02:17:14Z

@cfriedt FYI, this PR is currently pointing to add-zephyr-module-dir branch in the module.

Let me update that. Thanks @stephanosio

Add a manifest entry pointing to Zephyr's fork of `apache/thrift`. Signed-off-by: Chris Friedt <cfriedt@meta.com>

Add an entry for Apache Thrift and myself as maintainer. Signed-off-by: Chris Friedt <cfriedt@meta.com>

Add glue code for the thrift module. This includes: * workarounds for Zephyr's missing C++ facilities * thrift config.h This code was merged from the following repository at the commit specified below, with minor formatting and coding-style modifications. https://github.com/zephyrproject-rtos/gsoc-2022-thrift e12e014d295918cc5ba0b4c507d1bf595a2f539a Signed-off-by: Chris Friedt <cfriedt@meta.com>

These tests include: * ThriftTest - an upstream exercies for all Thrift facilities This code was merged from the following repository at the commit specified below, with minor formatting and coding-style modifications. https://github.com/zephyrproject-rtos/gsoc-2022-thrift e12e014d295918cc5ba0b4c507d1bf595a2f539a Signed-off-by: Chris Friedt <cfriedt@meta.com>

Here we add a client and server samples for the basic "hello" service. These samples are designed to be run by either Zephyr or the host machine, interchangeably. Additionally, there is a python version of the client to demonstrate Thrift's cross-language capabilities. This code was merged from the following repository at the commit specified below, with minor formatting and coding-style modifications. https://github.com/zephyrproject-rtos/gsoc-2022-thrift e12e014d295918cc5ba0b4c507d1bf595a2f539a Signed-off-by: Chris Friedt <cfriedt@meta.com>

cfriedt · 2023-02-09T04:12:48Z

update west.yml to point to zephyr branch of https://github.com/zephyrproject-rtos/thrift.git

cfriedt · 2023-02-09T04:33:39Z

@nordicjm - please re-ack when you have a moment

zephyrbot added manifest manifest-thrift DNM This PR should not be merged (Do Not Merge) labels Jan 23, 2023

cfriedt force-pushed the merge-thrift-into-zephyr branch from 0aa47a7 to 1d9d187 Compare January 23, 2023 14:38

gmarull reviewed Jan 23, 2023

View reviewed changes

west.yml Outdated Show resolved Hide resolved

cfriedt mentioned this pull request Jan 23, 2023

Introduce Pigweed Module #53760

Closed

cfriedt force-pushed the merge-thrift-into-zephyr branch from 1d9d187 to b22347e Compare January 24, 2023 04:11

cfriedt added the TSC Topics that need TSC discussion label Jan 24, 2023

cfriedt force-pushed the merge-thrift-into-zephyr branch from b22347e to 0cda6ea Compare January 24, 2023 04:40

cfriedt marked this pull request as ready for review January 24, 2023 04:42

cfriedt requested review from nashif and tejlmand as code owners January 24, 2023 04:42

zephyrbot added area: Documentation area: Build System labels Jan 24, 2023

zephyrbot requested review from carlescufi, jeremybettis and nordicjm January 24, 2023 04:43

nordicjm reviewed Jan 24, 2023

View reviewed changes

cfriedt commented Jan 24, 2023

View reviewed changes

cfriedt mentioned this pull request Jan 24, 2023

Add Apache Thrift Module (from GSoC 2022 Project) #51620

Closed

cfriedt force-pushed the merge-thrift-into-zephyr branch from 0cda6ea to 07cc614 Compare January 25, 2023 03:58

cfriedt requested review from mbolivar-nordic and galak as code owners January 25, 2023 03:58

cfriedt force-pushed the merge-thrift-into-zephyr branch from 07cc614 to 19e1d68 Compare January 25, 2023 04:20

cfriedt requested review from gmarull and nordicjm January 25, 2023 12:30

gmarull previously requested changes Jan 25, 2023

View reviewed changes

doc/develop/getting_started/index.rst Outdated Show resolved Hide resolved

nashif reviewed Jan 25, 2023

View reviewed changes

cmake/util/thrift.cmake Outdated Show resolved Hide resolved

cfriedt force-pushed the merge-thrift-into-zephyr branch from a7e9243 to dd1ea20 Compare February 1, 2023 21:57

cfriedt requested a review from nordicjm February 1, 2023 22:02

cfriedt force-pushed the merge-thrift-into-zephyr branch from dd1ea20 to f536d87 Compare February 2, 2023 01:01

nordicjm reviewed Feb 2, 2023

View reviewed changes

cfriedt force-pushed the merge-thrift-into-zephyr branch from f536d87 to acc87a4 Compare February 2, 2023 12:53

nordicjm requested changes Feb 2, 2023

View reviewed changes

tests/lib/thrift/ThriftTest/src/server.hpp Outdated Show resolved Hide resolved

cfriedt force-pushed the merge-thrift-into-zephyr branch from acc87a4 to 84121a6 Compare February 3, 2023 12:40

nordicjm previously approved these changes Feb 3, 2023

View reviewed changes

cfriedt added this to the v3.3.0 milestone Feb 7, 2023

stephanosio mentioned this pull request Feb 9, 2023

ci: Switch to CI image 0.24.10 #54626

Merged

stephanosio force-pushed the merge-thrift-into-zephyr branch from 84121a6 to 5bbbe04 Compare February 9, 2023 01:22

cfriedt added 5 commits February 8, 2023 22:10

manifest: add entry for zephyrproject-rtos/thrift

bf81498

Add a manifest entry pointing to Zephyr's fork of `apache/thrift`. Signed-off-by: Chris Friedt <cfriedt@meta.com>

MAINTAINERS: add thrift west project

276f632

Add an entry for Apache Thrift and myself as maintainer. Signed-off-by: Chris Friedt <cfriedt@meta.com>

cfriedt dismissed nordicjm’s stale review via 64c102b February 9, 2023 04:11

cfriedt force-pushed the merge-thrift-into-zephyr branch from 5bbbe04 to 64c102b Compare February 9, 2023 04:11

stephanosio removed the DNM This PR should not be merged (Do Not Merge) label Feb 9, 2023

stephanosio approved these changes Feb 9, 2023

View reviewed changes

nordicjm approved these changes Feb 9, 2023

View reviewed changes

stephanosio merged commit 3ea28b2 into zephyrproject-rtos:main Feb 9, 2023

cfriedt deleted the merge-thrift-into-zephyr branch February 9, 2023 23:46

pdgendt mentioned this pull request Mar 3, 2023

manifest: update LVGL to v8.3.3 #53974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

thrift: module integration, samples, and tests #54013

thrift: module integration, samples, and tests #54013

cfriedt commented Jan 23, 2023 •

edited

Loading

zephyrbot commented Jan 23, 2023 •

edited

Loading

cfriedt commented Jan 24, 2023 •

edited

Loading

nordicjm left a comment

cfriedt commented Feb 1, 2023 •

edited

Loading

nordicjm left a comment

stephanosio commented Feb 9, 2023

stephanosio commented Feb 9, 2023

cfriedt commented Feb 9, 2023

cfriedt commented Feb 9, 2023

cfriedt commented Feb 9, 2023

thrift: module integration, samples, and tests #54013

thrift: module integration, samples, and tests #54013

Conversation

cfriedt commented Jan 23, 2023 • edited Loading

zephyrbot commented Jan 23, 2023 • edited Loading

cfriedt commented Jan 24, 2023 • edited Loading

nordicjm left a comment

Choose a reason for hiding this comment

cfriedt commented Feb 1, 2023 • edited Loading

nordicjm left a comment

Choose a reason for hiding this comment

stephanosio commented Feb 9, 2023

stephanosio commented Feb 9, 2023

cfriedt commented Feb 9, 2023

cfriedt commented Feb 9, 2023

cfriedt commented Feb 9, 2023

cfriedt commented Jan 23, 2023 •

edited

Loading

zephyrbot commented Jan 23, 2023 •

edited

Loading

cfriedt commented Jan 24, 2023 •

edited

Loading

cfriedt commented Feb 1, 2023 •

edited

Loading