PARQUET-437: Add googletest setup and ADD_PARQUET_TEST helper by wesm · Pull Request #19 · apache/parquet-cpp

wesm · 2016-01-19T22:28:30Z

I adapted this functionality from Apache Kudu (incubating). There are no real unit tests, yet, but you can now run ctest after building to run all tests that have been created with ADD_PARQUET_TEST.

wesm · 2016-01-19T22:32:12Z

I think I got an older version of gtest, let me try updating to 1.7.0.

wesm · 2016-01-19T22:43:55Z

OK, this is now googletest 1.7.0 with the source files manually inlined into gtest-all.cc to simplify compilation.

nongli · 2016-01-19T23:38:12Z

Why are we inlining the source?

wesm · 2016-01-19T23:46:16Z

Here was the original https://github.com/google/googletest/blob/release-1.7.0/src/gtest-all.cc. I can omit gtest-all.cc and instead list out each of the individual sources

wesm · 2016-01-19T23:46:23Z

Either way is fine with me

nongli · 2016-01-20T00:12:04Z

Why are we adding the gtest source into this repo? This seems different than how we manage other third party dependencies.

wesm · 2016-01-20T00:22:08Z

Let me set things up to make googletest the same as the other thirdparty libraries

nongli · 2016-01-20T00:25:53Z

Thanks. I think kudu was particularly fancy and has patched it but I don't think we need that yet. If we do, we can think harder about how to deal with cases like that.

wesm · 2016-01-20T01:33:54Z

Annoyingly googletest is blacklisted in homebrew. Almost have this working

wesm · 2016-01-20T02:44:51Z

@nongli OK, I got the build working. The Linux compiler settings were messing up OS X but I fixed that finally

nongli · 2016-01-20T18:15:28Z

.travis.yml

can this pull this from thirdparty/versions.sh?

nongli · 2016-01-20T18:19:59Z

Thanks for doing this. Can you add a quick update to the README and the end of the build step on how to run tests?

wesm · 2016-01-20T18:25:12Z

Sure thing.

wesm · 2016-01-20T21:34:31Z

Something's up with Travis. I pushed commits to address the comments and the builds errored with no logs. Trying a touched version of the last passing build

wesm · 2016-01-20T21:35:22Z

https://www.traviscistatus.com/incidents/fcllblkclgmb

wesm · 2016-01-21T04:31:02Z

Okay, after a few bash mishaps we are back to green. @nongli have a look and lmk if any more comments.

…d support scripts for using ctest after make

nongli · 2016-01-25T22:14:27Z

setup_build_env.sh

This doesn't work for me. I think build_thirdparty leaves the working directory in build/.

looking now

wesm · 2016-01-25T23:46:22Z

@nongli fixed; this was already fixed this in #23 actually

nongli · 2016-01-26T00:28:47Z

How do you run setup_build_env.sh? If i just run it

./setup_build_env.sh, it doesn't set *_HOME so cmake . can't find them.

If I source it, it seems to mess up the shell environment (I think -e is set).

wesm · 2016-01-26T00:35:06Z

It should work now

nongli · 2016-01-26T00:50:06Z

Running into this now. Any ideas? The version mismatch is weird.

ld: warning: object file (../../build/thirdparty/googletest-release-1.7.0/libgtest.a(gtest-all.cc.o)) was built for newer OSX version (10.10) than being linked (10.9)
Undefined symbols for architecture x86_64:
  "testing::internal::MakeAndRegisterTestInfo(char const*, char const*, char const*, char const*, testing::internal::CodeLocation, void const*, void (*)(), void (*)(), testing::internal::TestFactoryBase*)", referenced from:
      ___cxx_global_var_init in reader-test.cc.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

wesm · 2016-01-26T00:54:49Z

I think that's an -fPIC issue, let me try to build from scratch on my Mac

wesm · 2016-01-26T01:01:40Z

