Hi all, I agree with Thomas. I think it would be beneficial to have a fallback when there is no gtest directory explicitly set.
Also, running make target "run-test-gtest" now without "--with-gtest" results in a not so descriptive error message. make run-test-gtest Building target 'run-test-gtest' in configuration 'linux-x86_64-server-release' Unknown test selection: 'gtest' See doc/testing.[md|html] for help RunTests.gmk:539: *** Cannot continue. Stop. make[2]: *** [test-gtest] Error 1 I think it would be good to have the error message say that one tries to run the gtest suite without having the gtest sources set. The documentation under doc/testing.[md|html] doesn't describe the new behavior, so hotspot developers that missed this change might be a bit confused with this error message. Thanks, Rene On Fri, May 29, 2020 at 7:05 AM Thomas Stüfe <thomas.stu...@gmail.com> 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. > > 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> > 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> > > 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> 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 > >> >>> > >> >> > >> > > >> > >> > >
