You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2020/08/17 05:22:35 UTC
[GitHub] [netbeans] jtulach opened a new pull request #2317: Check pack200 and warn the user on JDK14+
jtulach opened a new pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317
Yesterday I was helping a relative to install NetBeans. He was mostly interested in [Python support](http://plugins.netbeans.org/plugin/61688/python). However when we downloaded the ZIP and installed the support without errors, code in editor was still without syntax highlighting. I'd already seen such situation, so I soon realized why. [Python support](http://plugins.netbeans.org/plugin/61688/python) cannot be installed on JDK14 due to missing `pack200`.
However the problem isn't easy to find and we shall not let poor NetBeans users to face such silent errors. Here is my PR that detects presence of `.pack.gz` files in an NBM and fails its validation.
The second part of the PR shall add a UI to allow one to select `pack200` from another JDK.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r482128369
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
I suggest to take the licensing discussions elsewhere off this PR. It is not intention of this PR to bundle `unpack200` executable. The goal of this PR is to:
* warn the user
* allow the user to provide alternative implementation of the executable
The assumption is that most of the developers find/install older JDK and use its executable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r482128369
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
I suggest to take the licensing discussions elsewhere off this PR. It is not intention of this PR to bundle `unpack200` executable. The goal of this PR is to:
* warn user
* allow the user to provide alternative implementation of the executable
The assumption is that most of the developers find/install older JDK and use its executable.
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
I suggest to take the licensing discussions elsewhere off this PR. It is not intention of this PR to bundle `unpack200` executable. The goal of this PR is to:
* warn user
* allow the user to provide alternative implementation of the executable
The assumption is that most of the developers find/install older JDK and use its executable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] matthiasblaesing commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
matthiasblaesing commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481982587
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
Whether it it is java or not - we can call it via CLI, and most probably that is the best idea, but it looks as if there is a java implementation:
https://github.com/pack200/pack200/blob/master/src/main/java/io/pack200/UnpackerImpl.java#L114-L117
https://github.com/pack200/pack200/blob/master/src/main/java/io/pack200/UnpackerImpl.java#L200
It was just a thought.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach merged pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach merged pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] jtulach commented on pull request #2317: Check pack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
jtulach commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-678600317
Sorry, I hadn't had time to look at this issue during week days. Yes ...
> @geertjanw mentioned this as possible inclusion for 12.1-beta3.
> I'm inclined to think that's a good idea to at least warn people for now.
...warning users is good idea.
> Can you rebase or otherwise resolve the merge conflict?
Feel free to merge f6f2455 where ever you need it. Bump the version in `platform/autoupdate.services/manifest.mf` up as needed.
> Is the second part intended here or in a separate PR?
I hope to move that part forward during this weekend.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] ebresie commented on pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
ebresie commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-907657264
For references, NETBEANS-2842 Using of deprecated pack200 tool in nbm packaging
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] matthiasblaesing commented on pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
matthiasblaesing commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-685626069
My question was intended to make it clear, that pack200 support comes at a cost and my last venture into autoupdate code showed, that noone cared about the code at least it was an uphill battle to get code reviews.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] jtulach commented on pull request #2317: Check pack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
jtulach commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-678730378
Sure. Apache plugin portal is safe.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r482892822
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
@JaroslavTulach in general agreed. Mentioned because this seems to lock the idea it's an external executable into the API? Whereas something that allowed specifying source and dest files might not - eg. alternative implementations for ModuleUpdate::unpack200 passed in to API?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] jlahoda commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
jlahoda commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481705658
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
FWIW the pack200 in JDK is not in Java, it is a C/C++ implementation. So seems likely it would be run as a command line tool if we included it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481639961
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/OperationContainerImpl.java
##########
@@ -530,4 +532,33 @@ public OperationType getType () {
}
private OperationType type;
private OperationContainer delegate;
+
+ /**
+ * @return the unpack200 executable or {@code null}
+ */
+ public final File getUnpack200() {
+ NO_PACK: if (unpack200 == null) {
+ final String jreHome = System.getProperty("java.home"); // NOI18N
+ if (jreHome == null) {
+ break NO_PACK;
+ }
Review comment:
`java.home` maybe be missing on some obscure JVMs. Bck2Brwsr doesn't have it. Native image was missing it at the beginning. I am usually trying to check for `null` after reading it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] jtulach commented on pull request #2317: Check pack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
jtulach commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-678734974
With the 473d461 change I can run on JDK14 and install the [python support](http://plugins.netbeans.org/plugin/61688/python). During validation we get a warning describing the missing `unpack200` problem:
We can open a file chooser and select an alternative `unpack200` executable. The system tries to find one in sibling JDKs assuming people have JDK14 next to other older JDKs:
Then the validation is restarted and continues with normal licensing checks:
in case of the python support a restart is needed. The upgrader picks the selected `unpack200` up and uses it to unpack the legacy NBMs:
The endeavor ends with support for Python files being ready in the editor:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r482033212
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
@matthiasblaesing yes, the mix of GPLv2 and GPLv2-CPE in the JDK is a problem (although it's possible OpenJ9 wouldn't be). I don't think you can separate that fact from considerations of linking vs aggregation. Relevance? Linking in pack200 or nb-javac might be possible given current discussions on GPL+CPE. Calling the CLI tool as @jlahoda suggests might not allow us to include it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #2317: Check pack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-678620614
@jtulach no problem. We've passed option to merge for 12.1 now so moved back to 12.2. Would have been nice to have, but I'm not sure it's critical - nothing via the plugin centre should be hitting this problem by now I hope!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #2317: Check pack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-675343852
OK, pushing back to 12.2.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] matthiasblaesing commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
matthiasblaesing commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481986195
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/OperationContainerImpl.java
##########
@@ -530,4 +532,33 @@ public OperationType getType () {
}
private OperationType type;
private OperationContainer delegate;
+
+ /**
+ * @return the unpack200 executable or {@code null}
+ */
+ public final File getUnpack200() {
+ NO_PACK: if (unpack200 == null) {
+ final String jreHome = System.getProperty("java.home"); // NOI18N
+ if (jreHome == null) {
+ break NO_PACK;
+ }
Review comment:
Ok I did not see the special runtimes - what remains is the break without a loop. I never saw that combination before, but I admit it is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] jtulach commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
jtulach commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r485310260
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/InstallSupportImpl.java
##########
@@ -1121,7 +1121,22 @@ private int verifyNbm (UpdateElement el, File nbmFile, ProgressHandle progress,
if(res == null) {
try {
- Collection<CodeSigner> nbmCerts = Utilities.getNbmCertificates(nbmFile);
+ List<String> pack200Entries = new ArrayList<>();
+ Collection<CodeSigner> nbmCerts = Utilities.getNbmCertificates(nbmFile, pack200Entries);
+ if (!pack200Entries.isEmpty()) {
+ OperationContainer<InstallSupport> operationContainer = support.getContainer();
+ OperationContainerImpl ocImpl = Trampoline.API.impl(operationContainer);
+ File unpack200 = ocImpl.getUnpack200();
+ if (unpack200 == null || !unpack200.canExecute()) {
+ StringBuilder sb = new StringBuilder();
+ for (String entry : pack200Entries) {
+ sb.append("\n").append(entry);
+ }
+ throw new OperationException(OperationException.ERROR_TYPE.MISSING_UNPACK200,
+ NbBundle.getMessage(InstallSupportImpl.class, "InstallSupportImpl_Validate_MissingUnpack200", nbmFile, sb.toString()) // NOI18N
+ );
+ }
+ }
Review comment:
Done in 80a6d71
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] matthiasblaesing commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
matthiasblaesing commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481321250
##########
File path: platform/autoupdate.services/src/org/netbeans/api/autoupdate/OperationContainer.java
##########
@@ -352,6 +353,17 @@ public void removeAll() {
impl.removeAll ();
}
+ /** Specifies location of unpack200 executable. {@code unpack200} has been
+ * removed from JDK 14. As such it is not possible to unpack older NBM
+ * files without providing alternative JDK implementation of this file.
+ *
+ * @param executable path to the executable
+ * @since 1.65
+ */
+ public final void setUnpack200(File executable) {
+ this.impl.setUnpack200(executable);
+ }
Review comment:
Maybe at some point in the future (no I don't want to wait for that moment), we might be able to ship a pack200 implementation (if I read the discussion on apache-legal correctly pure GPLv2-CPE might ok, the problem was, that the JDK is a mix of GPLv2 and GPLv2-CPE). If we can ship an implementation, would it make sense to do the decompression in process? For tar, zip and maybe other compression formats I would not consider to call the CLI version.
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/OperationContainerImpl.java
##########
@@ -530,4 +532,33 @@ public OperationType getType () {
}
private OperationType type;
private OperationContainer delegate;
+
+ /**
+ * @return the unpack200 executable or {@code null}
+ */
+ public final File getUnpack200() {
+ NO_PACK: if (unpack200 == null) {
+ final String jreHome = System.getProperty("java.home"); // NOI18N
+ if (jreHome == null) {
+ break NO_PACK;
+ }
Review comment:
This looks strange to me. I have never seen a break without a loop (is this java style `goto`)? Can `java.home` even be unset? I admit I don't know what jlink and/or graal static image returns here.
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/InstallSupportImpl.java
##########
@@ -1121,7 +1121,22 @@ private int verifyNbm (UpdateElement el, File nbmFile, ProgressHandle progress,
if(res == null) {
try {
- Collection<CodeSigner> nbmCerts = Utilities.getNbmCertificates(nbmFile);
+ List<String> pack200Entries = new ArrayList<>();
+ Collection<CodeSigner> nbmCerts = Utilities.getNbmCertificates(nbmFile, pack200Entries);
+ if (!pack200Entries.isEmpty()) {
+ OperationContainer<InstallSupport> operationContainer = support.getContainer();
+ OperationContainerImpl ocImpl = Trampoline.API.impl(operationContainer);
+ File unpack200 = ocImpl.getUnpack200();
+ if (unpack200 == null || !unpack200.canExecute()) {
+ StringBuilder sb = new StringBuilder();
+ for (String entry : pack200Entries) {
+ sb.append("\n").append(entry);
+ }
+ throw new OperationException(OperationException.ERROR_TYPE.MISSING_UNPACK200,
+ NbBundle.getMessage(InstallSupportImpl.class, "InstallSupportImpl_Validate_MissingUnpack200", nbmFile, sb.toString()) // NOI18N
+ );
+ }
+ }
Review comment:
This code path will not be hit when the download happens from a trusted source (See Plugin Configuration -> Settings -> Update Center Customizer -> "Trust update center fully and allow automatic installations").
The determination whether pack200 is required could be done seperatedly from the codesigner extraction. We are not on a hotpath, so scanning the ZIP/JAR/NBM twice should be ok IMHO.
##########
File path: platform/autoupdate.services/libsrc/org/netbeans/updater/ModuleUpdater.java
##########
@@ -579,6 +579,34 @@ private boolean unpack200(File src, File dest) {
return result == 0;
}
+ private File findUnpack200Executable(String unpack200) {
+ File unpack200Executable = new File(new File(System.getProperty("java.home"), "bin"), unpack200);
+ if (!unpack200Executable.canExecute()) {
+ for (File clusterRoot : UpdateTracking.clusters(true)) {
+ File uiConfig = new File(new File(new File(new File(new File(new File(new File(
+ clusterRoot, "config"), "Preferences"), "org"), "netbeans"), "modules"), // NOI18N
+ "autoupdate"), "services.properties"); // NOI18N
Review comment:
What is the meaning of this path?
For me this would be more readable:
```java
File uiConfig = new File(clusterRoot, "config/Preferences/org/netbeans/modules/autoupdate/services.properties");
```
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/Utilities.java
##########
@@ -289,13 +289,15 @@ private static int mapVerificationResultToInt(String input) {
* Get the certpaths that were used to sign the NBM content.
*
* @param nbmFile
+ * @param pack200Entries list of strings to add any entries in the NBM file
+ * that end with {@code .pack.gz} extension and may require pack200 tool
* @return collection of CodeSigners, that were used to sign the non-signature
* entries of the NBM
* @throws IOException
* @throws SecurityException if JAR was tampered with or if the certificate
* chains are not consistent
*/
- public static Collection<CodeSigner> getNbmCertificates (File nbmFile) throws IOException, SecurityException {
+ public static Collection<CodeSigner> getNbmCertificates (File nbmFile, List<String> pack200Entries) throws IOException, SecurityException {
Review comment:
I wrote it above. I'm not sure whether this mix of return (return via return statement and return via method parameter) is worth the saved file scanning.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481646072
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/InstallSupportImpl.java
##########
@@ -1121,7 +1121,22 @@ private int verifyNbm (UpdateElement el, File nbmFile, ProgressHandle progress,
if(res == null) {
try {
- Collection<CodeSigner> nbmCerts = Utilities.getNbmCertificates(nbmFile);
+ List<String> pack200Entries = new ArrayList<>();
+ Collection<CodeSigner> nbmCerts = Utilities.getNbmCertificates(nbmFile, pack200Entries);
+ if (!pack200Entries.isEmpty()) {
+ OperationContainer<InstallSupport> operationContainer = support.getContainer();
+ OperationContainerImpl ocImpl = Trampoline.API.impl(operationContainer);
+ File unpack200 = ocImpl.getUnpack200();
+ if (unpack200 == null || !unpack200.canExecute()) {
+ StringBuilder sb = new StringBuilder();
+ for (String entry : pack200Entries) {
+ sb.append("\n").append(entry);
+ }
+ throw new OperationException(OperationException.ERROR_TYPE.MISSING_UNPACK200,
+ NbBundle.getMessage(InstallSupportImpl.class, "InstallSupportImpl_Validate_MissingUnpack200", nbmFile, sb.toString()) // NOI18N
+ );
+ }
+ }
Review comment:
OK, I investigate further and see what can I do improve this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach commented on pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-689324092
There are two failures in the recent test run (both related to Java hints, a NPE or something). They must be unrelated to the autoupdate changes. I plan to press the merge button today. Thanks for all your reviews!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] matthiasblaesing commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
matthiasblaesing commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481980056
##########
File path: platform/autoupdate.services/libsrc/org/netbeans/updater/ModuleUpdater.java
##########
@@ -579,6 +579,34 @@ private boolean unpack200(File src, File dest) {
return result == 0;
}
+ private File findUnpack200Executable(String unpack200) {
+ File unpack200Executable = new File(new File(System.getProperty("java.home"), "bin"), unpack200);
+ if (!unpack200Executable.canExecute()) {
+ for (File clusterRoot : UpdateTracking.clusters(true)) {
+ File uiConfig = new File(new File(new File(new File(new File(new File(new File(
+ clusterRoot, "config"), "Preferences"), "org"), "netbeans"), "modules"), // NOI18N
+ "autoupdate"), "services.properties"); // NOI18N
Review comment:
Yes, windows accepts "/" and "\" as directory separator. For example calling:
```
dir "c:/Windows/System32/drivers/etc"
```
rum in a cmd window on windows will yield the directory contents of the denoted directory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #2317: Check pack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-674819941
@geertjanw mentioned this as possible inclusion for 12.1-beta3. I'm inclined to think that's a good idea to at least warn people for now. Is the second part intended here or in a separate PR? Can you rebase or otherwise resolve the merge conflict?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] jlahoda commented on pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
jlahoda commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-685303935
I agree with Jaroslav that we should try to keep the ability to install existing NBMs, if we can. After all, for C/C++, people may still want to install CND from NetBeans 8.2, And there are other plugins as well users may want to install.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] jtulach commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
jtulach commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r485310363
##########
File path: platform/autoupdate.services/src/org/netbeans/modules/autoupdate/services/Utilities.java
##########
@@ -289,13 +289,15 @@ private static int mapVerificationResultToInt(String input) {
* Get the certpaths that were used to sign the NBM content.
*
* @param nbmFile
+ * @param pack200Entries list of strings to add any entries in the NBM file
+ * that end with {@code .pack.gz} extension and may require pack200 tool
* @return collection of CodeSigners, that were used to sign the non-signature
* entries of the NBM
* @throws IOException
* @throws SecurityException if JAR was tampered with or if the certificate
* chains are not consistent
*/
- public static Collection<CodeSigner> getNbmCertificates (File nbmFile) throws IOException, SecurityException {
+ public static Collection<CodeSigner> getNbmCertificates (File nbmFile, List<String> pack200Entries) throws IOException, SecurityException {
Review comment:
Reverted back to original signature in 80a6d71
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
neilcsmith-net commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-689442530
@JaroslavTulach yes, those particular tests regularly fail and need retriggering constantly, so not a sign of issue here. Still not entirely sure on baking external executable into the API, but not against merging either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach commented on pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach commented on pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#issuecomment-685288456
> One final question:
The real question is: Do we want to support installation of ancient, but useful NBMs created in pre JDK-14 era? My answer is yes. As such I decided to work on this UI-related feature (something I don't enjoy much and I usually try to avoid).
> ... pack200 ... has no future
Alas, `pack200` has a place in NetBeans history - unlike other development teams, I acknowledge my past decisions and I am ready to maintain and support such decisions regardless of how uncomfortable it is. Especially when I see people suffering with my own eyes like in the case of using `pack200` [compressed NBMs](http://plugins.netbeans.org/plugin/61688/python) on JDK14+.
> Do we want to add an API for a new feature that is deprecated from the beginning?
The cost of supporting the `autoupdate.services` <-> updater <-> `autoupdate.ui` API isn't high. I am ready to maintain this part in the future, should there ever be a problem.
> My understanding was, that pack200 is removed from the
> JDK because the developers consider it obsolete and
My understanding is that JDK developers honor history of JDK way less then I do in case of NetBeans...
> because it has deep tie ins into the the classfile structure.
Whatever... when there is a will, there is a way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] JaroslavTulach commented on a change in pull request #2317: Check unpack200 and warn the user on JDK14+
Posted by GitBox <gi...@apache.org>.
JaroslavTulach commented on a change in pull request #2317:
URL: https://github.com/apache/netbeans/pull/2317#discussion_r481637617
##########
File path: platform/autoupdate.services/libsrc/org/netbeans/updater/ModuleUpdater.java
##########
@@ -579,6 +579,34 @@ private boolean unpack200(File src, File dest) {
return result == 0;
}
+ private File findUnpack200Executable(String unpack200) {
+ File unpack200Executable = new File(new File(System.getProperty("java.home"), "bin"), unpack200);
+ if (!unpack200Executable.canExecute()) {
+ for (File clusterRoot : UpdateTracking.clusters(true)) {
+ File uiConfig = new File(new File(new File(new File(new File(new File(new File(
+ clusterRoot, "config"), "Preferences"), "org"), "netbeans"), "modules"), // NOI18N
+ "autoupdate"), "services.properties"); // NOI18N
Review comment:
Does that work on Windows and other systems? `new File(new File(...` is portable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists