> 19 sep. 2018 kl. 09:51 skrev Magnus Ihse Bursie
> <magnus.ihse.bur...@oracle.com>:
>
>
>
>> On 2018-09-19 09:25, Robin Westberg wrote:
>> Hi Erik,
>>
>>> On 14 Sep 2018, at 19:21, Erik Joelsson <erik.joels...@oracle.com> wrote:
>>>
>>>>> Main.gmk:888: I think you meant to add test-compile-commands here, which
>>>>> also requires you to define that target.
>>>> Fixed, the tests are now invoked from a top-level target. I also added a
>>>> second test to ensure that the no-build-libraries property of this
>>>> actually stays intact.
>>> This is ok, but the test will fail unless run from a clean output dir. This
>>> should probably be noted in a comment at least.
>> Yes, good point, I tried briefly to include a clean as a prerequisite target
>> to the test, but it refused to execute sequentially. But perhaps it’s good
>> enough with a comment, added one.
>
> Yes, the clean target is treated specially before entering Main.gmk, to
> ensure commands like "make clean all" gets executed correctly. A comment is
> enough, I think.
>
>>
>>>>> 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?
>
> 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?
>
> * In make/Main.gmk, you can just do "($(CD) $(OUTPUTDIR) && $(RM) -r
> build*.log* compile_commands.json)" on a single line.
>
> * In make/common/MakeBase.gmk: I'd prefer if these functions were move to
> make/common/JdkNativeCompilation.gmk, close to the FindSrcDirsForLib etc
> definitions.
>
> * 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.
/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.
>
> 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?
>
> /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
>
