Re: RFR: JEP 367: Remove the Pack200 Tools and API
Sounds good and looks good. /Erik On 2019-12-05 20:18, Henry Jen wrote: OK, so I created an issue[1] for follow up for Windows build and reverted the change in flags-cflags.m4, if nothing else, I’ll push without another webrev pinging that I get an +1 from someone in build-de, Erik? [1] https://bugs.openjdk.java.net/browse/JDK-8235461 Cheers, Henry On Dec 5, 2019, at 12:21 PM, Mandy Chung wrote: On 12/5/19 12:41 AM, Alan Bateman wrote: On 05/12/2019 08:16, Henry Jen wrote: Hi, Updated webrev[1] reflect comments since last webrev. Vicente had done all the heavy-lifting and hand over to me to finish up. Changes to symbols is reverted, as we expect that only need to be updated in next release by running make/scripts/generate-symbol-data.sh. The jar files are confusing in the webrev, but those files are removed. The whole test/jdk/tools/pack200 is removed. Cheers, Henry [1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/ The update webrev looks okay to me, except this part of the comment in flags-cflags.m4 "Now that unpack200 has been removed we should consider setting it for windows too. but this could be done as a follow-up effort. It could be that other other clients are relying on the current configuration for windows". I think it would be best to create an infrastructure/build issue and leave most of this confusing comment out. I also think keeping flags-cflags.m4 as is and file a new build issue as a follow-up would be better. Otherwise, this updated webrev looks okay to me. Mandy
Re: RFR: JEP 367: Remove the Pack200 Tools and API
OK, so I created an issue[1] for follow up for Windows build and reverted the change in flags-cflags.m4, if nothing else, I’ll push without another webrev pinging that I get an +1 from someone in build-de, Erik? [1] https://bugs.openjdk.java.net/browse/JDK-8235461 Cheers, Henry > On Dec 5, 2019, at 12:21 PM, Mandy Chung wrote: > > > > On 12/5/19 12:41 AM, Alan Bateman wrote: >> On 05/12/2019 08:16, Henry Jen wrote: >>> Hi, >>> >>> Updated webrev[1] reflect comments since last webrev. Vicente had done all >>> the heavy-lifting and hand over to me to finish up. >>> >>> Changes to symbols is reverted, as we expect that only need to be updated >>> in next release by running make/scripts/generate-symbol-data.sh. >>> >>> The jar files are confusing in the webrev, but those files are removed. The >>> whole test/jdk/tools/pack200 is removed. >>> >>> Cheers, >>> Henry >>> >>> [1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/ >>> >> The update webrev looks okay to me, except this part of the comment in >> flags-cflags.m4 >> >> "Now that unpack200 has been removed we should consider setting it for >> windows too. but this could be done as a follow-up effort. It could be that >> other other clients are relying on the current configuration for windows". >> >> I think it would be best to create an infrastructure/build issue and leave >> most of this confusing comment out. >> > > I also think keeping flags-cflags.m4 as is and file a new build issue as a > follow-up would be better. > > Otherwise, this updated webrev looks okay to me. > > Mandy
Re: RFR: JEP 367: Remove the Pack200 Tools and API
On 12/5/19 12:41 AM, Alan Bateman wrote: On 05/12/2019 08:16, Henry Jen wrote: Hi, Updated webrev[1] reflect comments since last webrev. Vicente had done all the heavy-lifting and hand over to me to finish up. Changes to symbols is reverted, as we expect that only need to be updated in next release by running make/scripts/generate-symbol-data.sh. The jar files are confusing in the webrev, but those files are removed. The whole test/jdk/tools/pack200 is removed. Cheers, Henry [1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/ The update webrev looks okay to me, except this part of the comment in flags-cflags.m4 "Now that unpack200 has been removed we should consider setting it for windows too. but this could be done as a follow-up effort. It could be that other other clients are relying on the current configuration for windows". I think it would be best to create an infrastructure/build issue and leave most of this confusing comment out. I also think keeping flags-cflags.m4 as is and file a new build issue as a follow-up would be better. Otherwise, this updated webrev looks okay to me. Mandy
Re: RFR: JEP 367: Remove the Pack200 Tools and API
On 05/12/2019 08:16, Henry Jen wrote: Hi, Updated webrev[1] reflect comments since last webrev. Vicente had done all the heavy-lifting and hand over to me to finish up. Changes to symbols is reverted, as we expect that only need to be updated in next release by running make/scripts/generate-symbol-data.sh. The jar files are confusing in the webrev, but those files are removed. The whole test/jdk/tools/pack200 is removed. Cheers, Henry [1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/ The update webrev looks okay to me, except this part of the comment in flags-cflags.m4 "Now that unpack200 has been removed we should consider setting it for windows too. but this could be done as a follow-up effort. It could be that other other clients are relying on the current configuration for windows". I think it would be best to create an infrastructure/build issue and leave most of this confusing comment out. -Alan.
Re: RFR: JEP 367: Remove the Pack200 Tools and API
Hi, Updated webrev[1] reflect comments since last webrev. Vicente had done all the heavy-lifting and hand over to me to finish up. Changes to symbols is reverted, as we expect that only need to be updated in next release by running make/scripts/generate-symbol-data.sh. The jar files are confusing in the webrev, but those files are removed. The whole test/jdk/tools/pack200 is removed. Cheers, Henry [1] http://cr.openjdk.java.net/~henryjen/jdk14/8234542/0/webrev/ > On Dec 2, 2019, at 6:50 PM, Joe Darcy wrote: > > Hi Vicente, > > It looks like the update to make/data/symbols/symbols removes the jdk.pack > module from the history JDKs 9, 10, and 11 when --release is used. > > If that is the case, it would be incorrect since historically the jdk.pack > module was present in those releases. > > Thanks, > > -Joe > > On 11/22/2019 1:30 PM, Vicente Romero wrote: >> Hi all, >> >> On 11/22/19 3:21 AM, Alan Bateman wrote: >>> >>> >>> On 21/11/2019 19:53, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] >>> Would it be possible to summarize what will remain in >>> test/jdk/tools/pack200 after this removal? The webrev makes it looks like >>> badattr.jar is being added but since it already exists then I'm not sure >>> whether to believe it. pack200-verifier/data/golden.jar is another one as >>> it looks like JAR file that is generated by the tests today is being >>> checked in, maybe `hg add` in error? >> >> I don't know why it is shown as added in the webrev, they have been removed. >> I have published another iteration of the webrev at [1] >>> >>> The change to flags-cflag.m4 to add LP64=1 on Windows will need eyes, it's >>> not immediately obvious to me which shared code compiled on Windows is >>> impacted by this. >> >> yes probably this change is risky. I did it as the comment suggested that >> the only reason the treatment for Windows was different was because of >> pack200. But not sure if we can trust that comment. Should I restore this >> code to its original state? >>> >>> -Alan >> >> >> Thanks, >> Vicente >> >> [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/
Re: RFR: JEP 367: Remove the Pack200 Tools and API
Hi Vicente, It looks like the update to make/data/symbols/symbols removes the jdk.pack module from the history JDKs 9, 10, and 11 when --release is used. If that is the case, it would be incorrect since historically the jdk.pack module was present in those releases. Thanks, -Joe On 11/22/2019 1:30 PM, Vicente Romero wrote: Hi all, On 11/22/19 3:21 AM, Alan Bateman wrote: On 21/11/2019 19:53, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Would it be possible to summarize what will remain in test/jdk/tools/pack200 after this removal? The webrev makes it looks like badattr.jar is being added but since it already exists then I'm not sure whether to believe it. pack200-verifier/data/golden.jar is another one as it looks like JAR file that is generated by the tests today is being checked in, maybe `hg add` in error? I don't know why it is shown as added in the webrev, they have been removed. I have published another iteration of the webrev at [1] The change to flags-cflag.m4 to add LP64=1 on Windows will need eyes, it's not immediately obvious to me which shared code compiled on Windows is impacted by this. yes probably this change is risky. I did it as the comment suggested that the only reason the treatment for Windows was different was because of pack200. But not sure if we can trust that comment. Should I restore this code to its original state? -Alan Thanks, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/
Re: RFR: JEP 367: Remove the Pack200 Tools and API
On 11/22/19 1:30 PM, Vicente Romero wrote: [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/ make/lib/Lib-jdk.pack.gmk should be removed. make/scripts/compare_exceptions.sh.incl Should ./lib/libunpack.so also be removed? make/scripts/compare.sh line 535-543 invokes unpack200. You should get the advice from the build team what to be done with this file (or file a JBS issue as a follow-up) Mandy
Re: RFR: JEP 367: Remove the Pack200 Tools and API
Hello, On 2019-11-22 13:30, Vicente Romero wrote: Hi all, On 11/22/19 3:21 AM, Alan Bateman wrote: On 21/11/2019 19:53, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Would it be possible to summarize what will remain in test/jdk/tools/pack200 after this removal? The webrev makes it looks like badattr.jar is being added but since it already exists then I'm not sure whether to believe it. pack200-verifier/data/golden.jar is another one as it looks like JAR file that is generated by the tests today is being checked in, maybe `hg add` in error? I don't know why it is shown as added in the webrev, they have been removed. I have published another iteration of the webrev at [1] The change to flags-cflag.m4 to add LP64=1 on Windows will need eyes, it's not immediately obvious to me which shared code compiled on Windows is impacted by this. yes probably this change is risky. I did it as the comment suggested that the only reason the treatment for Windows was different was because of pack200. But not sure if we can trust that comment. Should I restore this code to its original state? That comment is most likely originating from the build-infra rewrite in JDK 8. We probably added _LP64 on all platforms by mistake and noted that it caused a difference in the pack200 binary, so corrected the new build to mimic the old build. I wouldn't say just adding _LP64 on Windows is correct because of this. It may be correct regardless, or it may not. In make/scripts/compare.sh $UNPACK200 is still referenced. Should probably remove the whole section using it rather than just removing the string "jdk.pack". I would assume the *.pack and *.pack.gz files are long gone from any build. /Erik -Alan Thanks, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/
Re: RFR: JEP 367: Remove the Pack200 Tools and API
Hi all, On 11/22/19 3:21 AM, Alan Bateman wrote: On 21/11/2019 19:53, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Would it be possible to summarize what will remain in test/jdk/tools/pack200 after this removal? The webrev makes it looks like badattr.jar is being added but since it already exists then I'm not sure whether to believe it. pack200-verifier/data/golden.jar is another one as it looks like JAR file that is generated by the tests today is being checked in, maybe `hg add` in error? I don't know why it is shown as added in the webrev, they have been removed. I have published another iteration of the webrev at [1] The change to flags-cflag.m4 to add LP64=1 on Windows will need eyes, it's not immediately obvious to me which shared code compiled on Windows is impacted by this. yes probably this change is risky. I did it as the comment suggested that the only reason the treatment for Windows was different was because of pack200. But not sure if we can trust that comment. Should I restore this code to its original state? -Alan Thanks, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.03/
Re: RFR: JEP 367: Remove the Pack200 Tools and API
On 21/11/2019 19:53, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Would it be possible to summarize what will remain in test/jdk/tools/pack200 after this removal? The webrev makes it looks like badattr.jar is being added but since it already exists then I'm not sure whether to believe it. pack200-verifier/data/golden.jar is another one as it looks like JAR file that is generated by the tests today is being checked in, maybe `hg add` in error? The change to flags-cflag.m4 to add LP64=1 on Windows will need eyes, it's not immediately obvious to me which shared code compiled on Windows is impacted by this. -Alan
Re: RFR: JEP 367: Remove the Pack200 Tools and API
Hi Mandy, On 11/21/19 5:45 PM, Mandy Chung wrote: CSR needs to mention that jar -n option is removed. I made minor edit to the CSR to state that jdk.pack module is removed. thanks for the changes, I added a mention to the jar -n option, Mandy Thanks, Vicente On 11/21/19 2:22 PM, Vicente Romero wrote: please wait, I found some additional dependencies on module jdk.pack, will submit another webrev, sorry Vicente On 11/21/19 2:53 PM, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Thanks for all the comment so far, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8234596 On 11/20/19 8:21 PM, David Holmes wrote: Correction ... On 21/11/2019 9:10 am, David Holmes wrote: Hi Vicente, Not sure the best mailing list for this review ... jdk-dev may not be well monitored. Is there a separate review thread for the actual tool removal (jdk.pack) I overlooked the removal of jdk.pack (scrolling too fast through the webrev) - apologies. David - and build system changes? This removal seems okay, but I found one additional reference: ./src/utils/IdealGraphVisualizer/nbproject/project.properties:auxiliary.org-netbeans-modules-apisupport-installer.pack200-enabled=false Thanks, David - On 21/11/2019 8:54 am, Vicente Romero wrote: Hi, I need a reviewer for the changes to remove pack200 and unpack200 from the JDK. The webrev with the removal is at [1]. This patch is the "implementation" of JEP 367 [2]. The patch is basically removing the Pack200 related APIs plus its implementation plus any reference to it in other tools like `jar`. In the case of `jar`, Pack200 was only used if the `-n` flag was passed to the tool. I have removed the code that was executed when that flag was passed. I have also removed all the tests for Pack200. Thanks, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.00/ [2] https://bugs.openjdk.java.net/browse/JDK-8232022
Re: RFR: JEP 367: Remove the Pack200 Tools and API
CSR needs to mention that jar -n option is removed. I made minor edit to the CSR to state that jdk.pack module is removed. Mandy On 11/21/19 2:22 PM, Vicente Romero wrote: please wait, I found some additional dependencies on module jdk.pack, will submit another webrev, sorry Vicente On 11/21/19 2:53 PM, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Thanks for all the comment so far, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8234596 On 11/20/19 8:21 PM, David Holmes wrote: Correction ... On 21/11/2019 9:10 am, David Holmes wrote: Hi Vicente, Not sure the best mailing list for this review ... jdk-dev may not be well monitored. Is there a separate review thread for the actual tool removal (jdk.pack) I overlooked the removal of jdk.pack (scrolling too fast through the webrev) - apologies. David - and build system changes? This removal seems okay, but I found one additional reference: ./src/utils/IdealGraphVisualizer/nbproject/project.properties:auxiliary.org-netbeans-modules-apisupport-installer.pack200-enabled=false Thanks, David - On 21/11/2019 8:54 am, Vicente Romero wrote: Hi, I need a reviewer for the changes to remove pack200 and unpack200 from the JDK. The webrev with the removal is at [1]. This patch is the "implementation" of JEP 367 [2]. The patch is basically removing the Pack200 related APIs plus its implementation plus any reference to it in other tools like `jar`. In the case of `jar`, Pack200 was only used if the `-n` flag was passed to the tool. I have removed the code that was executed when that flag was passed. I have also removed all the tests for Pack200. Thanks, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.00/ [2] https://bugs.openjdk.java.net/browse/JDK-8232022
Re: RFR: JEP 367: Remove the Pack200 Tools and API
please wait, I found some additional dependencies on module jdk.pack, will submit another webrev, sorry Vicente On 11/21/19 2:53 PM, Vicente Romero wrote: Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Thanks for all the comment so far, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8234596 On 11/20/19 8:21 PM, David Holmes wrote: Correction ... On 21/11/2019 9:10 am, David Holmes wrote: Hi Vicente, Not sure the best mailing list for this review ... jdk-dev may not be well monitored. Is there a separate review thread for the actual tool removal (jdk.pack) I overlooked the removal of jdk.pack (scrolling too fast through the webrev) - apologies. David - and build system changes? This removal seems okay, but I found one additional reference: ./src/utils/IdealGraphVisualizer/nbproject/project.properties:auxiliary.org-netbeans-modules-apisupport-installer.pack200-enabled=false Thanks, David - On 21/11/2019 8:54 am, Vicente Romero wrote: Hi, I need a reviewer for the changes to remove pack200 and unpack200 from the JDK. The webrev with the removal is at [1]. This patch is the "implementation" of JEP 367 [2]. The patch is basically removing the Pack200 related APIs plus its implementation plus any reference to it in other tools like `jar`. In the case of `jar`, Pack200 was only used if the `-n` flag was passed to the tool. I have removed the code that was executed when that flag was passed. I have also removed all the tests for Pack200. Thanks, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.00/ [2] https://bugs.openjdk.java.net/browse/JDK-8232022
Re: RFR: JEP 367: Remove the Pack200 Tools and API
Hi, I think I have covered all the proposed fixes so far. This is the last iteration of the webrev [1], all the current changes are in this one, the code hasn't been split into different webrevs. I'm also forwarding to build-dev as there are some build related changes too. The CSR for this change is at [2] Thanks for all the comment so far, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8234596 On 11/20/19 8:21 PM, David Holmes wrote: Correction ... On 21/11/2019 9:10 am, David Holmes wrote: Hi Vicente, Not sure the best mailing list for this review ... jdk-dev may not be well monitored. Is there a separate review thread for the actual tool removal (jdk.pack) I overlooked the removal of jdk.pack (scrolling too fast through the webrev) - apologies. David - and build system changes? This removal seems okay, but I found one additional reference: ./src/utils/IdealGraphVisualizer/nbproject/project.properties:auxiliary.org-netbeans-modules-apisupport-installer.pack200-enabled=false Thanks, David - On 21/11/2019 8:54 am, Vicente Romero wrote: Hi, I need a reviewer for the changes to remove pack200 and unpack200 from the JDK. The webrev with the removal is at [1]. This patch is the "implementation" of JEP 367 [2]. The patch is basically removing the Pack200 related APIs plus its implementation plus any reference to it in other tools like `jar`. In the case of `jar`, Pack200 was only used if the `-n` flag was passed to the tool. I have removed the code that was executed when that flag was passed. I have also removed all the tests for Pack200. Thanks, Vicente [1] http://cr.openjdk.java.net/~vromero/8234542/webrev.00/ [2] https://bugs.openjdk.java.net/browse/JDK-8232022
