[11u] RFR: 8210459, 8218807, 8223678: Support for creating Visual Studio Code projects

[Andrew Hughes]

On 30/12/2019 20:18, Volker Simonis wrote:
> Hi,
> I'd like to downport the support for Visual Studio Code project
> creation to 11u. I think we will have to support 11u for quite some
> years and it makes sense to have as good as possible tool support in
> 11u as well.

I just came across this proposal in the process of backporting
JDK-8232748: "Build static versions of certain JDK libraries". The only
hunk of that changeset which isn't a simple context difference is that
which applies to JdkNativeCompilation.gmk and the FindLib &
FindStaticLib rules it introduces.

I was going to just drop this hunk, as I didn't see any reason to
backport JDK-8210459, but then I saw your existing RFR referenced on the
bug. If JDK-8210459 is to be backported, I thus need this in place
before posting JDK-8232748 for review.

> 
> This mail is especially intended to get a review from the build team
> that the few changes to the patches are OK:
> 
> https://bugs.openjdk.java.net/browse/JDK-8210459
> http://cr.openjdk.java.net/~simonis/webrevs/2019/8210459/
> 
> https://bugs.openjdk.java.net/browse/JDK-8218807
> http://cr.openjdk.java.net/~simonis/webrevs/2019/8218807/
> 
> https://bugs.openjdk.java.net/browse/JDK-8223678
> http://cr.openjdk.java.net/~simonis/webrevs/2019/8223678/
> 
> The downport consists of three changes which are additive (i.e. later
> changes depend on earlier ones). That's why I decided to post just one
> RFR, because looking at a single change makes no sense without the
> corresponding context. The changes mostly apply cleanly except for
> very few places where we need manual adjustments because of changed
> context.

I'm just going to stick to reviewing JDK-8210459 here. I feel an RFR
should stick to just the one single change, which a reviewer can then
apply and test on the current state of the repository. Having three
changes in one RFR is quite overwhelming and the later changes are being
applied against a repository state which doesn't yet exist. I suspect
that may be one of the reasons this review has not been undertaken thus far.

> 
> These changes only touch make files and actually only add new features
> to the make system, so the normal build results shouldn't be affected.
> I've tested that the build still works on Linux and Windows and "make
> test-make" succeeds.
> 
> Following some details on the three changes:
> 
> 8210459: Add support for generating compile_commands.json
> 
> The initial change which creates the compile_commands.json file (i.e.
> "make compile_commands"). This file can be used for many tools (e.g.
> CLion IDE, clangd, etc..)
> Applies cleanly except for the following hunk, which has a different context:
> 
> ORIGINAL
> ========
>     CFLAGS_solaris := -KPIC, \
>      CFLAGS_macosx := -fPIC, \
>      DISABLED_WARNINGS_clang := format-nonliteral, \
> -    LDFLAGS := $(UNPACKEXE_ZIPOBJS) \
> -        $(LDFLAGS_JDKEXE) $(LDFLAGS_CXX_JDK) \
> +    LDFLAGS := $(LDFLAGS_JDKEXE) $(LDFLAGS_CXX_JDK) \
> 
> JDK11u
> ======
>     CFLAGS_linux := -fPIC, \
>      CFLAGS_solaris := -KPIC, \
>      CFLAGS_macosx := -fPIC, \
> -    LDFLAGS := $(UNPACKEXE_ZIPOBJS) \
> -        $(LDFLAGS_JDKEXE) $(LDFLAGS_CXX_JDK) \
> +    LDFLAGS := $(LDFLAGS_JDKEXE) $(LDFLAGS_CXX_JDK) \
> 

This looks fine. I compared your patch and the original changeset and
this is indeed the only difference, and only occurs because of the
additional disabled warning for Clang.

> 
> Finally, I've added an additional fix to this last change, whch fixes
> the make tests (i.e. "make test-make"). These tests are currently
> broken in jdk11u. They have been broken by JDK-8212028 (which has
> already been downported to jdk11u) and fixed in jdk 12 as part of
> "8210958: Rename "make run-test" to "make test"" (which hasn't been
> downported yet and probably won't be donwported at all). Because the
> fix is trivial (that's why it was done as part of 8210958 without an
> extra bug ID) and because I wanted to run "make test-make" in jdk11u
> as well to verify my changes, I've decided to downport this fix as
> part of 8223678:
> 
> diff -r 8599975f5b33 test/make/TestMakeBase.gmk
> --- a/test/make/TestMakeBase.gmk    Tue Feb 12 08:40:43 2019 +0100
> +++ b/test/make/TestMakeBase.gmk    Mon Dec 23 22:10:46 2019 +0100
>  KWBASE := APA=banan;GURKA=tomat;COUNT=1%202%203%204%205;SUM=1+2+3+4+5;MANY_WORDS=I
> have the best words.
> 
>  $(eval $(call ParseKeywordVariable, KWBASE, \
> -    KEYWORDS := APA GURKA SUM, \
> +    SINGLE_KEYWORDS := APA GURKA SUM, \
>      STRING_KEYWORDS := COUNT MANY_WORDS, \
>  ))
> 
> @@ -364,7 +376,7 @@
>  KWBASE_WEIRD := ;;APA=banan;;;GURKA=apelsin;APA=skansen;;
> 
>  $(eval $(call ParseKeywordVariable, KWBASE_WEIRD, \
> -    KEYWORDS := APA GURKA SUM, \
> +    SINGLE_KEYWORDS := APA GURKA SUM, \
>      STRING_KEYWORDS := COUNT, \
>  ))
> 

I agree it doesn't make sense to backport JDK-8210958, as that's a major
change to the test infrastructure. However, I think this warrants its
own 11u bug rather than being smuggled in under JDK-8223678. Otherwise,
you're just mirroring the same problem that was created by mixing this
in with JDK-8210958. An independent bug can describe the breakage and
link to the original issue that caused it.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222