Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

gtest: vendor and update to 1.7.0 #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

liamstask
Copy link

I wasn't totally sure what the intention was, since it appeared the gtest sources had been combined into a single file, and were still referencing includes that were out of tree. Seems like the easiest way to ensure that the project is buildable out of the box is just to vendor the unmodified gtest release. This should make it easier to set up an autobuilder (travis-ci.org or similar) as well.

Let me know if you'd like to re-org this in any way, happy to arrange it to your liking.

…ored gtest-all.cpp

builds cleanly (minus a couple warnings in the test code itself) on OS X 10.10.4 with “Apple LLVM version 6.1.0 (clang-602.0.53) (based on LLVM 3.6.0svn)”
previously, this was emitting the following warning (treated as error):

`c++ -g -Wall -Werror -I. -Igtest-1.7.0/include -o capn-test.o -c capn-test.cpp
capn-test.cpp:66:29: error: unused variable 'SUBSTRUCT_DEFAULT' [-Werror,-Wunused-const-variable]
static const AlignedData<2> SUBSTRUCT_DEFAULT = {{0,0,0,0,1,0,0,0,  0,0,0,0,0,0,0,0}};
                            ^
capn-test.cpp:67:29: error: unused variable 'STRUCTLIST_ELEMENT_SUBSTRUCT_DEFAULT' [-Werror,-Wunused-const-variable]
static const AlignedData<2> STRUCTLIST_ELEMENT_SUBSTRUCT_DEFAULT =
                            ^`
@kylemanna
Copy link

Original build log @ https://gist.github.com/e7e563b1981170236562

@liamstask
Copy link
Author

@kylemanna - i've also included these fixes (slightly different from yours), and gotten an auto build setup at https://github.com/liamstask/c-capnproto in case you are interested in collaborating. based on msgs on the capnproto mailing list, i'm not sure this repo is likely to be updated any time soon, but would definitely hope to merge changes back in should that change in the future.

@kylemanna
Copy link

@liamstask I took a look at your repo, but it still has a few issues:

  1. Including the entire gtest repository is a portability nightmare
  2. Uninitialized variables
  3. Signedness issues

Here's the build log: https://gist.github.com/f6e0977da030db7a7971

@liamstask
Copy link
Author

@kylemanna - it looks like that build is not using the same makefiles (or at least compiler flags). the build log i see is at https://travis-ci.org/liamstask/c-capnproto/jobs/74119871

what is your portability concern with gtest? the authors recommend vendoring it per project: https://code.google.com/p/googletest/wiki/FAQ#Why_is_it_not_recommended_to_install_a_pre-compiled_copy_of_Goog

@kylemanna
Copy link

@kylemanna - it looks like that build is not using the same makefiles (or at least compiler flags). the build log i see is at https://travis-ci.org/liamstask/c-capnproto/jobs/74119871

Your build is using an old version of gcc gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3. I'm running gcc 5.2.0 and clang 3.6.2. Then again, it looks like by not setting the CXX variable correctly your build then uses gcc-4.8. The compiler flags are exactly as the Makefile says they should be.

If you review my build log, you cannot argue with the warnings that are promoted to errors. The code doesn't lie: signed types are mixed and variables are uninitialized. The gcc compiler in your build is 4+ years old and is letting these errors pass by. My branch is more explicit.

what is your portability concern with gtest? the authors recommend vendoring it per project: https://code.google.com/p/googletest/wiki/FAQ#Why_is_it_not_recommended_to_install_a_pre-compiled_copy_of_Goog

The FAQ is interesting. I'd like to avoid the mess. At minimum gtest should be downloaded and extracted by the developer. At best it would be a git submodule pointing to the repository. Dumping 90k lines of code in to this repo with no good way to maintain history and the same clunky process for updating it seems like a bad approach.

In the meantime I'll roll the dice with system pre-compiler gtest libs.

@liamstask
Copy link
Author

The issue in the compiler flags that didn't look right to me was the fact that it wasn't passing -Wno-maybe-uninitialized - are the build logs you posted from a gcc build? If so, I don't see why this is not getting added to CFLAGS, per the test near the top of the makefile. If not, that option is not enabled by default in clang and I don't see it triggering locally when running with clang 3.6.1.

There are indeed some remaining signed/unsigned comparison warnings in the test code raised by newer gcc versions, I'll definitely apply fixes for those - thanks for the heads up.

For gtest, I'm not sure vendoring it in repo is so bad - why do you say there's no way to maintain history for it? Updating it is pretty simple, and should only need to happen very infrequently, if ever.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants