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

Mon Jun 8 06:56:39 UTC 2020

Taking this up again, why could we just not roll back this change if it was
ill advised?

Also, it may be me, but where do I find the official build documentation?
Googling "building openjdk" gives me a number of hits, neither one seems
official - the top hit seems to be one from Magnus' private cr directory :)
but since it does not mention google test at all I don't think this is
recent.

Thanks, Thomas

On Mon, Jun 1, 2020 at 6:01 PM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:

> Hi Magnus,
>
> On Mon, Jun 1, 2020 at 1:35 PM Magnus Ihse Bursie <
> magnus.ihse.bursie at oracle.com> wrote:
>
>>
>>
>> On 2020-05-29 07:04, Thomas Stüfe wrote:
>>
>> 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.
>>
>> I agree, Thomas. The patch went in too fast, without proper exploration
>> of alternatives, or how this would affect the usability of the build.
>>
>> One of the main goals of the build system has been that it should be easy
>> to build, and the system should be "self-instructing", so that if a
>> dependency is missing, this should be made clear, and a suitable suggestion
>> for correction should be printed.
>>
>> The gtest removal fails on both these accounts. :(
>>
>> I think we should change configure so that gtest is a required
>> dependency, unless specifically disabled. We can look  for gtest in
>> standard locations, like /usr/src/googletest, where it is installed by e.g.
>> "sudo apt install googletest".
>>
>
> I fear that we are more reliant on a specific version of googletest than
> is the case with standard libraries. Just a gut feeling, but the fact that
> we cannot use the latest googletest version is a strong indicator. So using
> the OS-provided version may be the wrong one, which may be a constant
> source of annoying build errors.
>
> Installation instructions, such as this apt command, must also be added.
>> (I'd appreciate feedback on what the package is called on RH/Fedora!) We
>> could also consider checking for an environment variable, similar to how
>> the boot JDK looks at JAVA_HOME, so you can set up a non-standard path in
>> your environment, and do not have to pass it to configure as a flag all the
>> time.
>>
>> And we also need to fix the RunTest framework, as René pointed out, so
>> that if you try to run gtests without the gtest library, you need to get a
>> proper error message that describes the step you need to take to be able to
>> run gtest tests.
>>
>> /Magnus
>>
>>
>>
> Thanks, Thomas
>
>
>> 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
>>>> >>>
>>>> >>
>>>> >
>>>>
>>>>
>>>
>>