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 2022/09/21 18:21:32 UTC
[GitHub] [netbeans] errael opened a new pull request, #4672: Avoid using an invalid JavaPlatform
errael opened a new pull request, #4672:
URL: https://github.com/apache/netbeans/pull/4672
This fixes #4651 in my case.
--
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] mbien commented on a diff in pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4672:
URL: https://github.com/apache/netbeans/pull/4672#discussion_r977032122
##########
java/spi.java.hints/src/org/netbeans/modules/java/hints/spiimpl/Utilities.java:
##########
@@ -1083,7 +1083,7 @@ public synchronized ClasspathInfo createUniversalCPInfo() {
final JavaPlatformManager man = JavaPlatformManager.getDefault();
if (select.getSpecification().getVersion() != null) {
for (JavaPlatform p : JavaPlatformManager.getDefault().getInstalledPlatforms()) {
- if (!"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
+ if (!p.isValid() || !"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
if (p.getSpecification().getVersion().compareTo(select.getSpecification().getVersion()) > 0) {
select = p;
}
Review Comment:
the fix makes sense. But I believe this whole section needs some work.
E.g if the default platform has no spec version, it would simply select the default platform without searching further?
how about:
```java
final JavaPlatformManager man = JavaPlatformManager.getDefault();
JavaPlatform select = Stream.of(man.getInstalledPlatforms())
.filter(JavaPlatform::isValid)
.filter((p) -> "j2se".equals(p.getSpecification().getName()))
.filter((p) -> p.getSpecification().getVersion() != null)
.max((p1, p2) -> p1.getSpecification().getVersion().compareTo(p2.getSpecification().getVersion()))
.orElse(JavaPlatform.getDefault());
```
edit: maybe we should even add a cap to not pick a version which goes beyond (nb-)javac. Getting the feature version of (nb-javac) is easy: `SourceVersion.latest().ordinal()`. The SpecificationVersion is just a bit limited at the moment.
```java
SpecificationVersion maxVersion = new SpecificationVersion(SourceVersion.latest().ordinal()+".99");
...
.filter((p) -> p.getSpecification().getVersion().compareTo(maxVersion) < 0)
```
this would filter out JDK 20 for example on master, but pick 19
--
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] mbien commented on a diff in pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4672:
URL: https://github.com/apache/netbeans/pull/4672#discussion_r977032122
##########
java/spi.java.hints/src/org/netbeans/modules/java/hints/spiimpl/Utilities.java:
##########
@@ -1083,7 +1083,7 @@ public synchronized ClasspathInfo createUniversalCPInfo() {
final JavaPlatformManager man = JavaPlatformManager.getDefault();
if (select.getSpecification().getVersion() != null) {
for (JavaPlatform p : JavaPlatformManager.getDefault().getInstalledPlatforms()) {
- if (!"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
+ if (!p.isValid() || !"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
if (p.getSpecification().getVersion().compareTo(select.getSpecification().getVersion()) > 0) {
select = p;
}
Review Comment:
the fix makes sense. But I believe this whole section needs some work.
E.g if the default platform has no spec version, it would simply select the default platform without searching further?
how about:
```java
final JavaPlatformManager man = JavaPlatformManager.getDefault();
JavaPlatform select = Stream.concat(Stream.of(man.getInstalledPlatforms()), Stream.of(JavaPlatform.getDefault()))
.filter(JavaPlatform::isValid)
.filter((p) -> "j2se".equals(p.getSpecification().getName()))
.filter((p) -> p.getSpecification().getVersion() != null)
.max((p1, p2) -> p1.getSpecification().getVersion().compareTo(p2.getSpecification().getVersion()))
.orElse(JavaPlatform.getDefault());
```
edit: maybe we should even add a cap to not pick a version which goes beyond (nb-)javac. Getting the feature version of (nb-javac) is easy: `SourceVersion.latest().ordinal()`. The SpecificationVersion is just a bit limited at the moment.
```java
SpecificationVersion maxVersion = new SpecificationVersion(SourceVersion.latest().ordinal()+".99");
...
.filter((p) -> p.getSpecification().getVersion().compareTo(maxVersion) < 0)
```
this would filter out JDK 20 for example on master, but pick 19
--
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] mbien commented on a diff in pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4672:
URL: https://github.com/apache/netbeans/pull/4672#discussion_r977032122
##########
java/spi.java.hints/src/org/netbeans/modules/java/hints/spiimpl/Utilities.java:
##########
@@ -1083,7 +1083,7 @@ public synchronized ClasspathInfo createUniversalCPInfo() {
final JavaPlatformManager man = JavaPlatformManager.getDefault();
if (select.getSpecification().getVersion() != null) {
for (JavaPlatform p : JavaPlatformManager.getDefault().getInstalledPlatforms()) {
- if (!"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
+ if (!p.isValid() || !"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
if (p.getSpecification().getVersion().compareTo(select.getSpecification().getVersion()) > 0) {
select = p;
}
Review Comment:
the fix makes sense. But I believe this whole section needs some work.
E.g if the default platform has no spec version, it would simply select the default platform without searching further?
how about:
```java
final JavaPlatformManager man = JavaPlatformManager.getDefault();
JavaPlatform select = Stream.concat(Stream.of(man.getInstalledPlatforms()), Stream.of(JavaPlatform.getDefault()))
.filter(JavaPlatform::isValid)
.filter((p) -> "j2se".equals(p.getSpecification().getName()))
.filter((p) -> p.getSpecification().getVersion() != null)
.max((p1, p2) -> p1.getSpecification().getVersion().compareTo(p2.getSpecification().getVersion()))
.orElse(JavaPlatform.getDefault());
```
edit: maybe we should even add a cap to not pick a version which goes beyond (nb-)javac. Getting the feature version of (nb-javac) is easy: `SourceVersion.latest().ordinal()`. The SpecificationVersion is just a bit limited at the moment.
```java
.filter((p) -> p.getSpecification().getVersion().compareTo(new SpecificationVersion(SourceVersion.latest().ordinal()+".99")) < 0)
```
this would filter out JDK 20 for example on master, but pick 19
--
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] mbien commented on a diff in pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4672:
URL: https://github.com/apache/netbeans/pull/4672#discussion_r977178060
##########
java/spi.java.hints/src/org/netbeans/modules/java/hints/spiimpl/Utilities.java:
##########
@@ -1083,7 +1083,7 @@ public synchronized ClasspathInfo createUniversalCPInfo() {
final JavaPlatformManager man = JavaPlatformManager.getDefault();
if (select.getSpecification().getVersion() != null) {
for (JavaPlatform p : JavaPlatformManager.getDefault().getInstalledPlatforms()) {
- if (!"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
+ if (!p.isValid() || !"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
if (p.getSpecification().getVersion().compareTo(select.getSpecification().getVersion()) > 0) {
select = p;
}
Review Comment:
> > f the default platform has no spec version, it would simply select the default platform without searching further?
>
> Yes, I see that; but is it wrong?
my inspector gadget senses tell me that the loop appears to be there to select the most recent JDK. The null check might have been added to prevent a NPE in the loop, I can't imagine that it is supposed to ignore all configured platforms if the platform NB was started on doesn't have a spec version for whatever reason - but I might be wrong.
> > not pick a version which goes beyond (nb-)javac
> What would be the implication here if jdk-20 was picked and nb-javac only supported 19?
Its an untested configuration and usually breaks things (netbeans expects having a javac of a certain version). NB also doesn't work very well if the runtime JDK is newer than nb-javac. In this case its better to uninstall nb-javac and hope for best. This situation would be similar. If nb-javac is uninstalled and NB would be started on JDK 20, `SourceVersion.latest().ordinal()` would return 20 and it would be selected. With nb-javac, it would cap at 19 which is probably safer.
> Once I new how to fix the hints for my usage, it became a puzzle. I tried to see if I could find the spot without a lot of debugger action. I got lucky. I'm OK with closing this PR if you want to play with it,
nono don't close - you did all the work. I can open a followup PR to add the upper bound check later.
> About the Stream.concat: isn't it required that the default platform be an installed platform?
the original code sets `JavaPlatform select = JavaPlatform.getDefault();` first. After that it checks if any of the `getInstalledPlatforms()` are better choices. The Stream picks the best installed platform first, and if there is none it falls back to default platform. Same result just other way around. Btw I might have edited the snipped while you replied - sorry for the confusion.
--
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] errael commented on a diff in pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
errael commented on code in PR #4672:
URL: https://github.com/apache/netbeans/pull/4672#discussion_r977147721
##########
java/spi.java.hints/src/org/netbeans/modules/java/hints/spiimpl/Utilities.java:
##########
@@ -1083,7 +1083,7 @@ public synchronized ClasspathInfo createUniversalCPInfo() {
final JavaPlatformManager man = JavaPlatformManager.getDefault();
if (select.getSpecification().getVersion() != null) {
for (JavaPlatform p : JavaPlatformManager.getDefault().getInstalledPlatforms()) {
- if (!"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
+ if (!p.isValid() || !"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
if (p.getSpecification().getVersion().compareTo(select.getSpecification().getVersion()) > 0) {
select = p;
}
Review Comment:
> f the default platform has no spec version, it would simply select the default platform without searching further?
Yes, I see that; but is it wrong?
I'm not familiar with this code. Your suggestion makes sense in principle; but I don't know, and can't readily discern, if there might be some significance to a default platform with no spec.version. Clearly the original author made a decision about how to handle this. I'm uncomfortable changing the behavior of code paths I don't grok.
> not pick a version which goes beyond (nb-)javac
What would be the implication here if jdk-20 was picked and nb-javac only supported 19?
About the `Stream.concat`: isn't it required that the default platform be an installed platform?
---
Once I new how to fix the hints for my usage, it became a puzzle. I tried to see if I could find the **spot** without a lot of debugger action. I got lucky. I'm OK with closing this PR if you want to play with it, and would certainly watch for any discussion/changes. But I'm not qualified, without lots of effort, to go beyond avoiding a !valid platform
BTW, thanks for the `Stream` implementation. I've done little work with them. The more I work through some examples the better.
--
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] mbien merged pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
mbien merged PR #4672:
URL: https://github.com/apache/netbeans/pull/4672
--
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] mbien commented on a diff in pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4672:
URL: https://github.com/apache/netbeans/pull/4672#discussion_r977032122
##########
java/spi.java.hints/src/org/netbeans/modules/java/hints/spiimpl/Utilities.java:
##########
@@ -1083,7 +1083,7 @@ public synchronized ClasspathInfo createUniversalCPInfo() {
final JavaPlatformManager man = JavaPlatformManager.getDefault();
if (select.getSpecification().getVersion() != null) {
for (JavaPlatform p : JavaPlatformManager.getDefault().getInstalledPlatforms()) {
- if (!"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
+ if (!p.isValid() || !"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
if (p.getSpecification().getVersion().compareTo(select.getSpecification().getVersion()) > 0) {
select = p;
}
Review Comment:
the fix makes sense. But I believe this whole section needs some work.
E.g if the default platform has no spec version, it would simply select the default platform without searching further?
how about:
```java
final JavaPlatformManager man = JavaPlatformManager.getDefault();
JavaPlatform select = Stream.concat(Stream.of(man.getInstalledPlatforms()), Stream.of(JavaPlatform.getDefault()))
.filter(JavaPlatform::isValid)
.filter((p) -> "j2se".equals(p.getSpecification().getName()))
.filter((p) -> p.getSpecification().getVersion() != null)
.max((p1, p2) -> p1.getSpecification().getVersion().compareTo(p2.getSpecification().getVersion()))
.orElse(JavaPlatform.getDefault());
```
edit: maybe we should even add a cap to not pick a version which goes beyond (nb-)javac. Getting the feature version of (nb-javac) is easy: `SourceVersion.latest().ordinal()`. The SpecificationVersion is just a bit limited at the moment.
```java
.filter((p) -> p.getSpecification().getVersion().compareTo(new SpecificationVersion(SourceVersion.latest().ordinal()+".99")) < 0)
```
--
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] mbien commented on a diff in pull request #4672: Avoid using an invalid JavaPlatform
Posted by GitBox <gi...@apache.org>.
mbien commented on code in PR #4672:
URL: https://github.com/apache/netbeans/pull/4672#discussion_r977032122
##########
java/spi.java.hints/src/org/netbeans/modules/java/hints/spiimpl/Utilities.java:
##########
@@ -1083,7 +1083,7 @@ public synchronized ClasspathInfo createUniversalCPInfo() {
final JavaPlatformManager man = JavaPlatformManager.getDefault();
if (select.getSpecification().getVersion() != null) {
for (JavaPlatform p : JavaPlatformManager.getDefault().getInstalledPlatforms()) {
- if (!"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
+ if (!p.isValid() || !"j2se".equals(p.getSpecification().getName()) || p.getSpecification().getVersion() == null) continue;
if (p.getSpecification().getVersion().compareTo(select.getSpecification().getVersion()) > 0) {
select = p;
}
Review Comment:
the fix makes sense. But I believe this whole section needs some work.
E.g if the default platform has no spec version, it would simply select the default platform without searching further?
how about:
```java
final JavaPlatformManager man = JavaPlatformManager.getDefault();
JavaPlatform select = Stream.concat(Stream.of(man.getInstalledPlatforms()), Stream.of(JavaPlatform.getDefault()))
.filter(JavaPlatform::isValid)
.filter((p) -> "j2se".equals(p.getSpecification().getName()))
.filter((p) -> p.getSpecification().getVersion() != null)
.max((p1, p2) -> p1.getSpecification().getVersion().compareTo(p2.getSpecification().getVersion()))
.orElse(JavaPlatform.getDefault());
```
--
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