Hi Erik & Magnus,

Thanks for the final reviews, pushed it with the suggested changes.

Best regards,
Robin

> On 4 Oct 2018, at 18:24, Erik Joelsson <erik.joels...@oracle.com> wrote:
> 
> 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