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).

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.

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