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> 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>> 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 > >>> > >> > > > >
