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