Taking this up again, why could we just not roll back this change if it was ill advised?
Also, it may be me, but where do I find the official build documentation? Googling "building openjdk" gives me a number of hits, neither one seems official - the top hit seems to be one from Magnus' private cr directory :) but since it does not mention google test at all I don't think this is recent. Thanks, Thomas On Mon, Jun 1, 2020 at 6:01 PM Thomas Stüfe <thomas.stu...@gmail.com> wrote: > 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 >>>> >>> >>>> >> >>>> > >>>> >>>> >>> >>
