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

Fri May 29 05:04:18 UTC 2020

Hi Igor,

thank you for taking the time to answer my grumblings.

Yes, comparison with jtreg is a bit crooked - it is not needed to get a
valid build. But jtreg is also maintained in the official OpenJDK
repositories; I can clone codetools/jtreg and have the correct version.
With gtests, I have to know which version OpenJDK needs, which is not the
latest version, and have to download that from outside our repositories.
Getting it wrong version may yield me difficult to analyze build errors
(plattform zoo, handicapped C++ compilers etc). Also, updating to a new
gtest version will now involve a lot more communicatio (a version check in
configure would help with that).

But this is a minor complaint, I could live with that. I most dislike the
fact that I have to specify this option *every single time*, and that
omitting it is objectively wrong and may give me a false sense of security.
Omitting it gives no warning, but if my changes break gtests I will only
notice it hours later when jdk/submit results are back.

Yes, I can hide this behind an alias or script, but I think this is the
wrong way. You guys did such a good job in making the build dead easy:
first time, it tells me exactly which Debian packets to install, I always
loved that special touch. I specify boot jdk and maybe debug level and I am
done. In fact, the build is so easy that until recently I did not even know
we had a build documentation :) The new --with-gtest option is a jarring
break from that.

In my mind, gtest is in the same ballpark as the freetype library on
Windows. That has always been a bit annoying but that was only Windows.
Something you cannot rely the OS library mechanism to deliver but have to
download and place yourself and tell the build about it.

I wonder whether we can find a compromise somehow. Maybe something like
an agreed on structure and place for a "companion directory", containing
build dependencies like these. Its location could be by convention in a
clear relation to the OpenJDK sources (e.g. just beside it) and configure
would look for the libraries in there by default. Like this:

- openjdk-source
   - configure
   - Makefile
   - ..
- openjdk-builddeps
  - google-test
  - freetype
  - ..

This would also lend itself very well to a maintained repository of build
dependencies somewhere (not necessarily maintained by Oracle). We would
have similar ease of use as with platform libraries, which by default are
also expected in a standard place in the file system.

Thanks, Thomas

On Fri, May 29, 2020 at 5:20 AM Igor Ignatyev <igor.ignatyev at oracle.com>
wrote:

> Hi Thomas,
>
> I regret that this patch made you unhappier. I fully agree that all
> hotspot developers are to always build *and* run gtest tests, yet not all
> people who work on OpenJDK project are hotspot developers.
>
> I also believe that all OpenJDK developers are to run tests related to the
> area they are changing. The vast majority of such tests are jtreg tests.
> And jtreg, for better or worse, is a counterexample to your last paragraph
> -- it's an essential part of OpenJDK, but it doesn't come in a form of well
> established library, and none of known to me platforms have jtreg in their
> package manager, so you do have to manually download jtreg and specify its
> location one way or another. I understand that there is a difference b/w
> gtest and jtreg, as jtreg isn't really need at build time. However the
> proper way to run any of OpenJDK tests is by `make test` and for it to work
> you need to either executed configure w/ --with-jtreg or specify JT_HOME on
> each invocation of `make test`, where the latter is a preferred way. From
> that point of view, gtest and jtreg aren't different, you need to download
> both manually and specify their location at configure-time.
>
> with that being said, I truly understand the inconvenience it causes to
> you, yet given you need to download gtest only once and it's fairly easy to
> hide `with-gtest` behind a script or an alias like configure_openjdk='bash
> ./configure --with-gtest=$GTEST_HOME', I hope it won't cause problems for
> you and won't discourage you in anyways.
>
> Thanks,
> -- Igor
>
> On May 28, 2020, at 12:31 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
>
> 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
>> >>>
>> >>
>> >
>>
>>
>