RFR(S) : 8245610 : remove in-tree copy on gtest
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Mon Jun 1 11:32:50 UTC 2020
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". 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
>
> 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 <mailto: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 <mailto: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
>> <mailto: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>
>> >> <mailto: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
>> >>>
>> >>
>> >
>>
>
More information about the build-dev
mailing list