Bug 187562 - Update devel/googletest to version 1.7.0
Summary: Update devel/googletest to version 1.7.0
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: John Marino
URL:
Keywords:
: 187563 (view as bug list)
Depends on:
Blocks: 192736
  Show dependency treegraph
 
Reported: 2014-03-14 01:50 UTC by Jimmy Kelley
Modified: 2014-11-25 17:41 UTC (History)
3 users (show)

See Also:


Attachments
patch.txt (2.35 KB, text/plain; charset=US-ASCII)
2014-03-14 02:00 UTC, Jimmy Kelley
no flags Details
combined (6.01 KB, patch)
2014-07-19 16:40 UTC, Jan Beich
no flags Details | Diff
Update port to 1.7.0 (5.54 KB, patch)
2014-08-16 20:24 UTC, Roger Leigh
no flags Details | Diff
combined (5.93 KB, patch)
2014-10-31 00:42 UTC, Jan Beich
no flags Details | Diff
|poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=off (default) (26.89 KB, text/plain)
2014-10-31 00:45 UTC, Jan Beich
no flags Details
|poudriere testport -P| googlemock-1.7.0.log (9.3R i386) TEST=off (default) (24.26 KB, text/plain)
2014-10-31 00:45 UTC, Jan Beich
no flags Details
|poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=on (145.95 KB, text/plain)
2014-10-31 00:57 UTC, Jan Beich
no flags Details
|poudriere testport -P| googlemock-1.7.0.log (9.3R i386) TEST=on (59.82 KB, text/plain)
2014-10-31 00:58 UTC, Jan Beich
no flags Details
combined (5.99 KB, patch)
2014-10-31 01:03 UTC, Jan Beich
no flags Details | Diff
diff: update using CMake (4.51 KB, patch)
2014-11-13 00:29 UTC, Pedro F. Giffuni
no flags Details | Diff
combined (13.76 KB, patch)
2014-11-25 13:47 UTC, Jan Beich
no flags Details | Diff
|poudriere testport -P| googlemock-1.7.0.log (10.1R i386) TEST=on (73.11 KB, text/plain)
2014-11-25 13:50 UTC, Jan Beich
no flags Details
|poudriere testport -P| googletest-1.7.0.log (10.1R i386) TEST=on (234.05 KB, text/plain)
2014-11-25 13:53 UTC, Jan Beich
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jimmy Kelley 2014-03-14 01:50:01 UTC
	GoogleTest version 1.7.0 is now available.

Fix: 

Patch attached.
Comment 1 Edwin Groothuis freebsd_committer 2014-03-14 03:51:10 UTC
Responsible Changed
From-To: freebsd-ports-bugs->clsung

Over to maintainer (via the GNATS Auto Assign Tool)
Comment 2 Jan Beich freebsd_committer 2014-07-19 16:40:36 UTC
Created attachment 144798 [details]
combined

Here's a recent patch prepared independently. It includes:
- update gtest/gmock to 1.7.0
- copy shebangfix and regression-test from gtest to gmock
- convert regression-test to TEST option for better integration
- make sure configure detects python2 to follow shebangfix
- fetch missing gmock_doctor.py from svn (fixed in r455)
- enable vendor make install again (revert r562)[1]
- add LICENSE=BSD3CLAUSE (idea from comment 0)

[1] One Definition Rule isn't really specific to system gtest. It
    causes issues for pretty much any software that bundles a library
    that is also often installed system-wide without properly renaming
    symbols/headers to avoid conflict. Here's a recent example for libpng
    https://lists.freebsd.org/pipermail/freebsd-chromium/2014-July/001395.html

