On 2018-09-19 04:02, Magnus Ihse Bursie wrote:
19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie
<magnus.ihse.bur...@oracle.com <mailto:magnus.ihse.bur...@oracle.com>>:
On 2018-09-19 09:25, Robin Westberg wrote:
Hi Erik,
On 14 Sep 2018, at 19:21, Erik Joelsson <erik.joels...@oracle.com
<mailto:erik.joels...@oracle.com>> wrote:
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.
Yes, good point, I tried briefly to include a clean as a
prerequisite target to the test, but it refused to execute
sequentially. But perhaps it’s good enough with a comment, added one.
Yes, the clean target is treated specially before entering Main.gmk,
to ensure commands like "make clean all" gets executed correctly. A
comment is enough, I think.
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.
Yeah I certainly agree, but a quick grep shows that there’s about 50
such constructs present right now. I wouldn’t mind cleaning those
up, but perhaps that should be in a separate change?
If that does not affect your patch, you do not need to clean up those
constructs.
I've now looked through your patch. Overall, it looks good. Some
minor comments:
* In make/CompileCommands.gmk, are you sure the find -exec +
construct does not exceed command line limits on problematic
platforms such as Solaris and Windows?
The AWT constructs also confuses me. Maybe you can expand a bit on
the comment, because it really is non-trivial. You are executing a cp
for each tmpfile you find? But what if you find multiple tmpfiles?
There could certainly be multiple commands using @ constructs?
* In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r
build*.log* compile_commands.json)" on a single line.
* In make/common/MakeBase.gmk: I'd prefer if these functions were
move to make/common/JdkNativeCompilation.gmk, close to the
FindSrcDirsForLib etc definitions.
* In make/common/NativeCompilation.gmk, I'm not entirely happy with
the $1_OBJ_COMMON_OPTIONS_PRE/POST construct. I understand what you
are trying to achieve, but I think this gets very hard to read. I
don't have a perfect solution in mind. But perhaps something like this:
$1_DEPS_OPTIONS := $$($1_DEP_FLAG) $$($1_DEP).tmp
$1_COMPILE_COMMAND := $$($1_COMPILER) $$($1_FLAGS)
$$($1_DEPS_OPTIONS) $(CC_OUT_OPTION)$$($1_OBJ) $$($1_SRC_FILE))
and for the compile commands, use $$(filter-out $$($1_DEPS_OPTIONS),
$$($1_COMPILE_COMMAND)).
Perhaps some unification with the Microsoft toolchain is possible by
setting $1_DEPS_OPTIONS to -showIncludes.
An alternative, perhaps better, is to move the deps flag to the start.
Then you could do something like the above, but set
$1_COMPILE_OPTIONS := $$($1_FLAGS) $(CC_OUT_OPTION)$$($1_OBJ)
$$($1_SRC_FILE))
and when compiling do this:
$$($1_COMPILER) $$($1_DEPS_OPTIONS) $$($1_COMPILE_OPTIONS)
That would get rid of the filter-out, keep duplication low but still
be readable.
This assumes that ordering for the deps option is irrelevant. I think
it is but it would have to be tested.
I think that sounds good. The ordering of deps options should certainly
be irrelevant.
/Erik
/Magnus
Erik, what do you think?
* In make/lib/Lib-jdk.accessibility.gmk, this seems double incorrect.
You are missing a libjawt path segment. And you wanted to use
FindStaticLib.
Overall, I believe we're misusing the "static lib" concept on
Windows. The .lib files are not static libs, the way we mean it on
unixes. Instead, the lib files is some kind of metadata on dll that
are used when linking with those dlls. So we should introduce a
better concept for these, and maybe something like FindLibDescriptor
(or whatever). That should not really be part of this fix, though, so
for the moment I'm going to accept that we call these "static libs"
on Windows.
This also makes me wonder how much testing this patch has recieved? I
know a broken dependency in the worst case only shows up as a race,
but have you verified it thoroughly even on Windows? And even without
compile_commands?
/Magnus
Otherwise this looks good now.
Thanks, I’ll include the latest webrevs with a comment added:
Incremental:
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01-02/
<http://cr.openjdk.java.net/%7Erwestberg/8210459/webrev.01-02/>
Full: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02/
<http://cr.openjdk.java.net/%7Erwestberg/8210459/webrev.02/>
Best regards,
Robin
/Erik
Webrev (incremental):
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00-01/
<http://cr.openjdk.java.net/%7Erwestberg/8210459/webrev.00-01/>
Webrev (full):
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.01/
<http://cr.openjdk.java.net/%7Erwestberg/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/
<http://cr.openjdk.java.net/%7Erwestberg/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
