Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3207)

Issue 47042: Bundle full gtest package with protocol buffers.

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years ago by kenton
Modified:
15 years ago
Reviewers:
chandlerc1, chandlerc
Base URL:
http://protobuf.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Instead of bundling bits of the gtest code with protocol buffers and compiling it as part of the protobuf makefiles, bundle the verbatim gtest package and use it as a nested autoconf package. This should make it much easier to update to the latest gtest release in the future, or use a non-bundled gtest.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -11083 lines) Patch
M Makefile.am View 1 chunk +27 lines, -1 line 2 comments Download
M autogen.sh View 1 chunk +8 lines, -1 line 0 comments Download
M configure.ac View 2 chunks +3 lines, -0 lines 0 comments Download
M src/Makefile.am View 3 chunks +7 lines, -25 lines 0 comments Download
D src/gtest/CHANGES View 1 chunk +0 lines, -3 lines 0 comments Download
D src/gtest/CONTRIBUTORS View 1 chunk +0 lines, -23 lines 0 comments Download
D src/gtest/COPYING View 1 chunk +0 lines, -28 lines 0 comments Download
D src/gtest/README View 1 chunk +0 lines, -150 lines 0 comments Download
D src/gtest/gen_gtest_pred_impl.py View 1 chunk +0 lines, -733 lines 0 comments Download
D src/gtest/gtest.h View 1 chunk +0 lines, -1243 lines 0 comments Download
D src/gtest/gtest.cc View 1 chunk +0 lines, -3540 lines 0 comments Download
D src/gtest/gtest-death-test.h View 1 chunk +0 lines, -205 lines 0 comments Download
D src/gtest/gtest-death-test.cc View 1 chunk +0 lines, -751 lines 0 comments Download
D src/gtest/gtest-filepath.cc View 1 chunk +0 lines, -208 lines 0 comments Download
D src/gtest/gtest-internal-inl.h View 1 chunk +0 lines, -1118 lines 0 comments Download
D src/gtest/gtest-message.h View 1 chunk +0 lines, -236 lines 0 comments Download
D src/gtest/gtest-port.cc View 1 chunk +0 lines, -292 lines 0 comments Download
D src/gtest/gtest-spi.h View 1 chunk +0 lines, -247 lines 0 comments Download
D src/gtest/gtest_main.cc View 1 chunk +0 lines, -39 lines 0 comments Download
D src/gtest/gtest_pred_impl.h View 1 chunk +0 lines, -368 lines 0 comments Download
D src/gtest/gtest_prod.h View 1 chunk +0 lines, -58 lines 0 comments Download
D src/gtest/internal/gtest-death-test-internal.h View 1 chunk +0 lines, -201 lines 0 comments Download
D src/gtest/internal/gtest-filepath.h View 1 chunk +0 lines, -168 lines 0 comments Download
D src/gtest/internal/gtest-internal.h View 1 chunk +0 lines, -569 lines 0 comments Download
D src/gtest/internal/gtest-port.h View 1 chunk +0 lines, -596 lines 0 comments Download
D src/gtest/internal/gtest-string.h View 1 chunk +0 lines, -280 lines 0 comments Download

Messages

Total messages: 2
kenton
15 years ago (2009-04-21 04:08:29 UTC) #1
chandlerc
15 years ago (2009-04-21 04:29:02 UTC) #2
LGTM

Out of curiosity, why not drop the gtest branch directly into your subversion
tree and always package it? I can see possible reasons, just wondered what yours
are. On at least two of my projects, they just always provide gtest in order to
have complete control over the dependencies.

Regarding the build, I actually really like this strategy upon looking at it.
The nice thing is that gtest isn't built at all until you actually need to build
tests; I'm not sure there is a "better" way to package this up. Just some random
comments below.

http://codereview.appspot.com/47042/diff/1/26
File Makefile.am (right):

http://codereview.appspot.com/47042/diff/1/26#newcode12
Line 12: DIST_SUBDIRS = gtest src
Any reason not to use $(subdirs) from AC_CONFIG_SUBDIRS here?

http://codereview.appspot.com/47042/diff/1/26#newcode21
Line 21: @(cd gtest && $(MAKE) $(AM_MAKEFLAGS) lib/libgtest.la
lib/libgtest_main.la)
Automake's manual indicates you shouldn't need the subshell here and below to
keep the cd local to the rule, but it wouldn't be the first time that manual has
misled me if that's not actually true.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b