Looks good.

Minor style issue, the broken find line should be indented 4 spaces for continuation. Strictly speaking so should the awk program lines be in this case. No need for respin.

/Erik


On 2018-10-04 06:31, Magnus Ihse Bursie wrote:
4 okt. 2018 kl. 15:03 skrev Robin Westberg <robin.westb...@oracle.com>:

Hi Magnus,

Thanks again for reviewing!

On 4 Oct 2018, at 13:38, Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> 
wrote:

Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/
I was about to say that this looks good, but then I discovered a weird thing. I'm the SED 
line, line 50, there is a weird C-like Unicode character instead of a quote, after -e. 
Looks really odd. Is it a "smart quote" gone wrong? Can you look into it and 
fix it? You don't need to respin the webrev for that. Otherwise, you're all good to go.
Ah indeed, nice catch! Fortunately it seems to be an issue with the “git-webrev” 
tool, there’s a missing semicolon in the html’ized version: $(SED) -e 
&#391s/^/[. (The ‘raw’ view for example is correct).
Good.

I did notice one more thing that I was hoping to sneak into an in-place update: 
the raw ‘cat’ on line 47 should probably be $(CAT) as well, right? I’ll sanity 
check it with that change to be certain.
Yeah, it should. Shame on me and Erik for not catching it. ;-)

No need to respin.

/Magnus

Best regards,
Robin

/Magnus

Webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03-04/

Best regards,
Robin

Otherwise I think this is good now.

/Erik
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?
What it does it actually to invoke fixpath on everything inside the final 
compile_commands.json file, but in order to make fixpath do that, it presents 
the compile_commands.json file as an @-argument. Fixpath will then translate 
that argument to a temporary filename containing corrected paths, and that’s 
the file we save (since it’s deleted when the invoked command terminates). I’ve 
updated the comment a bit, hopefully it’s more clear now.

Another option would be to extend the fixpath utility itself to support some 
additional argument to just do inline path correction on a given file, but I 
felt that it would be a more risky change.

* In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r build*.log* 
compile_commands.json)" on a single line.
Fixed.

* In make/common/MakeBase.gmk: I'd prefer if these functions were move to 
make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib etc 
definitions.
Fixed.

* 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.
Sounds good, fixed.

/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.
Ah, good catch, I suspect it still works as the jawt.lib file in the 
modules_lib folder is dependent on jawt.lib in the native folder. Fixed.

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?
What I’ve been doing, apart from making sure tier1 tests and tier5-builds for 
all platforms still work, is to “diff” the .cmdline files from a build of “make 
jdk” with and without the patch applied (and with --with-version-opt= set 
during configure to avoid the timestamp). The only difference so far is that 
the EXTRA_OBJECT_FILES change for the make/launcher/Launcher-jdk.pack.gmk moves 
the files from the command line to an @file, but the contents are the same. 
(Had to apply a bit of sed to perform this verification after reordering the 
dependency flags, but still looks correct).

This obviously won’t catch any subtle dependency mistakes though, not sure if 
there’s much to be done about that though unfortunately.

Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.03/
Webrev (incremental): 
http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02-03/

Best regards,
Robin

/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/
Full: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.02/

Best regards,
Robin

/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

Reply via email to