> 4 okt. 2018 kl. 10:33 skrev Robin Westberg <robin.westb...@oracle.com>:
> 
> Hi Erik,
> 
>> On 3 Oct 2018, at 20:51, Erik Joelsson <erik.joels...@oracle.com> wrote:
>> 
>> Hello Robin,
>> 
>> make/CompileCommands.gmk: typo in comment: “prepened"
> 
> Fixed. (Couldn’t resist a slight rewording so the text has reflowed a bit, 
> sorry!)

Much better written, thank you!

> 
>>> On 2018-10-03 11:09, Robin Westberg wrote:
>>> Hi Magnus,
>>> 
>>> Thanks for reviewing, sorry for taking a while to respond!
>>> 
>>>> On 19 Sep 2018, at 13:02, Magnus Ihse Bursie 
>>>> <magnus.ihse.bur...@oracle.com> wrote:
>>>> 
>>>> 19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie 
>>>> <magnus.ihse.bur...@oracle.com>:
>>>> 
>>>>>>>>> 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?
>>> It runs successfully on Windows and Solaris right now at least, and the 
>>> filenames only include a relative path, so should not be sensitive to 
>>> working directory location. But I have to admit I’m not sure how close to 
>>> the limit it is right now.. Perhaps I can build something around “xargs -s” 
>>> instead if you think this is risky?
>> I think it would be good to make sure the file list is split using xargs to 
>> avoid weird failures in the future. I also just realized it would be good to 
>> sort the file list to guarantee stable output.
> 
> Makes sense, changed it to use sort and xargs.

Much better, thanks!

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

/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