RFR(M) : 8148244 : Finalize and integrate GTest implementation

Fri May 6 07:05:21 UTC 2016

Good catch with disabling gtest in the case of the "build jdk".

I would prefer if you introduced a new variable for it, something more 
obvious like BUILD_GTEST. Default it to true in spec.gmk.in and override 
that value to false in buildjdk-spec.gmk.in. (The TEST_IN_BUILD variable 
had a different purpose and is now dead, so please also remove it from 
buildjdk-spec.gmk.in). I would also prefer if you put the whole 
inclusion of CompileGtest.gmk in a conditional in CompileLibraries.gmk 
instead, so we avoid all of the evaluation of building it when disabled. 
Also, please don't forget to indent anything inside a conditional. Like 
this:

ifeq ($(BUILD_GTEST), true) include lib/CompileGtest.gmk endif

For completeness, there should probably also be a conditional of 
BUILD_GTEST like this in Main.gmk:

ifeq ($(BUILD_GTEST), true) test-image-hotspot-gtest:
+($(CD) $(HOTSPOT_TOPDIR)/make/test && $(MAKE) $(MAKE_ARGS) -f 
GtestImage.gmk) endif

/Erik

On 2016-05-05 16:58, Igor Ignatyev wrote:
> Hi,
>
> I’ve rebased the changes to the latest jdk9/hs and removed the comment from JvmMapfile.gmk. Also I’ve guarded building gtest w/ if TEST_IN_BUILD != false (http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.02/make/lib/CompileGtest.gmk.html), so we won’t build it when we compile build jdk in case of cross-compliation.
>
> new webrevs:
> http://cr.openjdk.java.net/~iignatyev/8148244-root/webrev.02
> http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.02/
>
> Thanks,
> — Igor
>
>
>> On May 2, 2016, at 3:55 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>
>> Even though the comment on line 155 in JvmMapfile.gmk is accurate I agree with removing it.
>>
>> Besides that the change looks good!
>> /Jesper
>>
>>
>> Den 2/5/16 kl. 14:04, skrev Erik Joelsson:
>>> Hello,
>>>
>>> Build changes look good to me, but I also wrote most of them, except for
>>> makefiles/lib/JvmMapfile.gmk line 155. I could not have written that. Please
>>> remove.
>>>
>>> The webrevs seem to be generated against jdk9/hs without the removal of the old
>>> build system. I would recommend making that merge and generate new webrevs since
>>> makefiles moved around quite a bit.
>>>
>>> /Erik
>>>
>>> On 2016-05-02 13:21, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.00
>>>>> 615 lines changed: 605 ins; 2 del; 8 mod;
>>>> http://cr.openjdk.java.net/~iignatyev/8148244-root-wo-gtest/webrev.00/
>>>>> 218 lines changed: 209 ins; 0 del; 9 mod;
>>>> http://cr.openjdk.java.net/~iignatyev/8148244-root/webrev.00/
>>>>> 32029 lines changed: 32018 ins; 0 del; 11 mod;
>>>> Hi all,
>>>>
>>>> could you please review this patch which delivers core of JEP 281: HotSpot C++
>>>> Unit-Test Framework[1]?
>>>> in more details, this patch
>>>>   - integrates sources of gtest-1.7.0[2] to /test/fmw/gtest (~32k LOC)
>>>>   - introduces TEST macros (/hotspot/test/native/unittest.hpp)
>>>>    - TEST — a basic unit test, doesn’t require an inited JVM
>>>>    - TEST_VM — a test which requires an inited JVM, but expected to leave JVM
>>>> in a valid state
>>>>    - TEST_OTHER_VM — a test which requires a clean inited JVM
>>>>    - TEST_VM_ASSERT/TEST_VM_ASSERT_MSG - tests to verify assert, “death”[3]
>>>> tests which require inited JVM
>>>>   - contains two tests as smoke tests for the project and examples. new tests
>>>> should be added to /hotspot/test/native/ using the same directories layout as
>>>> a corresponding product source files, and have prefix ‘test_’, e.g.
>>>> test/native/runtime/test_os.cpp w/ tests for runtime/test_os.cpp
>>>>   - updates makefiles to build/run tests : to run tests, one should use 'make
>>>> test TEST=hotspot_gtest’
>>>>   - adds hotspot_gtest to jprt testset
>>>>
>>>> more information on the project can be found in JEP 281[1].
>>>>
>>>> please be informed, that existing unit tests (aka internal tests) will be
>>>> ported into new framework later, this activity is tracking by JDK-8077965[4].
>>>>
>>>> PS This patch relies on JDK-8149591[5] which is under review[6].
>>>>
>>>> Thanks,
>>>> — Igor
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8148244
>>>> webrevs:
>>>>   root      : http://cr.openjdk.java.net/~iignatyev/8148244-root/webrev.00/
>>>>   w/o gtest :
>>>> http://cr.openjdk.java.net/~iignatyev/8148244-root-wo-gtest/webrev.00/
>>>>   hotspot   : http://cr.openjdk.java.net/~iignatyev/8148244-hotspot/webrev.00
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8047975
>>>> [2] https://github.com/google/googletest/releases/tag/release-1.7.0
>>>> [3]
>>>> https://github.com/google/googletest/blob/master/googletest/docs/V1_7_AdvancedGuide.md#death-tests
>>>>
>>>> [4] https://bugs.openjdk.java.net/browse/JDK-8077965
>>>> [5] https://bugs.openjdk.java.net/browse/JDK-8149591
>>>> [6] http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-April/022937.html