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