Looks basically good to me.

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.

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.

/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


Reply via email to