-
Notifications
You must be signed in to change notification settings - Fork 874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove downloader from NBI #7542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I want to Veto this. I understand why it was done, but what I'm missing is analysis of the change for downstream users of NBI and considering alternative approaches.
Given that the JDK has now an async capable http client, there should be a better option, that randomly kill functions without warning.
Edit: I'll have a look at this, but I can't promise how fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that indeed fixing NBI is riding a dead horse and I don't want to focus on it.
I pull my Veto.
Looked through the changes and they look good to me.
@matthiasblaesing tbh my first thought was also to try to reimplement it which I gave up on fairly quickly after going through the code. My second thought was to remove the time slicing scheduler and sequentialize the download to get rid of the suspend/resume at least. But since the threading model leaked through the API, this wouldn't really work (interrupts etc) - and testing this would have been a pain. Any modern downloader implementation would be trivial today but couldn't really be put behind that API. @neilcsmith-net did also take a closer look through NBI long ago and also decided that it was not worth it, this is what spawned https://github.com/apache/netbeans-nbpackage as you know. Last but not least: unmaintained net code isn't ideal, even if it would continue to work. We would keep releasing it even though all tests are disabled and nobody is looking at it anymore. |
This comment was marked as off-topic.
This comment was marked as off-topic.
I am planing to merge this relatively soon so that we can start building/testing on JDK 23. I tested this once more on win 10 (in conjunction with #7550) and saw no problems. |
will rebase and then merge.
edit: actually I believe the nb-javac PR is a precondition of the CI PR |
873fe3d
to
3f8bafe
Compare
lets wait with merge #7387 (comment) / #7553 |
- the download manager relied on suspend/resume/stop from java.lang.Thread which were deprecated since Java 1.2 - JDK 23 removes suspend/resume, others are already no-ops - fixing this would require a completely different approach, and since NBI is no longer maintained and in the process to be replaced by nbpackage this is th easiest option
3f8bafe
to
ed247c4
Compare
how to test:
this should create the windows installer:
which is now testable on windows (not testable on JDK 23ea since the native launcher can't parse ea java versions)
this PR would unblock #7525 (and #7484)
full test on JDK 23 and nb-javac 23 ran in #7538
related to #4952