ProtocolBench Build Rules

[lukalt]

This PR adds CMake build rules to build ProtocolBench. This will generate a ProtocolBench executable inside bin/ in fbthrift's install directory.

This required a change to the ThriftLibrary, as generating sources from multiple thrift files with the same name (in different) directories was no supported. Previously, this led to multiple targets with identical name in CMake. By overloading the thrift_generate macro with an variant that takes a separate target name as an input, we add support for this while maintaining backwards compatibility.

[facebook-github-bot]

Hi @lukalt!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[praihan]

Hey @lukalt Thanks for the contribution. Could you please sign the CLA?

[lukalt]

Hey @lukalt Thanks for the contribution. Could you please sign the CLA?

The signed CLA will be submitted beginning of next week. Sorry for the delay

[vitaut]

Thanks for the PR. I think we shouldn't introduce another version of thrift_generate and the latter has too many positional arguments already. Instead add an optional named argument to thrift_generate.

[lukalt]

Hey,

@praihan The CLA has been submitted now
@vitaut Thank you for the feedback. I absolutely agree and made the new parameter an optional named argument of that macro.

I also integrated the Protocol tests into the CMake project. There seems to be some incompatibility between gtest and glog causing CHECK(..) calls inside EXPECTED_DEATH to fail with the error macro "EXPECT_EXIT" passed 4 arguments, but takes just 3. I rewrote the failing lines to not use glog for now.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[vitaut]

Just one more minor comment, otherwise LGTM.

[lukalt]

Thank you. I made the final change now. Builds on Windows seem to be failing but it does not look like it is caused by my changes. It looks like it is related to fmtlib

[facebook-github-bot]

@vitaut has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[vitaut]

Looks like there are some test failures:

[ RUN      ] BinaryProtocolTest.writeInvalidBool
.../thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp:63: Failure
Death test: { w.writeBool(makeInvalidBool()); auto s = std::string(); q.appendToString(s); if(s != std::string(1, '\0')) { exit(1); } }
    Result: failed to die.
 Error msg:
[  DEATH   ] 

[facebook-github-bot]

@vitaut has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[lukalt]

@vitaut Formatting issues and test failures are now resolved

[vitaut]

@vitaut Formatting issues and test failures are now resolved

Great but please update the copyright headers per my earlier comment.

[lukalt]

@vitaut Copyright note has been updated now

[facebook-github-bot]

@vitaut has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[vitaut]

There are still test failures:

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BinaryProtocolTest
[ RUN      ] BinaryProtocolTest.writeInvalidBool
fbcode/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp:63: Failure
Death test: { w.writeBool(makeInvalidBool()); auto s = std::string(); q.appendToString(s); if (s == std::string(1, '\0')) { exit(1); } }
    Result: died but not with expected error.
  Expected: contains regular expression "invalid bool value"
Actual msg:
[  DEATH   ] 

[  FAILED  ] BinaryProtocolTest.writeInvalidBool (58 ms)

[lukalt]

There are still test failures:

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BinaryProtocolTest
[ RUN      ] BinaryProtocolTest.writeInvalidBool
fbcode/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp:63: Failure
Death test: { w.writeBool(makeInvalidBool()); auto s = std::string(); q.appendToString(s); if (s == std::string(1, '\0')) { exit(1); } }
    Result: died but not with expected error.
  Expected: contains regular expression "invalid bool value"
Actual msg:
[  DEATH   ] 

[  FAILED  ] BinaryProtocolTest.writeInvalidBool (58 ms)

You run the tests on x86, right? I was running them on arm64 and got no failures, probably because char is unsigned on aarch64. I will see how we can fix this test case

[vitaut]

You run the tests on x86, right?

Yes, the failures are on x86_64.