> 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