I was able to compile without making any changes but I made sure that gtest is being compiled with -fPIC (it was not before, I think that's your linker error). I also removed the CMAKE_OSX_DEPLOYMENT_TARGET which was causing the ld version warning.

nongli · 2016-01-26T06:19:34Z

@wesm can you remove the brackets in the title. I'll merge this.

wesm · 2016-01-26T08:10:18Z

done

nongli · 2016-01-26T18:05:57Z

@wesm merged. Thanks!

wesm · 2016-01-26T18:30:25Z

thanks!

I adapted this functionality from Apache Kudu (incubating). There are no real unit tests, yet, but you can now run `ctest` after building to run all tests that have been created with `ADD_PARQUET_TEST`. Author: Wes McKinney <wes@cloudera.com> Closes apache#19 from wesm/googletest-infra and squashes the following commits: 758328f [Wes McKinney] BLD: disable fixed OSX deployment target. Compile gtest with -fPIC 61cc5bb [Wes McKinney] Remove 'set -e' from setup_build_env.sh 6435970 [Wes McKinney] Fix setup_build_env.sh script a54a219 [Wes McKinney] Add googletest to thirdparty and add ADD_PARQUET_TEST cmake helper and support scripts for using ctest after make

wesm force-pushed the googletest-infra branch from 16b54c7 to e237a55 Compare January 19, 2016 22:43

wesm force-pushed the googletest-infra branch from e237a55 to add00eb Compare January 20, 2016 01:09

wesm mentioned this pull request Jan 20, 2016

PARQUET-434: Add a ParquetFileReader class #20

Closed

nongli reviewed Jan 20, 2016
View reviewed changes

.travis.yml Outdated

Copy link
Copy Markdown

Contributor

nongli Jan 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this pull this from thirdparty/versions.sh?

wesm force-pushed the googletest-infra branch from 037ecea to 2fbedf2 Compare January 20, 2016 21:33

wesm force-pushed the googletest-infra branch 2 times, most recently from 423c99d to 24250f5 Compare January 21, 2016 04:23

wesm mentioned this pull request Jan 21, 2016

PARQUET-418: Refactored parquet_reader utility for printing file contents. #18

Closed

Add googletest to thirdparty and add ADD_PARQUET_TEST cmake helper an…

a54a219

…d support scripts for using ctest after make

wesm force-pushed the googletest-infra branch from 24250f5 to a54a219 Compare January 22, 2016 21:11

wesm mentioned this pull request Jan 23, 2016

PARQUET-433: Add TypedColumnReader template and templatize value decoders #22

Closed

nongli reviewed Jan 25, 2016
View reviewed changes

Fix setup_build_env.sh script

6435970

Remove 'set -e' from setup_build_env.sh

61cc5bb

BLD: disable fixed OSX deployment target. Compile gtest with -fPIC

758328f

wesm changed the title ~~[PARQUET-437]: Add googletest setup and ADD_PARQUET_TEST helper~~ PARQUET-437: Add googletest setup and ADD_PARQUET_TEST helper Jan 26, 2016

asfgit closed this in b31baa0 Jan 26, 2016

wesm deleted the googletest-infra branch January 26, 2016 18:30

Conversation

wesm commented Jan 19, 2016

Uh oh!

wesm commented Jan 19, 2016

Uh oh!

wesm commented Jan 19, 2016

Uh oh!

nongli commented Jan 19, 2016

Uh oh!

wesm commented Jan 19, 2016

Uh oh!

wesm commented Jan 19, 2016

Uh oh!

nongli commented Jan 20, 2016

Uh oh!

wesm commented Jan 20, 2016

Uh oh!

nongli commented Jan 20, 2016

Uh oh!

wesm commented Jan 20, 2016

Uh oh!

wesm commented Jan 20, 2016

Uh oh!

nongli Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

nongli commented Jan 20, 2016

Uh oh!

wesm commented Jan 20, 2016

Uh oh!

wesm commented Jan 20, 2016

Uh oh!

wesm commented Jan 20, 2016

Uh oh!

wesm commented Jan 21, 2016

Uh oh!

nongli Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

wesm Jan 25, 2016

Choose a reason for hiding this comment

Uh oh!

wesm commented Jan 25, 2016

Uh oh!

nongli commented Jan 26, 2016

Uh oh!

wesm commented Jan 26, 2016

Uh oh!

nongli commented Jan 26, 2016

Uh oh!

wesm commented Jan 26, 2016

Uh oh!

wesm commented Jan 26, 2016

Uh oh!

nongli commented Jan 26, 2016

Uh oh!

wesm commented Jan 26, 2016

Uh oh!

nongli commented Jan 26, 2016

Uh oh!

wesm commented Jan 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants