Bad idea to bundle gtest

[yurivict]

Test fails on the system with pre-installed googletest-1.7.0:

In file included from gtest/src/gtest-all.cc:42:
gtest/src/gtest.cc:991:13: error: use of undeclared identifier 'EditType'
std::vector<EditType> CalculateOptimalEdits(const std::vector<size_t>& left,
            ^
gtest/src/gtest.cc:995:27: error: use of undeclared identifier 'EditType'
  std::vector<std::vector<EditType> > best_move(

It's better to just say in the README that test requires googletest and not bundle it.

[yurivict]

This patch to Makefile makes it use the standard googletest install.

--- Makefile.orig       2016-06-21 21:09:17 UTC
+++ Makefile
@@ -48,7 +48,7 @@ check: test
        ./test --alsologtostderr --gtest_color=yes

 clean:
-       $(RM) test hiptext $(wildcard *.o *.d *.S $(GTEST_DIR)/*.o) cpplint.py
+       $(RM) test hiptext $(wildcard *.o *.d *.S) cpplint.py

 install: hiptext
        install --mode=0755 hiptext $(PREFIX)/bin
@@ -78,12 +78,11 @@ pixel_parse.o: CXXFLAGS := $(filter-out 
 font.%:        CXXFLAGS += $(shell freetype-config --cflags)

 # google-test integration magic.
-GTEST_DIR ?= gtest
-TESTS = $(GTEST_DIR)/src/gtest-all.o $(GTEST_DIR)/src/gtest_main.o \
-        $(patsubst %.cc,%.o,$(wildcard *_test.cc))
-$(TESTS): CXXFLAGS += -I$(GTEST_DIR)/include -I$(GTEST_DIR) -pthread
+GTEST_DIR = $(PREFIX)
+TESTS = $(patsubst %.cc,%.o,$(wildcard *_test.cc))
+$(TESTS): CXXFLAGS += -I$(GTEST_DIR)/include -pthread
 $(filter gtest%,$(TESTS)): CXXFLAGS := $(filter-out -MD -Wall,$(CXXFLAGS))
-test: $(TESTS) $(SOURCES) ; $(LINK.cc) $^ $(LDLIBS) -lpthread -o $@
+test: $(TESTS) $(SOURCES) ; $(LINK.cc) $^ $(LDLIBS) -lpthread -L$(GTEST_DIR)/lib -lgtest -lgtest_main -o $@

 # Recompile sources when headers change.
 CXXFLAGS += -MD

[jart]

One of the reasons I didn't bundle google test years ago, is because it was kind of a mess in the open source world. It didn't really have a properly released autoconf project where you could run pkgconfig or anything like that. There were some other things weird about it I couldn't quite remember. Hopefully things are better now.

In all seriousness, I'm actually considering migrating this project to Bazel. What would you think about that?

[yurivict]

Hm, Bazel looks a bit over-engineered to me. Gigantic, requires Java. I realize bazel is the tool used in google. But it seems like an overkill for this project. When I used bazel it also often left the background processes running after you stop it.

[jart]

Yep! That's Bazel. It's what we use at Google to compile a repository with literally petabytes of code. But since this repo doesn't have petabytes of code, maybe I should keep it traditional and just set up a proper autotools build system. Thoughts on that?

[yurivict]

Yes, I think autotools fits hiptext well.

[jart]

I just migrated to GNU autotools. PTAL?

I'm still bundling gtest. I took a look at libgtest-dev on Ubuntu 14.04. The way it's packaged isn't kosher. It doesn't actually have a compiled .a or .so file. They just drop /usr/src/gtest/src/gtest.cc on the filesystem and expect you to compile it yourself. Is this your doing @steverobbins?

Reading your diff @yurivict, it sounds like FreeBSD packages gtest in a friendly way. So I'm not sure how I'd unbundle gtest and be able to support all distros.

Is there a way to force clang/gcc to make my -Igtest/include flag take precedence? Maybe that's the real problem.

[yurivict]

I had to remove the bundle in the freebsd port: https://svnweb.freebsd.org/ports/head/graphics/hiptext/Makefile?revision=417308&view=markup

Bundles only cause trouble in general. I know, in this case it is a faulty distro(s) problem.

[jart]

Thanks for packaging this for FreeBSD. When you get a chance to look at the new autotools setup, let me know if you approve. Otherwise I'll change it.

[yurivict]

You welcome!

I look at a875509. You expect .pc from libgflags, but gflags-2.1.2 doesn't provide .pc, despite gflags/gflags#50 saying it was committed in 1.4, and gflags/gflags#120 asking for it again. They need to sort it out there.

[yurivict]

You probably should just wait for them to fix it, no need to change hiptext itself.

[jart]

I'm assuming then that the Debian maintainers wrote their own pkgconfig
file. I'll yell at them.

On Fri, Jun 24, 2016 at 2:23 PM yurivict notifications@github.com wrote:

You probably should just urge them to fix it, mo need to change hiptext
itself.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#35 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AADAbiv0TI7p6FQI9z8HElzdVyVZuj1eks5qPCC5gaJpZM4I7Xlr
.

[yurivict]

Hi @jart ,

gflags now provides .pc file: /usr/local/libdata/pkgconfig/gflags.pc

Cheers,
Yuri

[jart]

That's great news. Kudos on gflags maintainer for doing the right thing. Would anyone be interested in sending a pull request to this project, helping it use the new pkgconfig file?