Page MenuHomeFreeBSD

devel/googletest: build from cmake instead of autotools build infra
AbandonedPublic

Authored by ngie on Mar 3 2019, 1:47 AM.

Details

Summary

This change accomplishes the following items:

  1. Make libgtest require pthread support.
  2. Stop building static copies of the libraries.
  3. Do not install gtest-config.
  4. Build the upstream-provided tests.

Make libgtest require pthread support

As noted by @asomers in rS300906 (cddl/usr.sbin/zfsd/tests/Makefile
specifically), googletest requires libpthread in order to function, if
-DGTEST_HAS_PTHREAD is defined.

While this is true, the code was not being linked against libpthread,
resulting in libgtest being broken, and thus all tests reliant on the library
broken at runtime.

Long story short, the integration logic in googletest/m4/acx_pthread.m4 is
broken. Switching to cmake unbreaks using googletest out of the box [1].

Stop building static copies of the libraries

For whatever reason, the maintainers thought it was best to build either with
shared or static libraries, but not both. Switching to cmake makes this an
either-or transition.

Do not install gtest-config

Using gtest-config when building from a GitHub clone results in incorrect
behavior, as it looks up the headers in /usr/local/include before the
headers in the local clone, making local builds with installed copies of
devel/googletest impossible.

Build the upstream-provided tests

A number of tests are available with the gtest project. In order to do a
compare and contrast between the work I started as far as importing the
project to base, I needed to build/run them.

PR: 236169 [1]

Test Plan

I built and installed the tests. Unfortunately some of the tests don't
pass. However, given that this is the first iteration towards getting
googletest built properly for consumers, I am willing to work towards
accepting this as a better than previously working solution and fix the tests
over the longterm.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22847
Build 21935: arc lint + arc unit

Event Timeline

ngie created this revision.Mar 3 2019, 1:47 AM
ngie edited the summary of this revision. (Show Details)Mar 3 2019, 1:50 AM
ngie edited the summary of this revision. (Show Details)
mat added a comment.Mar 3 2019, 12:12 PM

I am not sure /usr/local/tests is the right place to put these files, it should probably more be something like /usr/local/share/tests.

devel/googletest/Makefile
31–36

LOCALBASE is where dependencies are installed, not where the port installs itself, that's PREFIX.

devel/googletest/pkg-plist
27

This is probably not needed, I doubt make makeplist generated this.

In D19430#416155, @mat wrote:

I am not sure /usr/local/tests is the right place to put these files, it should probably more be something like /usr/local/share/tests.

Why? /usr/local/tests is where ports install their tests. Few do it currently, but there are some, like devel/kyua.

jbeich added a comment.Mar 3 2019, 5:51 PM

devel/googlemock needs to be converted as well or merged into this port. If you're interested I can hand over maintainership of both to you.

make check-plist fails:

===> Checking for items in STAGEDIR missing from pkg-plist
Error: Orphaned: include/gtest/gtest-param-test.h.pump
Error: Orphaned: include/gtest/internal/custom/README.md
Error: Orphaned: include/gtest/internal/gtest-param-util-generated.h.pump
Error: Orphaned: include/gtest/internal/gtest-tuple.h.pump
Error: Orphaned: include/gtest/internal/gtest-type-util.h.pump
Error: Orphaned: lib/cmake/GTest/GTestConfig.cmake
Error: Orphaned: lib/cmake/GTest/GTestConfigVersion.cmake
Error: Orphaned: lib/cmake/GTest/GTestTargets-%%CMAKE_BUILD_TYPE%%.cmake
Error: Orphaned: lib/cmake/GTest/GTestTargets.cmake
Error: Orphaned: libdata/pkgconfig/gtest.pc
Error: Orphaned: libdata/pkgconfig/gtest_main.pc

This change accomplishes the following items:

  1. Makes libgtest require pthread support.
  2. Stops building static copies of the libraries.
  3. Drops gtest-config.
  4. Builds the upstream-provided tests.

Where's the rationale why installing tests by default is useful?

Makes libgtest require pthread support


[...]

Long story short, the integration logic in googletest/m4/acx_pthread.m4 is
broken. Switching to cmake unbreaks using googletest out of the box [1].

acx_pthread is not aware libc can have pthread_* stubs. Adding LIBS += -lpthread to the port would have been enough. Another option is to check a non-stubbed symbol at configure.

OTOH, consumers should use either gtest-config or pkg-config instead of guessing flags.

$ gtest-config --ldflags --libs
-L/usr/local/lib -lgtest -D_THREAD_SAFE -pthread

$ pkg-config gtest --libs
-L/usr/local/lib -lgtest -pthread

Drops gtest-config


Using gtest-config when building from a GitHub clone results in incorrect
behavior, as it looks up the headers in /usr/local/include before the
headers in the local clone, making local builds with installed copies of
devel/googletest impossible.

-I vs. -isystem issue, exposed because FreeBSD cannot agree how to treat /usr/local. It affects pkg-config as well.

devel/googletest/Makefile
19
  • Why :noninja is necessary?
  • USES=libtool isn't used with USES=cmake
22

Replace check (automake) with test (CMake):

$ make test
===>  Testing for googletest-1.8.1_2
make[1]: don't know how to make check. Stop

make[1]: stopped in /usr/ports/devel/googletest/work/.build
*** Error code 1

Stop.
make: stopped in /usr/ports/devel/googletest
26

Both are already default on FreeBSD and nop here due to a typo.

27

Already default for both Clang and GCC.

28

C++14 is already default for both Clang and GCC (implied via USES=compiler:c++11-lang). If C++11 (not C++14 or later) is important cherry-pick upstream fixes or update to a snapshot.

30

Use post-install for additional files.

35

Use ${INSTALL_WRKSRC}.

devel/googletest/pkg-plist
1

Either gtest-config or pkg-config gtest need to work. Consumers cannot be expect to know where the library is installed.

28

If SONAME has changed consumers need PORTREVISION bump. OTOH, none appear to depend at runtime:

$ pkg rquery '%o %B' | awk '/libgtest/ { print $1 }'
$ 
asomers added inline comments.Mar 3 2019, 5:57 PM
devel/googletest/Makefile
31

Installing tests should be conditionalized on a TEST option. That's what Kyua does. It should probably be off-by-default, but that's just my opinion.

ngie abandoned this revision.Mar 4 2019, 8:45 PM
ngie marked 6 inline comments as done.

Ok, trying to fidget around with this more, I can see that it's a bit too early to make the cmake cutover. I will shoot for the cutover in version 1.9.0 (they're basically working on abandoning autotools in favor of bazel/cmake).

devel/googletest/Makefile
28

I was being unnecessarily conservative.

devel/googletest/pkg-plist
1

Installing gtest-config broke building from my GitHub checkout because it was picking up headers from /usr/local/include, instead of the relative directories.

I will see about making pkg-config gtest work.