Re: [11u] RFR: 8210459, 8218807, 8223678: Support for creating Visual Studio Code projects
On Sat, Feb 22, 2020 at 12:08 AM Langer, Christoph wrote: > Hi, > > Thanks, Volker for backporting this to JDK11 and thanks Andrew for > reviewing it. > > > 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. > > Thanks for waiting, Andrew. I'll approve JDK-8210459 now in order to get > it pushed to unblock your work on JDK-8210459. > Thanks, pushed. > > > > 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.gmkTue Feb 12 08:40:43 2019 +0100 > > > +++ b/test/make/TestMakeBase.gmkMon 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. > > Hm, I think Volker's way of adding this trivial correction to the backport > of 8223678 is okay. But, of course a separate bug could also be a good way > to document this fix. Maybe we need a tiebreaker here? > No problem. I've created "JDK-8240073: Fix 'test-make' build target in 11u" for this issue now and sent out a RFR here: https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2020-February/002589.html Please take a look if possible :) > So I'll approve the series of backports now, except for JDK-8223678. > > Thanks, I've pushed 8210459 and 8218807 now and sent out a new RFR for "JDK-8223678: Add Visual Studio Code workspace generation support (for native code)" without the extra fix which has been moved to JDK-8240073: https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2020-February/002588.html Please take a look if possible :) Thank you and best regards, Volker > Cheers > Christoph > >
Re: [11u] RFR: 8210459, 8218807, 8223678: Support for creating Visual Studio Code projects
On Wed, Feb 19, 2020 at 6:14 AM Andrew Hughes wrote: > > > > 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. > I'm always torn between the two approaches here. On one side I completely understand and respect your position. On the other hand, I find it hard to judge and review backport changes which are only required as dependencies for other backports out of their context. And we don't have any good possibilities of somehow grouping such changes together logically. It also increases the latency considerably, because you can't post the second change for review before the first one has been approved and pushed. This is exactly for the reason you mention, that a reviewer will first have to find and apply the first change, before he can look at the second one. Having all the related changes in one RFR at least keeps the context together and allows a review to apply and review them one after another. Anyway, thanks for looking at JDK-8210459 :) > > > > 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. > Thanks. I've pushed JDK-8210459 now, after Christoph has approve it. > > > > 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 > >
RE: [11u] RFR: 8210459, 8218807, 8223678: Support for creating Visual Studio Code projects
Hi, Thanks, Volker for backporting this to JDK11 and thanks Andrew for reviewing it. > 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. Thanks for waiting, Andrew. I'll approve JDK-8210459 now in order to get it pushed to unblock your work on JDK-8210459. > > 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.gmkTue Feb 12 08:40:43 2019 +0100 > > +++ b/test/make/TestMakeBase.gmkMon 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. Hm, I think Volker's way of adding this trivial correction to the backport of 8223678 is okay. But, of course a separate bug could also be a good way to document this fix. Maybe we need a tiebreaker here? So I'll approve the series of backports now, except for JDK-8223678. Cheers Christoph
Re: [11u] RFR: 8210459, 8218807, 8223678: Support for creating Visual Studio Code projects
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.gmkTue Feb 12 08:40:43 2019 +0100 > +++ b/test/make/TestMakeBase.gmkMon 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
