Skip to content
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

[NETBEANS-4328] Make sure JavaFX is recommended but not required on JDK 11+. #2135

Merged
merged 1 commit into from
May 22, 2020

Conversation

neilcsmith-net
Copy link
Member

This is a bare minimum fix to ensure that the user can opt out of nb-javac and JavaFX installation when enabling Java support via Open Project or import settings. This is particularly bad in import settings as it causes a continuous cascade of dialogs on first run. See mention in https://issues.apache.org/jira/browse/NETBEANS-3810

Merging this would (partially?) break the fix for https://issues.apache.org/jira/browse/NETBEANS-2439 in #1612 IMO that triggered what should have been a blocker for 11.3 though.

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 from me. The behavior (of requiring JavaFX) was introduced by NETBEANS-2439 half a year ago. It is too late to change it for 12.0 at least I cannot support regression in https://issues.apache.org/jira/browse/NETBEANS-2439 few days before 12.0 release.

The proper fix would, in my opinion, be to move the JavaFX and Java Frontend Application wizards to javafx cluster. Then only JavaFX related functionality would require downloading JavaFX. Again, it is too late to do that in 12.0 in my opinion.

@neilcsmith-net
Copy link
Member Author

@JaroslavTulach OK, so what is your desired solution for 12.0? We shouldn't make JavaFX non-optional, nor have the current situation that forces nb-javac installation alongside - nb-javac doesn't support Java 14. We've treated non-optional installation of either as a blocker in the past because it's counter to ASF policy, and this basically blocks the UI on first run.

@matthiasblaesing
Copy link
Contributor

An open question: why not let the user override with a checkbox "No I don't want that!"? I tried to get a quick look at the code, but I was mildly baffeld to see the method "isRequired", that interprets properties, that are named recommed and derives from these hard requirements.

It would be nice if @dukescript could chime in as the author of the offending commit.

@neilcsmith-net
Copy link
Member Author

@matthiasblaesing agreed, and it is also this use of isRequired for the UI that is forcing JavaFX and nb-javac installation in the scenarios outlined, away from wizard use. If the user is unwilling, or unable, to install both then it leads to the dialog cascade mentioned in NETBEANS-3810, effectively rendering the IDE unusable.

@matthiasblaesing
Copy link
Contributor

Ok - I rethought this. From my perspective the real problem is the incompatibility of nbjavac with JDK 14. The same will be true for 15 and onwards. It would be better to really only depend on FX if necessary, but that indeed might to much for now. We also have a hard dependency on an installed GPLv2ed javac and noone bothers, so ...

If the above is accepted, the real issue is, that feature on demand behaves different in the new project dialog, than in the activation dialog, that pops up, when you open a project:

New project dialog - feature activation (notice that only FX is required, nbjavac is not required)
new_project

Project open - feature activation (notice, that you can only install both or none)
project_open

So as a stop-gap measure, it might be easiest to ask the user at this point whether he/she wants to install only FX or also nbjavac?

@jlahoda
Copy link
Contributor

jlahoda commented May 14, 2020

If we can show a dialog there, that would seem like a good solution to me.

@neilcsmith-net
Copy link
Member Author

I don't think we can require JavaFX. It's fundamentally different to Java itself because we're downloading it, not relying on a system install - it needs to follow ASF rules on optional dependencies http://apache.org/legal/resolved#optional

Prior to 11.3, Java support worked without JavaFX, why require it now just to fix one feature? Read the description in https://issues.apache.org/jira/browse/NETBEANS-3810 of what happens in 11.3 vs 11.2 if the user cannot download JavaFX. It turned something that mostly worked to completely unusable.

@jlahoda
Copy link
Contributor

jlahoda commented May 14, 2020

Well, I'd say:
-we have already released a version with this. Given the release timeline, seems much safer to release once more with this, rather than (risk) introducing some regressions.
-there was already a proposal to fix this properly in the next release.
-I guess we can argue that, technically speaking, having the Java language support is optional; does not feel so bad given we are talking about one release.

As a side note, I never quite understood the real point behind system dependencies - we can say "NetBeans requires a JDK with JavaFX", and it will be OK. But we cannot say "NetBeans requires a JDK" and provide in some way the JavaFX - despite the latter is giving the user much more freedom in his/hers choices.

@neilcsmith-net
Copy link
Member Author

Yes, you don't have to convince me of the absurdity of the "system dependencies" argument - made it many times! 😄

I'm not sure it gets us out of the need to follow the rules that exist now.

It also doesn't get away from the user effectively being left to force quit the IDE if the download won't work during import settings. That seems a much bigger regression in 11.3. Can you replicate that incidentally?

@errael
Copy link
Contributor

errael commented May 15, 2020

So as a stop-gap measure, it might be easiest to ask the user at this point whether he/she wants to install only FX or also nbjavac?

Perhaps even easier, don't handle nbjavac at all in this dialog. But mention in the existing dialog that nb-javac could/should be installed manually in certain circumstances. Recall that there was a "bold suggestion" by Gj to require jdk14 for editing.

@dukescript
Copy link
Contributor

-1 to current diff. +1 to fixing Project open - feature activation dialog. Consider #2137 - which shall be the direction to solve the JavaFX download problem in the future.

