RFR(S) : 8245610 : remove in-tree copy on gtest

[Thomas Stüfe]

Hi,

I know the boat has sailed, I missed this one. But I am a bit unhappy about
this.

I always build with gtests; I think it makes no sense to not build with
gtest. Even if you do not want to run the gtests (and it makes sense to
always do since its a good first-base validity test), hotspot developers
should always build them since changes in the hotspot sources may break
hotspot gtests. hotspot source and gtests are a unity.

So if it makes no sense to not build gtest, I should not need a special
option to specify gtest location - I'd argue that the default case should
not require special options.

The JBS issue states that "it can be treated like any other external
dependencies" but this comparison does not hold - almost all other
dependencies come in the form of well established libraries with standard
headers and standard installation locations which can be coded as default
values. On a recent mainstream platform I do not have to specify any paths
to libraries to build OpenJDK. This is now different, I always have to
manually download gtests and specify gtest location. This is regrettable.

Thanks, Thomas


On Tue, May 26, 2020 at 2:27 PM Magnus Ihse Bursie <
magnus.ihse.bursie at oracle.com> wrote:

>
>
> On 2020-05-25 19:53, Igor Ignatyev wrote:
> > Hi Magnus,
> >
> >> On May 25, 2020, at 7:46 AM, Magnus Ihse Bursie
> >> <magnus.ihse.bursie at oracle.com
> >> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
> >>
> >> Looks basically good to me.
> > thanks for your review!
> >>
> >> We need to document the dependency on gtest, and how to retrieve it.
> >> I recommend you add a section next to the JTReg information (under
> >> the "## Running Tests" heading) in doc/building.md. I think you
> >> should include basically the same information as you did in your
> >> follow-up mail, that was informative and clear.
> > that's a good suggestion, I've added a small paragraph to 'Running
> > Tests' in doc/building.md and regenerated corresponding .html file --
> > http://cr.openjdk.java.net/~iignatyev/8245610/webrev.doc/
> > please let me know if you think something should be added or reworded.
> Looks great! Thank you.
>
> /Magnus
> >
> >>
> >> I assume the most suitable replacement for developers who are used to
> >> add a '--disable-hotspot-gtest' to e.g. a pre-definied jib
> >> configuration is to now use '--without-gtest'. This will need to be
> >> communicated, perhaps to both hotspot-dev and jdk-dev.
> > sure, after this gets integrated, I'll let both hotspot-dev and
> > jdk-dev aliases know which changes might be required to enable/disable
> > hotspot gtest tests compilation.
> >
> > Thanks.
> > -- Igor
> >
> >>
> >> /Magnus
> >>
> >> On 2020-05-22 20:12, Igor Ignatyev wrote:
> >>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/
> >>>> 132 lines changed: 80 ins; 36 del; 16 mod
> >>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/
> >>>> 57482 lines changed: 80 ins; 57385 del; 17 mod;
> >>> Hi all,
> >>>
> >>> could you please review this small (if you ignore removal part)
> >>> patch which removes in-tree copy of gtest (test/fmw/gtest) and
> >>> updates makefiles to use one provided thru an added configure option
> >>> `--with-gtest`? the previously used configure option
> >>> `--enable-hotspot-gtest` gets depricated.
> >>>
> >>> the patch also compiles gtest and gmock source code into a static
> >>> library so they can be compiled w/ all warnings disabled.
> >>>
> >>> from JBS:
> >>>> w/ JEP 381 (JDK-8241787 / JDK-8244224) being integrated, all
> >>>> compilers used by OpenJDK became supported by gtest out-of-box, so
> >>>> there is no need to have our copy of gtest framework (including
> >>>> gmock) within OpenJDK source tree. instead, it can be treated like
> >>>> any other external dependencies, and a pointer to the directory w/
> >>>> gtest code can be passed via configure options.
> >>>
> >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245610
> >>> webrevs:
> >>>  - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/
> >>> (meaningful changes)
> >>>  -
> >>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/
> >>> (all changes)
> >>> testing:
> >>> - gtest tests on {linux,windows,macosx}-x64;
> >>> - tier[1-5] builds which include but not limited to linux-aarch64,
> >>> linux-arm32, linux-x64-zero
> >>>
> >>> Thanks,
> >>> -- Igor
> >>>
> >>
> >
>
>