Bug 187562
| Summary: | Update devel/googletest to version 1.7.0 | ||
|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Jimmy Kelley <ljboiler> |
| Component: | Individual Port(s) | Assignee: | John Marino <marino> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | jbeich, marino, rleigh |
| Priority: | Normal | ||
| Version: | Latest | ||
| Hardware: | Any | ||
| OS: | Any | ||
| Bug Depends on: | |||
| Bug Blocks: | 192736 | ||
| Attachments: | |||
|
Description
Jimmy Kelley
2014-03-14 01:50:01 UTC
Responsible Changed From-To: freebsd-ports-bugs->clsung Over to maintainer (via the GNATS Auto Assign Tool) 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 *** Bug 187563 has been marked as a duplicate of this bug. *** Created attachment 145880 [details]
Update port to 1.7.0
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 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 (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. 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. Created attachment 148803 [details]
combined
Rebased.
Created attachment 148804 [details]
|poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=off (default)
Created attachment 148805 [details]
|poudriere testport -P| googlemock-1.7.0.log (9.3R i386) TEST=off (default)
Created attachment 148806 [details]
|poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=on
Created attachment 148807 [details]
|poudriere testport -P| googlemock-1.7.0.log (9.3R i386) TEST=on
Created attachment 148808 [details] combined - typo: TEST_USE vs. TEST_USES after ports r371280 - strip libs per stage-qa Comment on attachment 148806 [details] |poudriere testport -P| googletest-1.7.0.log (9.3R i386) TEST=on Actually tested with bug 192736 applied. 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.
(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 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. (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. (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. 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] Created attachment 149823 [details]
|poudriere testport -P| googlemock-1.7.0.log (10.1R i386) TEST=on
Created attachment 149824 [details]
|poudriere testport -P| googletest-1.7.0.log (10.1R i386) TEST=on
okay, yes, this is perfect. I'll commit it sometime tonight, thanks. 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 Great work, Jan. Thanks for everyone involved, and I trust these ports are in good hands. |
