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

Fri May 29 03:20:17 UTC 2020

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 <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/ <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/ <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/ <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 <https://bugs.openjdk.java.net/browse/JDK-8245610>
> >>> webrevs:
> >>>  - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/ <http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/> 
> >>> (meaningful changes)
> >>>  - 
> >>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/ <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
> >>>
> >>
> >
> 