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.ignat...@oracle.com <mailto:igor.ignat...@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.stu...@gmail.com <mailto:thomas.stu...@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.bur...@oracle.com
<mailto:magnus.ihse.bur...@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.bur...@oracle.com
<mailto:magnus.ihse.bur...@oracle.com>
>> <mailto:magnus.ihse.bur...@oracle.com
<mailto:magnus.ihse.bur...@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
>>>
>>
>
