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!) > 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. Webrev (full): http://cr.openjdk.java.net/~rwestberg/8210459/webrev.04/ 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
