Skip to content

Commit

Permalink
[thrift] add a patch to revert THRIFT-3650 (#2688)
Browse files Browse the repository at this point in the history
Revert breaking change in thrift 0.11.0;
saithrift implementation relies on the bug in union serialization
(details: https://jira.apache.org/jira/browse/THRIFT-3650)

Add this revert patch untill saithrift is fixed according to
correct thrift behaviour

Signed-off-by: Stepan Blyschak <[email protected]>
  • Loading branch information
stepanblyschak authored and lguohan committed Mar 21, 2019
1 parent 698b248 commit 0a0f5b8
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 0 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ src/telemetry/debian/*
!src/telemetry/debian/rules
!src/telemetry/debian/telemetry.init.d
src/thrift/*
!src/thrift/patch/
!src/thrift/Makefile

# Autogenerated Dockerfiles
Expand Down
5 changes: 5 additions & 0 deletions src/thrift/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ $(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% :

dpkg-source -x thrift_$(THRIFT_VERSION_FULL).dsc
pushd thrift-$(THRIFT_VERSION)

# Revert breaking change in thrift 0.11.0
# saithrift implementation relies on the bug in union serialization
# (https://jira.apache.org/jira/browse/THRIFT-3650)
patch -p1 < ../patch/0001-Revert-THRIFT-3650-incorrect-union-serialization.patch
CXXFLAGS="-DFORCE_BOOST_SMART_PTR" DEB_BUILD_OPTIONS=nocheck dpkg-buildpackage -d -rfakeroot -b -us -uc -j$(SONIC_CONFIG_MAKE_JOBS)
popd

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
From f44f148bb9cff81f320e7094e7956d98fc5ac423 Mon Sep 17 00:00:00 2001
From: Stepan Blyschak <[email protected]>
Date: Wed, 20 Mar 2019 16:32:56 +0200
Subject: [PATCH] Revert "THRIFT-3650 incorrect union serialization"

This reverts commit b72bb94a8212edc83864edc435896fdcda6e796c.

Signed-off-by: Stepan Blyschak <[email protected]>
---
compiler/cpp/src/thrift/parse/t_struct.h | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/compiler/cpp/src/thrift/parse/t_struct.h b/compiler/cpp/src/thrift/parse/t_struct.h
index 4102da7..880e79b 100644
--- a/compiler/cpp/src/thrift/parse/t_struct.h
+++ b/compiler/cpp/src/thrift/parse/t_struct.h
@@ -66,16 +66,12 @@ public:
void validate_union_member(t_field* field) {
if (is_union_ && (!name_.empty())) {

- // 1) unions can't have required fields
- // 2) union members are implicitly optional, otherwise bugs like THRIFT-3650 wait to happen
- if (field->get_req() != t_field::T_OPTIONAL) {
- // no warning on default requiredness, but do warn on anything else that is explicitly asked for
- if(field->get_req() != t_field::T_OPT_IN_REQ_OUT) {
- pwarning(1,
- "Union %s field %s: union members must be optional, ignoring specified requiredness.\n",
- name_.c_str(),
- field->get_name().c_str());
- }
+ // unions can't have required fields
+ if (field->get_req() == t_field::T_REQUIRED) {
+ pwarning(1,
+ "Required field %s of union %s set to optional.\n",
+ field->get_name().c_str(),
+ name_.c_str());
field->set_req(t_field::T_OPTIONAL);
}

--
1.9.1

0 comments on commit 0a0f5b8

Please sign in to comment.