@neilcsmith-net
Copy link
Member Author

@dukescript +1 to moving to JavaFX and to fixing feature activation, but I'm not sure we can achieve that for 12.0. If I'd noticed how #1612 was working at the time I'd have -1'd it - I'm still of the opinion it probably needs to be reverted for 12.0, and we live with < 11.2 behaviour. There are multiple causes of issue 2439, including moving from JDK 8 which this doesn't solve, and network / download problems, which this has made far worse. Feature on demand activation doesn't handle network problems gracefully at all, and there are various reports of download issues with these modules for all sorts of reasons (network errors, firewalls, etc.)

Try this, roughly as discussed on dev@ -

  • Switch off your network connection
  • Make sure you have lots of Java projects open in 11.3 (fairly normal)
  • Open 12.0-beta4 with a clean userdir and click Yes to import settings.
  • Figure out how to get out of it! 😄

Prior to 11.3 people had a broken wizard in this scenario, they now have a broken IDE (well, it's fixable if you really know how). The ASF policy issue is a blocker in my opinion, but that technical issue seems far worse for the end user.

@matthiasblaesing
Copy link
Contributor

So that other don't have to rebuild the test case. For every open java project the user will be prompted by the "NetBeans IDE Installer" (the autoupdate component) to install "JavaFX implementation for X" and "nb-javac" (This is on JDK 11 and should not happen). The dialog can be canceled, the project will then be marked broken. If download fails, an error dialog is presented, that allows to retry. If that is cancled, process goes on and project is also marked as broken. For the next project the dialog is presented again.

The issue for me is, that nbjavac is forced, although I'm running on JDK11. The only work around was this:

  • cancel loading on project open
  • close all projects
  • install JavaFX module
  • open project -> Java cluster is activated

What happens with the IDE if ergonomics is not present and the extra clusters is not installed? If that works, why don't we just remove ergonomic right now, if it is such a PITA?

@matthiasblaesing
Copy link
Contributor

If someone whats to look into this. The magic happens in org.netbeans.modules.ide.ergonomics.fod.FeatureProjectFactory.FeatureNonProject.FeatureOpenHook (ergonomics/ide.ergonomics/src/org/netbeans/modules/ide/ergonomics/fod/FeatureProjectFactory.java).

@neilcsmith-net
Copy link
Member Author

@matthiasblaesing removing ergonomics would be a bit drastic at this stage, although it's certainly caused a range of issues over the last few releases - needs some love from 12.1. I deliberately went for the minimal option first, but if we can't get consensus on that, it would be good to get consensus on something that addresses the problem.

There are certainly issues (aside from policy) with requiring a dependency and assuming an infallible network connection - that was a cause of NETBEANS-2439 in the first place - we've just made it worse for them. We have reports in JIRA from 11.3 complaining about this issue. I would expect more with 12.0 having a longer lifetime.

@neilcsmith-net
Copy link
Member Author

neilcsmith-net commented May 18, 2020

@matthiasblaesing btw, the better "workaround" is -

  • click No on import settings
  • use Tools / Plugins / Installed to enable Java SE
  • use Tools / Options and import settings from previous version
  • enjoy a usable IDE without having to rely on the network to download anything

@matthiasblaesing
Copy link
Contributor

This is even more broken than I thought. I'm on linux and I was asked to install "JavaFX for Windows" and just now "JavaFX for Mac OS X" - what a trainwreck.

@JaroslavTulach
Copy link

Probably a result of using the regexp in the list of recommended modules. That's not ideal, but (assuming the proper module gets installed at the end), it is just very low priority UI issue.

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw. guys if this is the only issue that holds release 12 off, then feel free to apply original Neil's fix and go on. E.g. don't pay attention to my objections.

@neilcsmith-net
Copy link
Member Author

@JaroslavTulach pushed a couple of minimal (and somewhat ugly) changes. This keeps the UI in the wizard mostly the same, but doesn't use the required status underneath so that cancellation or network failure don't break everything. Also stops the same dialog cascade at startup with nb-javac on JDK 8, and appears to fix https://issues.apache.org/jira/browse/NETBEANS-3732 as well.

This any better for you? Pending better fixing in 12.1 obviously! 😄

Aside - how does one exclude the extra cluster when running in the debugger from the IDE? It was really confusing me why JavaFX wasn't showing up when stepping through this for a moment!

@JaroslavTulach
Copy link

I usually use rm -rf nbbuild/netbeans/extra nbbuild/testuserdir before I start the debugger.

@neilcsmith-net
Copy link
Member Author

@JaroslavTulach ah, thanks - the kill it with fire approach! 😄 I was doing the second bit (testuserdir) but assuming there was some property I needed to use to control clusters.

@ebarboni
Copy link
Contributor

@JaroslavTulach thanks for allowing a kind of agreement.
@neilcsmith-net merge once ok for you please

@neilcsmith-net
Copy link
Member Author

@ebarboni OK, thanks, squashed locally and merging.

@neilcsmith-net neilcsmith-net merged commit a012c77 into apache:master May 22, 2020
@neilcsmith-net neilcsmith-net deleted the issue3810 branch October 17, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants