PARQUET-416: C++11 compilation, code reorg, libparquet and installation targets by wesm · Pull Request #14 · apache/parquet-cpp

wesm · 2016-01-07T00:10:09Z

Reorganize code into a top level src/parquet directly, add a libparquet shared library, and add install targets for libparquet and its header files. Add cpplint script and make lint target for code linting.

Replaces earlier PR #13

wesm · 2016-01-07T00:23:44Z

OS X is failing with a homebrew error on brew install thrift; trying work around for now, if not we can disable OS X for the time being.

wesm · 2016-01-07T01:15:22Z

I disabled OS X in the Travis CI setup for now; the .travis.yml will need some more twiddling which can go in a separate patch. For some reason brew install thrift was failing with a checksum error

nongli · 2016-01-07T01:29:24Z

I tried this patch on osx by cloning and following the build steps in the readme. It worked on master but not this patch.

cmake .
...
Make Warning (dev):
Policy CMP0042 is not set: MACOSX_RPATH is enabled by default. Run "cmake
--help-policy CMP0042" for policy details. Use the cmake_policy command to
set the policy and suppress this warning.
MACOSX_RPATH is not specified for the following targets:
parquet
This warning is for project developers. Use -Wno-dev to suppress it.

make [ 50%] Linking CXX shared library build/debug/libparquet.dylib
Undefined symbols for architecture x86_64:
"snappy::RawCompress(char const*, unsigned long, char*, unsigned long*)", referenced from:
parquet_cpp::SnappyCodec::Compress(int, unsigned char const*, int, unsigned char*) in libparquet_compression.a(snappy-codec.cc.o)
"snappy::RawUncompress(char const*, unsigned long, char*)", referenced from:
parquet_cpp::SnappyCodec::Decompress(int, unsigned char const*, int, unsigned char*) in libparquet_compression.a(snappy-codec.cc.o)
"snappy::MaxCompressedLength(unsigned long)", referenced from:
parquet_cpp::SnappyCodec::MaxCompressedLen(int, unsigned char const*) in libparquet_compression.a(snappy-codec.cc.o)
"typeinfo for apache::thrift::transport::TMemoryBuffer", referenced from:
boost::shared_ptr<apache::thrift::transport::TMemoryBuffer> boost::dynamic_pointer_cast<apache::thrift::transport::TMemoryBuffer, apache::thrift::transport::TTransport>(boost::shared_ptr<apache::thrift::transport::TTransport> const&) in parquet.cc.o
"typeinfo for apache::thrift::transport::TTransportException", referenced from:
apache::thrift::transport::TMemoryBuffer::TMemoryBuffer(unsigned char*, unsigned int, apache::thrift::transport::TMemoryBuffer::MemoryPolicy) in parquet.cc.o
apache::thrift::transport::TTransport::open() in parquet.cc.o
apache::thrift::transport::TTransport::close() in parquet.cc.o
apache::thrift::transport::TTransport::read_virt(unsigned char*, unsigned int) in parquet.cc.o
apache::thrift::transport::TTransport::write_virt(unsigned char const*, unsigned int) in parquet.cc.o
apache::thrift::transport::TTransport::consume_virt(unsigned int) in parquet.cc.o
unsigned int apache::thrift::transport::readAll<apache::thrift::transport::TTransport>(apache::thrift::transport::TTransport&, unsigned char*, unsigned int) in parquet.cc.o
...
"vtable for apache::thrift::transport::TMemoryBuffer", referenced from:
apache::thrift::transport::TMemoryBuffer::TMemoryBuffer(unsigned char*, unsigned int, apache::thrift::transport::TMemoryBuffer::MemoryPolicy) in parquet.cc.o
NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
"vtable for apache::thrift::transport::TTransportException", referenced from:
apache::thrift::transport::TTransportException::TTransportException(apache::thrift::transport::TTransportException::TTransportExceptionType, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) in parquet.cc.o
NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
ld: symbol(s) not found for architecture x86_64

nongli · 2016-01-07T01:36:32Z

why doesn't this have more things in it? e.g. snappy

Ok, If i add snappystatic and thriftstatic, it compiles.

wesm · 2016-01-07T01:43:31Z

Dynamic linking on OS X is a nightmare. With the fix I put in ./debug/decode-benchmark works and dynamically links to libparquet.dylib properly.

wesm · 2016-01-07T01:50:02Z

As soon as we get the build environment question sorted out it would be good to use the same linkage as libparquet for the dependencies that have both static and shared libraries available.

nongli · 2016-01-07T02:15:42Z

Cool, I tried the latest version in your branch and it builds.

nongli · 2016-01-07T02:25:46Z

@wesm can you fix the build issue. I think you just need to add -fPIC

nongli · 2016-01-07T03:59:56Z

This looks good to me. Can you file a jira and prefix the commit so the title is
PARQUET-XXX: description?

wesm · 2016-01-07T18:29:26Z

Ready for cherry-picking. I updated the PARQUET-416 title and description. Thanks!

nongli · 2016-01-07T19:26:09Z

@wesm is disabling osx still necessary?

wesm · 2016-01-07T19:46:35Z

I can try the build again to see if the Homebrew thrift error cleared. I also could not get Thrift to build from source on Travis yesterday

nongli · 2016-01-07T19:48:43Z

Nvm. I got confused. It's not the osx issues that that been fixed.

Thanks. I'll merge this later today.

grundprinzip · 2016-01-08T04:52:27Z

can you wrap this in if (APPLE) please?

wesm · 2016-01-08T17:54:17Z

@grundprinzip sure; I'm seeing if OS X travis build will pass now and then will squash all these again

kalaxy · 2016-01-08T20:45:41Z

Let me know when this is no longer in flux and I'lll rebase on top of it again with my pull request.

…tr with std::shared_ptr and other C++11 fixes. Reorganize code into a top level src/parquet directly, add a libparquet shared library, and add install targets for libparquet and its header files. Add cpplint script and `make lint` target for code linting.

wesm · 2016-01-08T21:34:42Z

@kalaxy I got gcc-4.9 working and got the OS X build passing again (the magic trick was to call brew update before using brew install >:|). @nongli this should be good to cherry pick if the build is passing

nongli · 2016-01-08T23:52:46Z

@wesm Thanks! Can you not squash in the future? It makes it hard to review the deltas and the merge tool we use will squash them.

wesm · 2016-01-09T00:37:48Z

Sure, no problem.

This is based off of pull request #14. Author: Kalon Mills <kmills@adobe.com> Closes#16 from kalaxy/libparquet-library-update-build and squashes the following commits: 0ce51db [Kalon Mills] Add script for automating build env setup. 82a198c [Kalon Mills] Make thrift build on mac only a warning when not specified explicitly. d096c64 [Kalon Mills] Update build instructions. 6709182 [Kalon Mills] Support thrift dependency in thirdparty scripts for linux. bedd0d4 [Kalon Mills] Remove thirdparty code lz4 from repo. 30c2b7e [Kalon Mills] Support build environment configuration of LZ4 library. 73e7785 [Kalon Mills] Support build environment configuration of Snappy library.

This is based off of pull request apache/parquet-cpp#14. Author: Kalon Mills <kmills@adobe.com> Closesapache#16 from kalaxy/libparquet-library-update-build and squashes the following commits: 0ce51db [Kalon Mills] Add script for automating build env setup. 82a198c [Kalon Mills] Make thrift build on mac only a warning when not specified explicitly. d096c64 [Kalon Mills] Update build instructions. 6709182 [Kalon Mills] Support thrift dependency in thirdparty scripts for linux. bedd0d4 [Kalon Mills] Remove thirdparty code lz4 from repo. 30c2b7e [Kalon Mills] Support build environment configuration of LZ4 library. 73e7785 [Kalon Mills] Support build environment configuration of Snappy library. Change-Id: I266283fc15457a6116fc944b29656d071a917159

wesm mentioned this pull request Jan 7, 2016
Add option for external C++ development toolchain, use C++11, configure libparquet library / header installation #13
Closed

wesm force-pushed the libparquet-library branch from 126379f to 2bd7c4aCompare January 7, 2016 00:34

nongli reviewed Jan 7, 2016
View reviewed changes

wesm force-pushed the libparquet-library branch 2 times, most recently from 8b71d4f to 636d0b7Compare January 7, 2016 03:57

wesm changed the title ~~C++11 compilation, code reorg, libparquet and installation targets~~PARQUET-416: C++11 compilation, code reorg, libparquet and installation targetsJan 7, 2016

wesm force-pushed the libparquet-library branch from 636d0b7 to 63d7794Compare January 7, 2016 18:27

grundprinzip reviewed Jan 8, 2016
View reviewed changes

Comment threadCMakeLists.txt Outdated
grundprinzipJan 8, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you wrap this in if (APPLE) please?

kalaxy mentioned this pull request Jan 8, 2016
PARQUET-267: Detach thirdparty code from build configuration. #16
Closed

wesm force-pushed the libparquet-library branch from 8b16ef2 to 2e356fdCompare January 8, 2016 21:33

asfgit closed this in 337cf58Jan 8, 2016

wesm deleted the libparquet-library branch January 9, 2016 00:37

asfimport mentioned this pull request Jun 23, 2024
[C++][Parquet] 11, cpplint cleanup, package target and header installation apache/arrow#42252
Closed

Uh oh!

Conversation

wesm commented Jan 7, 2016

Uh oh!

wesm commented Jan 7, 2016

Uh oh!

wesm commented Jan 7, 2016

Uh oh!

nongli commented Jan 7, 2016

Uh oh!

nongliJan 7, 2016

Choose a reason for hiding this comment

Uh oh!

nongliJan 7, 2016

Choose a reason for hiding this comment

Uh oh!

wesm commented Jan 7, 2016

Uh oh!

wesm commented Jan 7, 2016

Uh oh!

nongli commented Jan 7, 2016

Uh oh!

nongli commented Jan 7, 2016

Uh oh!

nongli commented Jan 7, 2016

Uh oh!

wesm commented Jan 7, 2016

Uh oh!

nongli commented Jan 7, 2016

Uh oh!

wesm commented Jan 7, 2016

Uh oh!

nongli commented Jan 7, 2016

Uh oh!

grundprinzipJan 8, 2016

Choose a reason for hiding this comment

Uh oh!

wesm commented Jan 8, 2016

Uh oh!

kalaxy commented Jan 8, 2016

Uh oh!

wesm commented Jan 8, 2016

Uh oh!

nongli commented Jan 8, 2016

Uh oh!

wesm commented Jan 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants