Hello,
On 2018-09-14 05:43, Robin Westberg wrote:
Hi Erik,
Thanks for the review!
On 10 Sep 2018, at 19:09, Erik Joelsson <erik.joels...@oracle.com> wrote:
Hello Robin,
Overall very nice work. A few things though:
The output dir compile_commands should use a dash instead to fit with current
patterns. That way the clean target will also be all dashes.
Fixed. I actually removed the clean target, as the output is located in a
subfolder of make-support, so probably don’t need a special one.
Main.gmk:888: I think you meant to add test-compile-commands here, which also
requires you to define that target.
Fixed, the tests are now invoked from a top-level target. I also added a second
test to ensure that the no-build-libraries property of this actually stays
intact.
This is ok, but the test will fail unless run from a clean output dir.
This should probably be noted in a comment at least.
In NativeCompilation.gmk:739: I don't see a reason for = assignment, so please
use :=. On the next line, please indent for continuation with 4 spaces.
Fixed.
In Launcher-jdk.pack.gmk, this SetupJdkExecutable can be rewritten to use the
EXTRA_OBJECT_FILES parameter (inherited from SetupNativeCompilation macro) for
UNPACKEXE_ZIPOBJS instead of adding them to LDFLAGS. Then the explicit
dependency declaration can go away completely.
Fixed.
In Awt2dLibraries.gmk, I think lines 733 and 735 needs to be fixed as well.
Using FindLib is generally a good idea for this. I suspect there may be more
such instances sprinkled around the makefiles.
Only fixed the last one, I think the first one is ok as is?
The first one is sort of OK, but it's a questionable construct as the
$(BUILD_LIBAWT_XAWT) variable may contain additional targets. We used to
do it that way but these days I prefer the more explicit and precise
FindLib. In this particular case you get a non needed dependency added
when running compile-commands with ENABLE_HEADLESS_ONLY=true, which will
not really affect anything so it doesn't really matter.
Otherwise this looks good now.
/Erik
Webrev (incremental):
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/
Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/
Testing: tier1, builds-tier5
Best regards,
Robin
/Erik
On 2018-09-10 06:36, Robin Westberg wrote:
Hi all,
Please review the following change that adds support for generating compile_commands.json
as a top-level make target. This is a popular format for describing how to compile object
files for a project, and is defined in [1]. This file can be used directly by IDEs such
as Visual Studio Code and CLion as well as "language servers" such as cquery
[2] and rtags [3].
While it’s currently possible to generate this file as part of a full build
(using tools such as Bear [4], or simply parsing .cmdline files), this change
aims to make it possible to generate the compile commands data without actually
compiling the object files. This means that it’s for example possible to start
using an IDE fairly quickly on freshly cloned source, instead of having to wait
for a full build to complete.
This was originally inspired by the work done in [5] by Mikael Gerdin and Erik
Joelsson, but has been through a few revisions since then. Erik has also
provided a lot of assistance with the current version, thanks Erik!
Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
Testing: tier1, builds-tier5
Best regards,
Robin
[1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
[2] https://github.com/cquery-project/cquery
[3] https://github.com/Andersbakken/rtags
[4] https://github.com/rizsotto/Bear
[5] http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html
