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 Ƈs/^/[. (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
