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