search

facebook / fbthrift

Facebook's branch of Apache Thrift, including a new C++ server.
Apache License 2.0
2.57k stars 608 forks source link

CMake should specify minimum Bison version #316

Closed JoeLoser closed 5 years ago

JoeLoser commented 5 years ago

The system version of Bison for me (2.3) is insufficient to build all of the targets. CMake should specify a minimum version requirement in the find_package call with Bison.

Examples:

When I try building all of the targets with Bison 2.3:

[6/152] [BISON][ThriftParser] Building parser with bison 2.3
FAILED: ../thrift/compiler/thrifty.cc ../thrift/compiler/thrifty.hh
cd /Users/joe/dev/fbthrift/thrift/compiler && /usr/bin/bison --skeleton=lalr1.cc --defines=/Users/joe/dev/fbthrift/thrift/compiler/thrifty.hh -o /Users/joe/dev/fbthrift/thrift/compiler/thrifty.cc parse/thrifty.yy
parse/thrifty.yy:108.1-5: invalid directive: `%code'
parse/thrifty.yy:108.7-14: syntax error, unexpected identifier
[15/152] Building CXX object thrift/lib/cpp/CMakeFiles/async.dir/async/THeaderAsyncChannel.cpp.o
ninja: build stopped: subcommand failed.

I no longer get this error when I use Bison 3.3.2. So, the min version must be somewhere between these two.

Definition of done: the correct minimum version is determined and specified in the top level CMakeLists.txt at https://github.com/facebook/fbthrift/blob/master/CMakeLists.txt#L78

yfeldblum commented 5 years ago

I would go with Bison v3.1 as the minimum required version (currently).

Would you want to submit the PR for that?

JoeLoser commented 5 years ago

Sure, I'll submit a PR shortly. Thanks.

JoeLoser commented 5 years ago

Corresponding PR: https://github.com/facebook/fbthrift/pull/317