Hi Magnus, On Mon, Jun 1, 2020 at 1:35 PM Magnus Ihse Bursie < magnus.ihse.bur...@oracle.com> wrote:
> > > 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". > I fear that we are more reliant on a specific version of googletest than is the case with standard libraries. Just a gut feeling, but the fact that we cannot use the latest googletest version is a strong indicator. So using the OS-provided version may be the wrong one, which may be a constant source of annoying build errors. 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 > > > Thanks, Thomas > 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 >>> >>> >>> >> >>> > >>> >>> >> >