In my case I need gtest at least 1.6.0 to fix the following error in openh264

  test/api/decoder_test.cpp:31:45: error: unknown template name 'WithParamInterface'
  class DecoderOutputTest : public ::testing::WithParamInterface<FileParam>,
                                              ^
  In file included from test/api/decoder_test.cpp:1:
  In file included from /usr/local/include/gtest/gtest.h:61:
  In file included from /usr/local/include/gtest/gtest-param-test.h:162:
  /usr/local/include/gtest/internal/gtest-param-util.h:448:30: error: no type named
        'ParamType' in 'DecoderOutputTest'
    typedef typename TestCase::ParamType ParamType;
            ~~~~~~~~~~~~~~~~~~~^~~~~~~~~
  test/api/decoder_test.cpp:53:1: note: in instantiation of template class
        'testing::internal::ParameterizedTestCaseInfo<DecoderOutputTest>' requested
        here
  TEST_P (DecoderOutputTest, CompareOutput) {
  ^
  /usr/local/include/gtest/gtest-param-test.h:1361:51: note: expanded from macro
        'TEST_P'
                #test_case_name, __FILE__, __LINE__)->AddTestPattern(\
                                                    ^
  test/api/decoder_test.cpp:54:17: error: use of undeclared identifier 'GetParam'
    FileParam p = GetParam();
                  ^
  test/api/decoder_test.cpp:99:1: error: no member named 'ParamType' in
        'DecoderOutputTest'
  INSTANTIATE_TEST_CASE_P (DecodeFile, DecoderOutputTest,
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /usr/local/include/gtest/gtest-param-test.h:1378:55: note: expanded from macro
        'INSTANTIATE_TEST_CASE_P'
    ::testing::internal::ParamGenerator<test_case_name::ParamType> \
                                        ~~~~~~~~~~~~~~~~^
  test/api/decoder_test.cpp:100:26: error: no viable conversion from
        'internal::ParamGenerator<FileParam>' to 'int'
                           ::testing::ValuesIn (kFileParamArray));
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /usr/local/include/gtest/gtest-param-test.h:1379:66: note: expanded from macro
        'INSTANTIATE_TEST_CASE_P'
        gtest_##prefix##test_case_name##_EvalGenerator_() { return generator; } \
                                                                   ^
  In file included from test/api/decoder_test.cpp:1:
  In file included from /usr/local/include/gtest/gtest.h:61:
  In file included from /usr/local/include/gtest/gtest-param-test.h:162:
  /usr/local/include/gtest/internal/gtest-param-util.h:595:34: error: reference to
        type 'const value_type' (aka 'testing::internal::ParameterizedTestCaseInfoBase
        *const') could not bind to an lvalue of type
        'ParameterizedTestCaseInfo<DecoderOutputTest> *'
        test_case_infos_.push_back(typed_test_info);
                                   ^~~~~~~~~~~~~~~
  test/api/decoder_test.cpp:53:1: note: in instantiation of function template
        specialization
        'testing::internal::ParameterizedTestCaseRegistry::GetTestCasePatternHolder<DecoderOutputTest>'
        requested here
  TEST_P (DecoderOutputTest, CompareOutput) {
  ^
  /usr/local/include/gtest/gtest-param-test.h:1360:11: note: expanded from macro
        'TEST_P'
            GetTestCasePatternHolder<test_case_name>(\
            ^
  /usr/include/c++/v1/vector:700:62: note: passing argument to parameter '__x' here
      _LIBCPP_INLINE_VISIBILITY void push_back(const_reference __x);
                                                               ^
  6 errors generated.

https://trillian.chruetertee.ch/freebsd-gecko/browser/trunk/multimedia/openh264
Comment 3 John Marino freebsd_committer 2014-07-28 08:33:44 UTC
*** Bug 187563 has been marked as a duplicate of this bug. ***
Comment 4 Roger Leigh 2014-08-16 20:24:41 UTC
Created attachment 145880 [details]
Update port to 1.7.0
Comment 5 Roger Leigh 2014-08-16 20:30:39 UTC
I prepared a patch before I realised the existence of bugzilla or this ticket and a preexisting patch.  However, you might still want to pull out of my patch the patches in files which

- enable death tests (didn't see that in the existing patches), also pushed upstream here: https://groups.google.com/forum/#!topic/googletestframework/VyEPBMcQG6I
- patch the automake files so that "make install" works as it should

I also didn't seem to need to change as much as the existing patch to download and unpack the zip file; maybe the underlying tools have improved in the interim to be more intelligent with zips?


Regards,
Roger Leigh
Comment 6 Jan Beich freebsd_committer 2014-08-17 04:31:25 UTC
Comment on attachment 145880 [details]
Update port to 1.7.0

(In reply to rleigh from comment #5)
> - enable death tests (didn't see that in the existing patches), also pushed
> upstream here:
> https://groups.google.com/forum/#!topic/googletestframework/VyEPBMcQG6I

That's not how upstream reviews patches:
https://code.google.com/p/googletest/wiki/DevGuide#Submitting_Patches

and during regression-test it produces

[WARNING] ./src/gtest-death-test.cc:825:: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test couldn't detect the number of threads.

due to clone() and GetThreadCount() not implemented.

> I also didn't seem to need to change as much as the existing patch to
> download and unpack the zip file; maybe the underlying tools have improved
> in the interim to be more intelligent with zips?

Which patch? Let's review yours (alas, no bugzilla splinter).

> +++ googletest/Makefile
> @@ -11,7 +11,7 @@
>  MAINTAINER=	clsung@FreeBSD.org
>  COMMENT=	Framework for writing C++ tests on a variety of platforms
> 
> -USES=		shebangfix libtool
> +USES=		zip libtool

shebangfix is required during regression-test to build/run test/fused_gtest_test without implicit lang/python dep

> +++ googletest/pkg-plist
> @@ -1,7 +1,7 @@
> -bin/gtest-config

gtest-config is required to build devel/googlemock against system googletest
Comment 7 Jan Beich freebsd_committer 2014-08-17 09:39:40 UTC
(In reply to Jan Beich from comment #6)
> [WARNING] ./src/gtest-death-test.cc:825:: Death tests use fork(), which is
> unsafe particularly in a threaded context. For this test, Google Test
> couldn't detect the number of threads.
> 
> due to clone() and GetThreadCount() not implemented.

It appears implementing GetThreadCount was enough, see logs in bug 192736.
Comment 8 Roger Leigh 2014-08-20 09:29:14 UTC
Jan, thanks for the clarifications.  I didn't realise submitting a patch upstream was such an unfriendly and convoluted process.

Regarding the shebangfix, it was breaking the build for me by appending (invalid) "python22" to all the python scripts' shebangs, making them unusable.  Not sure why.
Comment 9 Jan Beich freebsd_committer 2014-10-31 00:42:18 UTC
Created attachment 148803 [details]
combined

Rebased.
Comment 10 Jan Beich freebsd_committer 2014-10-31 00:45:10 UTC
Created attachment 148804 [details]
|poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=off (default)
Comment 11 Jan Beich freebsd_committer 2014-10-31 00:45:53 UTC
Created attachment 148805 [details]
|poudriere testport -P| googlemock-1.7.0.log (9.3R i386) TEST=off (default)
Comment 12 Jan Beich freebsd_committer 2014-10-31 00:57:46 UTC
Created attachment 148806 [details]
|poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=on
Comment 13 Jan Beich freebsd_committer 2014-10-31 00:58:43 UTC
Created attachment 148807 [details]
|poudriere testport -P| googlemock-1.7.0.log (9.3R i386) TEST=on
Comment 14 Jan Beich freebsd_committer 2014-10-31 01:03:10 UTC
Created attachment 148808 [details]
combined

- typo: TEST_USE vs. TEST_USES after ports r371280
- strip libs per stage-qa
Comment 15 Jan Beich freebsd_committer 2014-10-31 01:11:39 UTC
Comment on attachment 148806 [details]
|poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=on

Actually tested with bug 192736 applied.
Comment 16 Pedro F. Giffuni freebsd_committer 2014-11-13 00:29:08 UTC
Created attachment 149345 [details]
diff: update using CMake

FWIW, I did an update on my own before noticing this PR but unlike the existing approach I decided to use CMake as that is the supported method upstream, which basically means the installation is done manually.

I added a patch from 145880 to define the GTEST_OS_FREEBSD.

I think this port should be basically equivalent to the one on this PR but upstream is only building the static libraries now.

I will leave it up to the maintainer to decide which is the best way to update this.
Comment 17 Jan Beich freebsd_committer 2014-11-16 00:28:18 UTC
(In reply to Pedro F. Giffuni from comment #16)
> FWIW, I did an update on my own before noticing this PR

A behavioral pattern? ;)

> but unlike the existing approach I decided to use CMake

Improves buildtime almost twice here (20s vs. 10s), at least without tests. I like the idea but would prefer this done as a separate step/bug/commit to avoid regressions. There're already enough possible regressions just from update to 1.7.0 thanks to an upstream decision, see below.

> as that is the supported method upstream, which basically means the
> installation is done manually.

Citation needed for "supported method upstream". According to FAQ it's more of "integrate with your existing build system".

https://code.google.com/p/googletest/wiki/FAQ#Why_is_it_not_recommended_to_install_a_pre-compiled_copy_of_Goog

In comment 2 I've tried to disprove One-Definition Rule argument. For one, such cases can be fixed like bug 193187.

> 
> I added a patch from 145880 to define the GTEST_OS_FREEBSD.

It'd still make marino@ sad for ignoring DragonFly. Can be fixed by bug 192736.

> 
> I think this port should be basically equivalent to the one on this PR but
> upstream is only building the static libraries now.

From a quick glance:
- not installing .so libs would break devel/ros and multimedia/openh264 (TEST=on)
- gtest-config is no longer installed but required by devel/googlemock
- regression-test is lost but can be restored by building with 
  -Dgtest_build_tests=ON and running gmake test

> 
> I will leave it up to the maintainer to decide which is the best way to
> update this.

Do you mean @freebsd.org maintainers cannot time out ? clsung@ doesn't seem active for about a year.

http://freshbsd.org/search?committer=clsung&project=freebsd-ports
Comment 18 John Marino freebsd_committer 2014-11-25 07:09:56 UTC
I'm taking this PR over.

I would like all the PRs related to this port combined into 1 great patch and put here.

From what I read last night, cmake is required at least for the regression tests and possibly more.  From the little I know, I believe this port now requires cmake.
Comment 19 Jan Beich freebsd_committer 2014-11-25 08:42:11 UTC
(In reply to John Marino from comment #18)
> I'm taking this PR over.
> 
> I would like all the PRs related to this port combined into 1 great patch
> and put here.

Two ports after bug 187563 was made duplicate of this one:

  devel/googletest
  devel/googlemock

Don't squash unrelated fixes in bug 192042 and bug 192736 unless subversion sucks so much you can't do otherwise. This bug already carries enough weight to make bisecting harder.

> 
> From what I read last night, cmake is required at least for the regression
> tests and possibly more.  From the little I know, I believe this port now
> requires cmake.

See vendor README. And as attachment 148808 [details] demonstrates autotools-based build still works. It's ready to land now while cmake conversion can wait/land as followup fix.

  ### Legacy Build Scripts ###

  Before settling on CMake, we have been providing hand-maintained build
  projects/scripts for Visual Studio, Xcode, and Autotools.  While we
  continue to provide them for convenience, they are not actively
  maintained any more.  We highly recommend that you follow the
  instructions in the previous two sections to integrate Google Test
  with your existing build system.
Comment 20 John Marino freebsd_committer 2014-11-25 10:19:11 UTC
(In reply to Jan Beich from comment #19)
> Don't squash unrelated fixes in bug 192042 and bug 192736 unless subversion
> sucks so much you can't do otherwise. This bug already carries enough weight
> to make bisecting harder.

This port is becoming a nightmare for a committer like me.

Why do we need to bisect?
The basic desire is to move the port from version 1.5 to 1.7, so let's just do that in 1 go.  

I want one single tested patch that converts from the current state of the port to a perfect version 1.7.  I think you have all the knowledge necessary to do that.

I'd rather close all the other PRs and point to one, e.g. this one.  Help me help you.  I don't want to commit 3-4 changes, only one big one.
Comment 21 Jan Beich freebsd_committer 2014-11-25 13:47:11 UTC
Created attachment 149822 [details]
combined

OK, here's the version with other bugs squashed.

changes since last version:
- claim maintainership to followup on regressions
- fix bug 192042 [1]
- integrate bug 192736

overall changes (aka commit message):
- update devel/googletest and devel/googlemock to 1.7.0
- copy shebangfix and regression-test from devel/googletest to devel/googlemock
- convert regression-test to TEST option for better integration with poudriere
  and visibility for users
- make sure configure detects python2 to follow shebangfix
- add LICENSE=BSD3CLAUSE (idea from comment 0)
- strip libs per stage-qa
- enable pthreads by default in devel/googletest
- enable death tests in devel/googletest
- enable socket streaming in devel/googletest
- implement GetThreadCount for death tests in devel/googletest
- disable streaming tests that fail with old gcc on 9.x in devel/googletest
- depend on devel/googletest in devel/googlemock as -lgmock fails otherwise [1]
- pass maintainership to the persistent submitter [2]

PR:		ports/187562
PR:		ports/192736 [2]
PR:		ports/192042 [1]
Approved by:	maintainer timeout (~8 months)
Submitted by:	rakuco [1]
Submitted by:	Jan Beich <jbeich@vfemail.net> [2]
Comment 22 Jan Beich freebsd_committer 2014-11-25 13:50:28 UTC
Created attachment 149823 [details]
|poudriere testport -P| googlemock-1.7.0.log (10.1R i386) TEST=on
Comment 23 Jan Beich freebsd_committer 2014-11-25 13:53:29 UTC
Created attachment 149824 [details]
|poudriere testport -P| googletest-1.7.0.log (10.1R i386) TEST=on
Comment 24 John Marino freebsd_committer 2014-11-25 13:59:49 UTC
okay, yes, this is perfect.  I'll commit it sometime tonight, thanks.
Comment 25 commit-hook freebsd_committer 2014-11-25 17:38:52 UTC
A commit references this bug:

Author: marino
Date: Tue Nov 25 17:38:12 UTC 2014
New revision: 373421
URL: https://svnweb.freebsd.org/changeset/ports/373421

Log:
  devel/googlemock, devel/googletest: Upgrade version 1.5 => 1.7

  As part of the upgrade process:
    - copy shebangfix and regression-test from googletest to googlemock
    - convert regression-test to TEST option for better integration with
      poudriere and visibility for users
    - make sure configure detects python2 to follow shebangfix
    - add LICENSE=BSD3CLAUSE (idea from comment 0)
    - strip libs per stage-qa
    - enable pthreads by default in devel/googletest
    - enable death tests in devel/googletest
    - enable socket streaming in devel/googletest
    - implement GetThreadCount for death tests in devel/googletest
    - disable streaming tests that fail with old gcc on 9.x in googletest
    - depend on googletest in googlemock as -lgmock fails otherwise [1]
    - pass maintainership to the persistent submitter [2]

  PR:		187562
  PR:		192736 [2]
  PR:		192042 [1]
  Approved by:	maintainer timeout (~8 months)
  Submitted by:	rakuco [1]
  Submitted by:	Jan Beich <jbeich@vfemail.net> [2]

Changes:
  head/devel/googlemock/Makefile
  head/devel/googlemock/distinfo
  head/devel/googlemock/pkg-plist
  head/devel/googletest/Makefile
  head/devel/googletest/distinfo
  head/devel/googletest/files/patch-bsd-defines
  head/devel/googletest/files/patch-include_gtest_internal_gtest-port.h
  head/devel/googletest/pkg-plist
Comment 26 John Marino freebsd_committer 2014-11-25 17:41:34 UTC
Great work, Jan.  Thanks for everyone involved, and I trust these ports are in good hands.