From a30ec6321b9d468f92942045035279927b0e5b40 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Tue, 22 Oct 2024 14:53:12 +0200 Subject: [PATCH 01/11] ProtocolBench Build Rules --- ThriftLibrary.cmake | 90 ++++++++++++++++++++++------ thrift/lib/cpp2/test/CMakeLists.txt | 91 +++++++++++++++++++++++++++++ thrift/lib/cpp2/test/Structs.h | 2 + thrift/lib/thrift/CMakeLists.txt | 40 +++++++++++++ 4 files changed, 205 insertions(+), 18 deletions(-) create mode 100644 thrift/lib/cpp2/test/CMakeLists.txt diff --git a/ThriftLibrary.cmake b/ThriftLibrary.cmake index 0e0a2dbb56c..70d01107598 100644 --- a/ThriftLibrary.cmake +++ b/ThriftLibrary.cmake @@ -165,10 +165,12 @@ macro(bypass_source_check sources) endmacro() # -# thrift_generate +# thrift_generate_named +# Supports libary names that differ from the file name (two handle two libraries with the same filename on disk (in different folders)) # This is used to codegen thrift files using the thrift compiler # Params: -# @file_name - The name of tge thrift file +# @file_name - The name of the library that is generated (without the language suffix) +# @real_file_name - The name of the thrift file on disk # @services - A list of services that are declared in the thrift file # @language - The generator to use (cpp, cpp2 or py3) # @options - Extra options to pass to the generator @@ -186,9 +188,9 @@ endmacro() # bypass_source_check(${file_language-SOURCES}) # This will prevent cmake from complaining about missing source files # - -macro(thrift_generate +macro(thrift_generate_named file_name + real_file_name services language options @@ -208,33 +210,39 @@ macro(thrift_generate endforeach() set("${file_name}-${language}-HEADERS" - ${output_path}/gen-${language}/${file_name}_constants.h - ${output_path}/gen-${language}/${file_name}_data.h - ${output_path}/gen-${language}/${file_name}_metadata.h - ${output_path}/gen-${language}/${file_name}_types.h - ${output_path}/gen-${language}/${file_name}_types.tcc + ${output_path}/gen-${language}/${real_file_name}_constants.h + ${output_path}/gen-${language}/${real_file_name}_data.h + ${output_path}/gen-${language}/${real_file_name}_metadata.h + ${output_path}/gen-${language}/${real_file_name}_types.h + ${output_path}/gen-${language}/${real_file_name}_types.tcc ) set("${file_name}-${language}-SOURCES" - ${output_path}/gen-${language}/${file_name}_constants.cpp - ${output_path}/gen-${language}/${file_name}_data.cpp - ${output_path}/gen-${language}/${file_name}_types.cpp + ${output_path}/gen-${language}/${real_file_name}_constants.cpp + ${output_path}/gen-${language}/${real_file_name}_data.cpp + ${output_path}/gen-${language}/${real_file_name}_types.cpp ) + if("${options}" MATCHES "layouts") + set("${file_name}-${language}-SOURCES" + ${${file_name}-${language}-SOURCES} + ${output_path}/gen-${language}/${real_file_name}_layouts.cpp + ) + endif() if(NOT "${options}" MATCHES "no_metadata") set("${file_name}-${language}-SOURCES" ${${file_name}-${language}-SOURCES} - ${output_path}/gen-${language}/${file_name}_metadata.cpp + ${output_path}/gen-${language}/${real_file_name}_metadata.cpp ) endif() foreach(service ${services}) set("${file_name}-${language}-HEADERS" - ${${file_name}-${language}-HEADERS} + ${${real_file_name}-${language}-HEADERS} ${output_path}/gen-${language}/${service}.h ${output_path}/gen-${language}/${service}.tcc ${output_path}/gen-${language}/${service}AsyncClient.h ${output_path}/gen-${language}/${service}_custom_protocol.h ) set("${file_name}-${language}-SOURCES" - ${${file_name}-${language}-SOURCES} + ${${real_file_name}-${language}-SOURCES} ${output_path}/gen-${language}/${service}.cpp ${output_path}/gen-${language}/${service}AsyncClient.cpp ) @@ -252,7 +260,7 @@ macro(thrift_generate set(gen_language "mstch_cpp2") elseif("${language}" STREQUAL "py3") set(gen_language "mstch_py3") - file(WRITE "${output_path}/gen-${language}/${file_name}/__init__.py") + file(WRITE "${output_path}/gen-${language}/${real_file_name}/__init__.py") endif() add_custom_command( OUTPUT ${${file_name}-${language}-HEADERS} @@ -261,10 +269,10 @@ macro(thrift_generate --gen "${gen_language}:${options}${include_prefix_text}" -o ${output_path} ${thrift_include_directories} - "${file_path}/${file_name}.thrift" + "${file_path}/${real_file_name}.thrift" DEPENDS ${THRIFT1} - "${file_path}/${file_name}.thrift" + "${file_path}/${real_file_name}.thrift" COMMENT "Generating ${file_name} files. Output: ${output_path}" ) add_custom_target( @@ -281,3 +289,49 @@ macro(thrift_generate DESTINATION include/${include_prefix} FILES_MATCHING PATTERN "*.tcc") endmacro() + + +# +# thrift_generate +# This is used to codegen thrift files using the thrift compiler +# Params: +# @file_name - The name of tge thrift file +# @services - A list of services that are declared in the thrift file +# @language - The generator to use (cpp, cpp2 or py3) +# @options - Extra options to pass to the generator +# @output_path - The directory where the thrift file lives +# +# Output: +# file-language-target - A custom target to add a dependenct +# ${file-language-HEADERS} - The generated Header Files. +# ${file-language-SOURCES} - The generated Source Files. +# +# Notes: +# If any of the fields is empty, it is still required to provide an empty string +# +# When using file_language-SOURCES it should always call: +# bypass_source_check(${file_language-SOURCES}) +# This will prevent cmake from complaining about missing source files +# +macro(thrift_generate + file_name + services + language + options + file_path + output_path + include_prefix +) + + thrift_generate_named( + "${file_name}" + "${file_name}" + "${services}" + "${language}" + "${options}" + "${file_path}" + "${output_path}" + "${include_prefix}" + "${ARGN}" + ) +endmacro() \ No newline at end of file diff --git a/thrift/lib/cpp2/test/CMakeLists.txt b/thrift/lib/cpp2/test/CMakeLists.txt new file mode 100644 index 00000000000..b92393030ed --- /dev/null +++ b/thrift/lib/cpp2/test/CMakeLists.txt @@ -0,0 +1,91 @@ +# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# This CMakeLists.txt takes care of building a selection of benchmarks and tests in this folder. +# Currently, it only builds ProtocolBench with its dependencies + +# obtain paths to other generated thrift files from the global property space +get_property(protocol-cpp2-SOURCES GLOBAL PROPERTY + protocol-cpp2-SOURCES) +get_property(protocol_detail-cpp2-SOURCES GLOBAL PROPERTY + protocol_detail-cpp2-SOURCES) +get_property(frozen-cpp2-SOURCES GLOBAL PROPERTY + frozen-cpp2-SOURCES) +get_property(field_mask-cpp2-SOURCES GLOBAL PROPERTY + field_mask-cpp2-SOURCES) + +# generate thrift files for this benchmark +thrift_generate( + "ProtocolBenchData" #file_name + "" #services + "cpp2" #language + "frozen2,layouts" #options + "${CMAKE_CURRENT_SOURCE_DIR}" #file_path + "${CMAKE_CURRENT_BINARY_DIR}" #output_path + "thrift/lib/cpp2/test" #include_prefix + THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} +) + +set(GENERATED_THRIFT_SOURCES ${ProtocolBenchData-cpp2-SOURCES} + ${protocol-cpp2-SOURCES} + ${protocol_detail-cpp2-SOURCES} + ${field_mask-cpp2-SOURCES} + ${frozen-cpp2-SOURCES}) +set(SOURCE_FILES ProtocolBench.cpp) +set(ADDITIONAL_SOURCE_FILES ../../thrift/detail/protocol.cpp ../../thrift/TypeToMaskAdapter.cpp) + +# generate conformance thrift files +foreach(conf_name "protocol" "type" "any" "serialization" "patch_data" "conformance") + thrift_generate_named( + "${conf_name}conformance" #file_name + "${conf_name}" + "" #services + "cpp2" #language + "frozen2" #options + "${CMAKE_CURRENT_SOURCE_DIR}/../../../conformance/if" #file_path + "${CMAKE_CURRENT_BINARY_DIR}/../../../conformance/if" #output_path + "thrift/conformance/if" #include_prefix + THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} + ) + set(GENERATED_THRIFT_SOURCES + ${GENERATED_THRIFT_SOURCES} + ${${conf_name}conformance-cpp2-SOURCES}) +endforeach() + +foreach(GENERATED ${GENERATED_THRIFT_SOURCES}) + set_source_files_properties(${GENERATED} PROPERTIES GENERATED TRUE) +endforeach() + +# build the executable +add_executable(ProtocolBench + ${SOURCE_FILES} + ${ADDITIONAL_SOURCE_FILES} + ${GENERATED_THRIFT_SOURCES} +) +target_link_libraries(ProtocolBench + thriftcpp2 + Folly::folly + Folly::follybenchmark + xxhash +) + +install( + TARGETS ProtocolBench + EXPORT fbthrift-exports + RUNTIME DESTINATION ${BIN_INSTALL_DIR} + LIBRARY DESTINATION ${LIB_INSTALL_DIR} + ARCHIVE DESTINATION ${LIB_INSTALL_DIR} +) + diff --git a/thrift/lib/cpp2/test/Structs.h b/thrift/lib/cpp2/test/Structs.h index 65ae697ae8d..8312eac6fb2 100644 --- a/thrift/lib/cpp2/test/Structs.h +++ b/thrift/lib/cpp2/test/Structs.h @@ -36,5 +36,7 @@ void populateMap(F&& getter) { } } +#if defined __has_include && __has_include() #include +#endif #include diff --git a/thrift/lib/thrift/CMakeLists.txt b/thrift/lib/thrift/CMakeLists.txt index 0e2ba2bc4ed..bb797f1eaae 100644 --- a/thrift/lib/thrift/CMakeLists.txt +++ b/thrift/lib/thrift/CMakeLists.txt @@ -133,6 +133,40 @@ thrift_generate( THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} ) +thrift_generate( + "protocol" #file_name + "" #services + "cpp2" #language + "" #options + "${CMAKE_CURRENT_SOURCE_DIR}" #file_path + "${CMAKE_CURRENT_BINARY_DIR}" #output_path + "thrift/lib/thrift" #include_prefix + THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} +) + +thrift_generate( + "protocol_detail" #file_name + "" #services + "cpp2" #language + "" #options + "${CMAKE_CURRENT_SOURCE_DIR}" #file_path + "${CMAKE_CURRENT_BINARY_DIR}" #output_path + "thrift/lib/thrift" #include_prefix + THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} +) + +thrift_generate( + "field_mask" #file_name + "" #services + "cpp2" #language + "no_metadata" #options + "${CMAKE_CURRENT_SOURCE_DIR}" #file_path + "${CMAKE_CURRENT_BINARY_DIR}" #output_path + "thrift/lib/thrift" #include_prefix + THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} +) + + set_property(GLOBAL PROPERTY any_rep-cpp2-SOURCES ${any_rep-cpp2-SOURCES}) set_property(GLOBAL PROPERTY RpcMetadata-cpp2-SOURCES ${RpcMetadata-cpp2-SOURCES}) @@ -147,3 +181,9 @@ set_property(GLOBAL PROPERTY type_rep-cpp2-SOURCES ${type_rep-cpp2-SOURCES}) set_property(GLOBAL PROPERTY type-cpp2-SOURCES ${type-cpp2-SOURCES}) set_property(GLOBAL PROPERTY serverdbginfo-cpp2-SOURCES ${serverdbginfo-cpp2-SOURCES}) +set_property(GLOBAL PROPERTY protocol-cpp2-SOURCES + ${protocol-cpp2-SOURCES}) +set_property(GLOBAL PROPERTY protocol_detail-cpp2-SOURCES + ${protocol_detail-cpp2-SOURCES}) +set_property(GLOBAL PROPERTY field_mask-cpp2-SOURCES + ${field_mask-cpp2-SOURCES}) From e56e374bcaecc62865a11d9bd3755877adb5581b Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Mon, 4 Nov 2024 14:41:43 +0100 Subject: [PATCH 02/11] Added CMake Rules for Protocol Tests and fixed test cases --- thrift/lib/cpp2/CMakeLists.txt | 4 ++ .../cpp2/protocol/test/BinaryProtocolTest.cpp | 4 +- thrift/lib/cpp2/protocol/test/CMakeLists.txt | 48 +++++++++++++++++++ .../protocol/test/CompactProtocolTest.cpp | 5 +- .../protocol/test/ContainerSkippingTest.cpp | 1 + 5 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 thrift/lib/cpp2/protocol/test/CMakeLists.txt diff --git a/thrift/lib/cpp2/CMakeLists.txt b/thrift/lib/cpp2/CMakeLists.txt index c1c18cf989f..7eab6f3f274 100644 --- a/thrift/lib/cpp2/CMakeLists.txt +++ b/thrift/lib/cpp2/CMakeLists.txt @@ -15,6 +15,10 @@ # Set the cpp2 directory set(LIB_CPP2_HOME ${CMAKE_CURRENT_SOURCE_DIR}) +if(enable_tests) + add_subdirectory(protocol/test) +endif(enable_tests) +add_subdirectory(test) ####### # CMAKE variables only have local/subdirectory scope diff --git a/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp b/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp index 17e06287fe0..e0bed9f4764 100644 --- a/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp +++ b/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp @@ -56,7 +56,9 @@ TEST_F(BinaryProtocolTest, writeInvalidBool) { auto s = std::string(); q.appendToString(s); // Die on success. - CHECK(s != std::string(1, '\0')) << "invalid bool value"; + if(s != std::string(1, '\0')) { + exit(1); + } }, "invalid bool value"); } diff --git a/thrift/lib/cpp2/protocol/test/CMakeLists.txt b/thrift/lib/cpp2/protocol/test/CMakeLists.txt new file mode 100644 index 00000000000..60ab0307ff7 --- /dev/null +++ b/thrift/lib/cpp2/protocol/test/CMakeLists.txt @@ -0,0 +1,48 @@ +enable_testing() +include(GoogleTest) + +# Generate required thrift dependencies +foreach(thrift_file Module CompactProtocolTestStructs) + thrift_generate( + "${thrift_file}" #file_name + "" #services + "cpp2" #language + "frozen2,layouts" #options + "${CMAKE_CURRENT_SOURCE_DIR}" #file_path + "${CMAKE_CURRENT_BINARY_DIR}" #output_path + "thrift/lib/cpp2/protocol/test" #include_prefix + THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} + ) + set_source_files_properties(${thrift_file} PROPERTIES GENERATED TRUE) + set(GENERATED_THRIFT_SOURCES ${GENERATED_THRIFT_SOURCES} ${${thrift_file}-cpp2-SOURCES}) +endforeach() + +# Test files to compile +set(PROTOCOL_TESTS + BinaryProtocolTest + CompactProtocolTest + CompactV1ProtocolTest + ContainerSkippingTest + DebugProtocolTest + F14RoundTripTest + InterfaceTest + JSONProtocolTest + ProtocolReaderStructReadStateTest + ProtocolTest + SimpleJSONProtocolTest + TraitsTest) + +# Build each test independently +foreach(test_name ${PROTOCOL_TESTS}) + add_executable(${test_name} + ${test_name}.cpp + ${GENERATED_THRIFT_SOURCES}) + target_compile_options(${test_name} PUBLIC + -Wno-shadow-compatible-local) + target_link_libraries(${test_name} + GTest::gtest_main + thriftcpp2 + Folly::folly + ) + gtest_discover_tests(${test_name}) +endforeach() \ No newline at end of file diff --git a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp index e6a8614d55e..c567c65dfdd 100644 --- a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp +++ b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp @@ -43,8 +43,9 @@ TEST(CompactProtocolTest, writeInvalidBool) { auto s = std::string(); q.appendToString(s); // Die on success. - CHECK(s != std::string(1, '\1') && s != std::string(1, '\2')) - << "invalid bool value"; + if(s != std::string(1, '\1') && s != std::string(1, '\2')) { + exit(1); + } }, "invalid bool value"); } diff --git a/thrift/lib/cpp2/protocol/test/ContainerSkippingTest.cpp b/thrift/lib/cpp2/protocol/test/ContainerSkippingTest.cpp index cbd2b861cd5..7ac410e6b14 100644 --- a/thrift/lib/cpp2/protocol/test/ContainerSkippingTest.cpp +++ b/thrift/lib/cpp2/protocol/test/ContainerSkippingTest.cpp @@ -21,6 +21,7 @@ #include #include #include +#include namespace apache::thrift::test { From 0bc1d18a7e5a9388f71051e5bf9e23afe5d34454 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Mon, 4 Nov 2024 16:04:14 +0100 Subject: [PATCH 03/11] Migrate overloaded macro to optional named parameter --- ThriftLibrary.cmake | 118 +++++++++------------------- thrift/lib/cpp2/test/CMakeLists.txt | 6 +- 2 files changed, 42 insertions(+), 82 deletions(-) diff --git a/ThriftLibrary.cmake b/ThriftLibrary.cmake index 70d01107598..6eea39f0a82 100644 --- a/ThriftLibrary.cmake +++ b/ThriftLibrary.cmake @@ -165,16 +165,18 @@ macro(bypass_source_check sources) endmacro() # -# thrift_generate_named +# thrift_generate # Supports libary names that differ from the file name (two handle two libraries with the same filename on disk (in different folders)) # This is used to codegen thrift files using the thrift compiler # Params: # @file_name - The name of the library that is generated (without the language suffix) -# @real_file_name - The name of the thrift file on disk # @services - A list of services that are declared in the thrift file # @language - The generator to use (cpp, cpp2 or py3) # @options - Extra options to pass to the generator # @output_path - The directory where the thrift file lives +# @include_prefix - Prefix to use for thrift includes in generated sources +# @TARGET_NAME_BASE - (optional) - name used for target instead of real filename +# @THRIFT_INCLUDE_DIRECTORIES - (optional) path to thrift include directories # # Output: # file-language-target - A custom target to add a dependenct @@ -188,9 +190,8 @@ endmacro() # bypass_source_check(${file_language-SOURCES}) # This will prevent cmake from complaining about missing source files # -macro(thrift_generate_named +macro(thrift_generate file_name - real_file_name services language options @@ -200,49 +201,54 @@ macro(thrift_generate_named ) cmake_parse_arguments(THRIFT_GENERATE # Prefix "" # Options - "" # One Value args + "TARGET_NAME_BASE" # One Value args "THRIFT_INCLUDE_DIRECTORIES" # Multi-value args "${ARGN}") + set(source_file_name ${file_name}) + set(target_file_name ${file_name}) set(thrift_include_directories) foreach(dir ${THRIFT_GENERATE_THRIFT_INCLUDE_DIRECTORIES}) list(APPEND thrift_include_directories "-I" "${dir}") endforeach() + if(DEFINED THRIFT_GENERATE_TARGET_NAME_BASE AND NOT THRIFT_GENERATE_TARGET_NAME_BASE STREQUAL "") + set(target_file_name ${THRIFT_GENERATE_TARGET_NAME_BASE}) + endif() - set("${file_name}-${language}-HEADERS" - ${output_path}/gen-${language}/${real_file_name}_constants.h - ${output_path}/gen-${language}/${real_file_name}_data.h - ${output_path}/gen-${language}/${real_file_name}_metadata.h - ${output_path}/gen-${language}/${real_file_name}_types.h - ${output_path}/gen-${language}/${real_file_name}_types.tcc + set("${target_file_name}-${language}-HEADERS" + ${output_path}/gen-${language}/${source_file_name}_constants.h + ${output_path}/gen-${language}/${source_file_name}_data.h + ${output_path}/gen-${language}/${source_file_name}_metadata.h + ${output_path}/gen-${language}/${source_file_name}_types.h + ${output_path}/gen-${language}/${source_file_name}_types.tcc ) - set("${file_name}-${language}-SOURCES" - ${output_path}/gen-${language}/${real_file_name}_constants.cpp - ${output_path}/gen-${language}/${real_file_name}_data.cpp - ${output_path}/gen-${language}/${real_file_name}_types.cpp + set("${target_file_name}-${language}-SOURCES" + ${output_path}/gen-${language}/${source_file_name}_constants.cpp + ${output_path}/gen-${language}/${source_file_name}_data.cpp + ${output_path}/gen-${language}/${source_file_name}_types.cpp ) if("${options}" MATCHES "layouts") - set("${file_name}-${language}-SOURCES" - ${${file_name}-${language}-SOURCES} - ${output_path}/gen-${language}/${real_file_name}_layouts.cpp + set("${target_file_name}-${language}-SOURCES" + ${${target_file_name}-${language}-SOURCES} + ${output_path}/gen-${language}/${source_file_name}_layouts.cpp ) endif() if(NOT "${options}" MATCHES "no_metadata") - set("${file_name}-${language}-SOURCES" - ${${file_name}-${language}-SOURCES} - ${output_path}/gen-${language}/${real_file_name}_metadata.cpp + set("${target_file_name}-${language}-SOURCES" + ${${target_file_name}-${language}-SOURCES} + ${output_path}/gen-${language}/${source_file_name}_metadata.cpp ) endif() foreach(service ${services}) - set("${file_name}-${language}-HEADERS" - ${${real_file_name}-${language}-HEADERS} + set("${target_file_name}-${language}-HEADERS" + ${${source_file_name}-${language}-HEADERS} ${output_path}/gen-${language}/${service}.h ${output_path}/gen-${language}/${service}.tcc ${output_path}/gen-${language}/${service}AsyncClient.h ${output_path}/gen-${language}/${service}_custom_protocol.h ) - set("${file_name}-${language}-SOURCES" - ${${real_file_name}-${language}-SOURCES} + set("${target_file_name}-${language}-SOURCES" + ${${source_file_name}-${language}-SOURCES} ${output_path}/gen-${language}/${service}.cpp ${output_path}/gen-${language}/${service}AsyncClient.cpp ) @@ -260,25 +266,25 @@ macro(thrift_generate_named set(gen_language "mstch_cpp2") elseif("${language}" STREQUAL "py3") set(gen_language "mstch_py3") - file(WRITE "${output_path}/gen-${language}/${real_file_name}/__init__.py") + file(WRITE "${output_path}/gen-${language}/${source_file_name}/__init__.py") endif() add_custom_command( - OUTPUT ${${file_name}-${language}-HEADERS} - ${${file_name}-${language}-SOURCES} + OUTPUT ${${target_file_name}-${language}-HEADERS} + ${${target_file_name}-${language}-SOURCES} COMMAND ${THRIFT1} --gen "${gen_language}:${options}${include_prefix_text}" -o ${output_path} ${thrift_include_directories} - "${file_path}/${real_file_name}.thrift" + "${file_path}/${source_file_name}.thrift" DEPENDS ${THRIFT1} - "${file_path}/${real_file_name}.thrift" - COMMENT "Generating ${file_name} files. Output: ${output_path}" + "${file_path}/${source_file_name}.thrift" + COMMENT "Generating ${target_file_name} files. Output: ${output_path}" ) add_custom_target( - ${file_name}-${language}-target ALL + ${target_file_name}-${language}-target ALL DEPENDS ${${language}-${language}-HEADERS} - ${${file_name}-${language}-SOURCES} + ${${target_file_name}-${language}-SOURCES} ) install( DIRECTORY gen-${language} @@ -288,50 +294,4 @@ macro(thrift_generate_named DIRECTORY gen-${language} DESTINATION include/${include_prefix} FILES_MATCHING PATTERN "*.tcc") -endmacro() - - -# -# thrift_generate -# This is used to codegen thrift files using the thrift compiler -# Params: -# @file_name - The name of tge thrift file -# @services - A list of services that are declared in the thrift file -# @language - The generator to use (cpp, cpp2 or py3) -# @options - Extra options to pass to the generator -# @output_path - The directory where the thrift file lives -# -# Output: -# file-language-target - A custom target to add a dependenct -# ${file-language-HEADERS} - The generated Header Files. -# ${file-language-SOURCES} - The generated Source Files. -# -# Notes: -# If any of the fields is empty, it is still required to provide an empty string -# -# When using file_language-SOURCES it should always call: -# bypass_source_check(${file_language-SOURCES}) -# This will prevent cmake from complaining about missing source files -# -macro(thrift_generate - file_name - services - language - options - file_path - output_path - include_prefix -) - - thrift_generate_named( - "${file_name}" - "${file_name}" - "${services}" - "${language}" - "${options}" - "${file_path}" - "${output_path}" - "${include_prefix}" - "${ARGN}" - ) endmacro() \ No newline at end of file diff --git a/thrift/lib/cpp2/test/CMakeLists.txt b/thrift/lib/cpp2/test/CMakeLists.txt index b92393030ed..fe335da3206 100644 --- a/thrift/lib/cpp2/test/CMakeLists.txt +++ b/thrift/lib/cpp2/test/CMakeLists.txt @@ -48,9 +48,8 @@ set(ADDITIONAL_SOURCE_FILES ../../thrift/detail/protocol.cpp ../../thrift/TypeTo # generate conformance thrift files foreach(conf_name "protocol" "type" "any" "serialization" "patch_data" "conformance") - thrift_generate_named( - "${conf_name}conformance" #file_name - "${conf_name}" + thrift_generate( + "${conf_name}" #file_name "" #services "cpp2" #language "frozen2" #options @@ -58,6 +57,7 @@ foreach(conf_name "protocol" "type" "any" "serialization" "patch_data" "conforma "${CMAKE_CURRENT_BINARY_DIR}/../../../conformance/if" #output_path "thrift/conformance/if" #include_prefix THRIFT_INCLUDE_DIRECTORIES ${THRIFT_SOURCE_DIR} + TARGET_NAME_BASE "${conf_name}conformance" ) set(GENERATED_THRIFT_SOURCES ${GENERATED_THRIFT_SOURCES} From 07e18f59f26148faeeebf2bbcf090e70e3500d19 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Tue, 5 Nov 2024 11:14:49 +0100 Subject: [PATCH 04/11] Changed formatting & documentation --- ThriftLibrary.cmake | 15 +++++++++------ thrift/lib/cpp2/protocol/test/CMakeLists.txt | 16 +++++++++++++++- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/ThriftLibrary.cmake b/ThriftLibrary.cmake index 6eea39f0a82..651bebe9719 100644 --- a/ThriftLibrary.cmake +++ b/ThriftLibrary.cmake @@ -117,7 +117,7 @@ endmacro() # #include_prefix # ) # add_library(somelib ...) -# target_link_libraries(somelibe ${file_name}-${language} ...) +# target_link_libraries(somelib ${file_name}-${language} ...) # macro(thrift_library @@ -166,10 +166,12 @@ endmacro() # # thrift_generate -# Supports libary names that differ from the file name (two handle two libraries with the same filename on disk (in different folders)) +# Supports libary names that differ from the file name (to handle two libraries +# with the same filename on disk (in different folders)) # This is used to codegen thrift files using the thrift compiler # Params: -# @file_name - The name of the library that is generated (without the language suffix) +# @file_name - Input file name. Will be used for naming the CMake +# target if TARGET_NAME_BASE is not specified. # @services - A list of services that are declared in the thrift file # @language - The generator to use (cpp, cpp2 or py3) # @options - Extra options to pass to the generator @@ -179,7 +181,7 @@ endmacro() # @THRIFT_INCLUDE_DIRECTORIES - (optional) path to thrift include directories # # Output: -# file-language-target - A custom target to add a dependenct +# file-language-target - A custom target to add a dependency # ${file-language-HEADERS} - The generated Header Files. # ${file-language-SOURCES} - The generated Source Files. # @@ -211,7 +213,8 @@ macro(thrift_generate foreach(dir ${THRIFT_GENERATE_THRIFT_INCLUDE_DIRECTORIES}) list(APPEND thrift_include_directories "-I" "${dir}") endforeach() - if(DEFINED THRIFT_GENERATE_TARGET_NAME_BASE AND NOT THRIFT_GENERATE_TARGET_NAME_BASE STREQUAL "") + if(DEFINED THRIFT_GENERATE_TARGET_NAME_BASE + AND NOT THRIFT_GENERATE_TARGET_NAME_BASE STREQUAL "") set(target_file_name ${THRIFT_GENERATE_TARGET_NAME_BASE}) endif() @@ -294,4 +297,4 @@ macro(thrift_generate DIRECTORY gen-${language} DESTINATION include/${include_prefix} FILES_MATCHING PATTERN "*.tcc") -endmacro() \ No newline at end of file +endmacro() diff --git a/thrift/lib/cpp2/protocol/test/CMakeLists.txt b/thrift/lib/cpp2/protocol/test/CMakeLists.txt index 60ab0307ff7..a413e8019f0 100644 --- a/thrift/lib/cpp2/protocol/test/CMakeLists.txt +++ b/thrift/lib/cpp2/protocol/test/CMakeLists.txt @@ -1,4 +1,18 @@ -enable_testing() +# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + include(GoogleTest) # Generate required thrift dependencies From 2d911f0ff0ce3ff4e9aabf7c364fe6c3386fd137 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Thu, 7 Nov 2024 13:17:27 +0100 Subject: [PATCH 05/11] Added trailing nl --- thrift/lib/cpp2/protocol/test/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/thrift/lib/cpp2/protocol/test/CMakeLists.txt b/thrift/lib/cpp2/protocol/test/CMakeLists.txt index a413e8019f0..74f8779293e 100644 --- a/thrift/lib/cpp2/protocol/test/CMakeLists.txt +++ b/thrift/lib/cpp2/protocol/test/CMakeLists.txt @@ -59,4 +59,4 @@ foreach(test_name ${PROTOCOL_TESTS}) Folly::folly ) gtest_discover_tests(${test_name}) -endforeach() \ No newline at end of file +endforeach() From bd12461128b12efa5fc2d7a3342910a753ce7988 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Fri, 8 Nov 2024 09:09:22 +0100 Subject: [PATCH 06/11] Fixed typo --- ThriftLibrary.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ThriftLibrary.cmake b/ThriftLibrary.cmake index 651bebe9719..2a4bbfb9b84 100644 --- a/ThriftLibrary.cmake +++ b/ThriftLibrary.cmake @@ -166,9 +166,9 @@ endmacro() # # thrift_generate -# Supports libary names that differ from the file name (to handle two libraries -# with the same filename on disk (in different folders)) # This is used to codegen thrift files using the thrift compiler +# Supports library names that differ from the file name (to handle two libraries +# with the same filename on disk (in different folders)) # Params: # @file_name - Input file name. Will be used for naming the CMake # target if TARGET_NAME_BASE is not specified. From aa02a7d4eaee99daa409ef7fddddf78809270445 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Sat, 9 Nov 2024 12:47:03 +0100 Subject: [PATCH 07/11] Changing comments and fixing invalid bool test cases --- thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp | 2 +- thrift/lib/cpp2/protocol/test/CMakeLists.txt | 6 ++---- thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp | 2 +- thrift/lib/cpp2/test/CMakeLists.txt | 5 ++--- 4 files changed, 6 insertions(+), 9 deletions(-) diff --git a/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp b/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp index e0bed9f4764..a0755458210 100644 --- a/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp +++ b/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp @@ -56,7 +56,7 @@ TEST_F(BinaryProtocolTest, writeInvalidBool) { auto s = std::string(); q.appendToString(s); // Die on success. - if(s != std::string(1, '\0')) { + if (s == std::string(1, '\0')) { exit(1); } }, diff --git a/thrift/lib/cpp2/protocol/test/CMakeLists.txt b/thrift/lib/cpp2/protocol/test/CMakeLists.txt index 74f8779293e..62feca95eed 100644 --- a/thrift/lib/cpp2/protocol/test/CMakeLists.txt +++ b/thrift/lib/cpp2/protocol/test/CMakeLists.txt @@ -1,18 +1,16 @@ -# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -# SPDX-License-Identifier: Apache-2.0 +# Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at # -# http://www.apache.org/licenses/LICENSE-2.0 +# http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - include(GoogleTest) # Generate required thrift dependencies diff --git a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp index c567c65dfdd..95d0a6af29a 100644 --- a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp +++ b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp @@ -43,7 +43,7 @@ TEST(CompactProtocolTest, writeInvalidBool) { auto s = std::string(); q.appendToString(s); // Die on success. - if(s != std::string(1, '\1') && s != std::string(1, '\2')) { + if (s == std::string(1, '\1') || == std::string(1, '\2')) { exit(1); } }, diff --git a/thrift/lib/cpp2/test/CMakeLists.txt b/thrift/lib/cpp2/test/CMakeLists.txt index fe335da3206..55f6eaa3916 100644 --- a/thrift/lib/cpp2/test/CMakeLists.txt +++ b/thrift/lib/cpp2/test/CMakeLists.txt @@ -1,11 +1,10 @@ -# SPDX-FileCopyrightText: Copyright (c) 2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -# SPDX-License-Identifier: Apache-2.0 +# Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at # -# http://www.apache.org/licenses/LICENSE-2.0 +# http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, From e61ed2774bb4119c3e573225f8309987de2a88e2 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Tue, 12 Nov 2024 14:59:30 +0000 Subject: [PATCH 08/11] Fixed typo --- thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp index 95d0a6af29a..fb45ce72cd1 100644 --- a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp +++ b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp @@ -43,7 +43,7 @@ TEST(CompactProtocolTest, writeInvalidBool) { auto s = std::string(); q.appendToString(s); // Die on success. - if (s == std::string(1, '\1') || == std::string(1, '\2')) { + if (s == std::string(1, '\1') || s == std::string(1, '\2')) { exit(1); } }, From 24775380e0478b6c94cdcf70a79306c45695da6c Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Tue, 19 Nov 2024 09:56:55 +0100 Subject: [PATCH 09/11] Updated copyright notice --- thrift/lib/cpp2/protocol/test/CMakeLists.txt | 2 +- thrift/lib/cpp2/test/CMakeLists.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/thrift/lib/cpp2/protocol/test/CMakeLists.txt b/thrift/lib/cpp2/protocol/test/CMakeLists.txt index 62feca95eed..82bc59842a5 100644 --- a/thrift/lib/cpp2/protocol/test/CMakeLists.txt +++ b/thrift/lib/cpp2/protocol/test/CMakeLists.txt @@ -1,4 +1,4 @@ -# Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# Copyright (c) Meta Platforms, Inc. and affiliates and Contributors. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/thrift/lib/cpp2/test/CMakeLists.txt b/thrift/lib/cpp2/test/CMakeLists.txt index 55f6eaa3916..03602b18854 100644 --- a/thrift/lib/cpp2/test/CMakeLists.txt +++ b/thrift/lib/cpp2/test/CMakeLists.txt @@ -1,4 +1,4 @@ -# Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# Copyright (c) Meta Platforms, Inc. and affiliates and Contributors. All rights reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. From 360ea6231dc0520d3029f53bb9ea16adb9aa9977 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Tue, 26 Nov 2024 14:32:55 +0000 Subject: [PATCH 10/11] Add CHECK call again in a new function --- .../protocol/test/CompactProtocolTest.cpp | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp index fb45ce72cd1..69d26033e87 100644 --- a/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp +++ b/thrift/lib/cpp2/protocol/test/CompactProtocolTest.cpp @@ -31,21 +31,24 @@ bool makeInvalidBool() { return *reinterpret_cast("\x42"); } +void writeInvalidBoolCheck() { + auto w = CompactProtocolWriter(); + auto q = folly::IOBufQueue(); + w.setOutput(&q); + w.writeBool(makeInvalidBool()); + auto s = std::string(); + q.appendToString(s); + // Die on success. + CHECK(s != std::string(1, '\1') && s != std::string(1, '\2')) + << "invalid bool value"; +} + TEST(CompactProtocolTest, writeInvalidBool) { - auto w = CompactProtocolWriter(); - auto q = folly::IOBufQueue(); - w.setOutput(&q); // writeBool should either throw or write a valid bool. The exact value may // depend on the build mode because the optimizer can make use of the UB. EXPECT_DEATH( { - w.writeBool(makeInvalidBool()); - auto s = std::string(); - q.appendToString(s); - // Die on success. - if (s == std::string(1, '\1') || s == std::string(1, '\2')) { - exit(1); - } + writeInvalidBoolCheck(); }, "invalid bool value"); } From d12f3810f5f0c8e9b1ccc0d36dbef667c5685e49 Mon Sep 17 00:00:00 2001 From: Lukas Alt Date: Tue, 26 Nov 2024 18:21:07 +0100 Subject: [PATCH 11/11] Fixed binary protocol test case --- .../cpp2/protocol/test/BinaryProtocolTest.cpp | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp b/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp index a0755458210..48b9d2ab946 100644 --- a/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp +++ b/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp @@ -45,20 +45,25 @@ bool makeInvalidBool() { return *reinterpret_cast("\x42"); } -TEST_F(BinaryProtocolTest, writeInvalidBool) { - auto w = BinaryProtocolWriter(); +void testWriteInvalidBool() { + auto w = BinaryProtocolWriter(); auto q = folly::IOBufQueue(); w.setOutput(&q); // writeBool should either fail CHECK or write a valid bool. - EXPECT_DEATH( - { - w.writeBool(makeInvalidBool()); + + w.writeBool(makeInvalidBool()); auto s = std::string(); q.appendToString(s); // Die on success. - if (s == std::string(1, '\0')) { - exit(1); - } + + CHECK(s != std::string(1, '\0')) << "invalid bool value"; + + +} +TEST_F(BinaryProtocolTest, writeInvalidBool) { + EXPECT_DEATH( + { + testWriteInvalidBool(); }, "invalid bool value"); }