Add jvm args support in run/debug tasks

[kawas44]

Update default nb-init-script and run/debug task windows to support jvm args definition.

Allow users to configure various jvm related properties of JavaExec gradle task from a single project property.

[kelemen]

Thanks, this looks good. I will give it a more through review and will probably merge it tomorrow.

[kawas44]

Hi kelemen, At the moment the code default to 128m of heap memory when no configuration is found. Null values will let the default values of JavaExec to be used.

If you agree I can make the changes in nb-init-script. By the way I think I'll remove debug messages. This may not be something we want to push to end users.

[kelemen]

Just wanted to ask the same things. Also, the system properties shouldn't be overwritten because the script might have set some default values for it. More importantly: If the user does not set anything, nothing should happen (i.e., as if this code in the init script was not there).

[kawas44]

Hi Kelemen,

I have worked on a new commit with no debug logs, with no update to javaExec if nothing has been configured but if a NB configuration is found it overwrites any previous configuration (ie: in gradle build file). Then NB debug jvmArgs are added to the set of existing jvm args.

I have done tests with no run/debug, with custom made run/debug and with application plugin. Everything seems to work as defined except for the application plugin and the use of the documented property "applicationDefaultJvmArgs".

In fact this property is taken into account but is not applied to JavaExec until execution (it is kind of lazy). So in debug mode, configured jvm args were overwritten by NB debug jvm args. I had to fix this by applying explicitly javaExec jvmArgs to itself.

Waiting for your feedback.

p.s: By the way the updateJvmArgs method that apply NB debug flag has an unused variable and seems complex for what it does. It may need a bit of refactoring.

[kawas44]

Looking at issues #313 , I have not done any test with Test task to see if jvmArgs are available in this task.

[kelemen]

I have fully reviewed what you wrote and only have the following concerns before merge:

• Why is the "application" plugin check in "updateDebugTaskArgs"? I don't think it should matter if the application plugin was applied (in this case).
• I think it is a bit surprising that you clear every property when the JVM arguments are explicitly specified. Though, I understand that this is probably the safest thing to do. I'm not 100% confident but I would prefer if those properties were only cleared if needed. That is, your original logic was mostly fine but it should not have removed already set properties and should have left the min-max memory untouched unless it had to (because of explicit specification in the JVM args).

updateJvmArgs tries to be as safe as possible (though what it does is probably not the best solution). I will only adjust this script after merging your changes (to make merging easier), so you can remove unused variables if you want to. To be honest, this is not the worst part :) The whole script has grown out of hand and probably needs a major refactoring. Maybe even have its own subproject which is bundled and extracted into .gradle.

[kawas44]

Hi Kelemen,

The answer of the 2nd point impact the 1st answer so I'll start with 2nd point.

• About your 2nd point, I think it is ambiguous to mix default configuration from build file and custom configuration in NB profiles. Even if there is a default way to launch the app for dev/qa platforms I usually define other way to launch the app for different purpose: local selenium test, custom Spring profile, etc. In those cases I don't assume that I have to complement what is already in the build file. I rather redefine my own configuration.

But as the maintainer of the project I let you choose the use case :)

• About your 1st point, as I previously wrote there is a strange behavior between the application plugin with jvmArgs defined via a documented property and the feature to just complement the jvmArgs line (ex: in case of NB debug jvmArgs complement). So I had to write the fix but did not know where, because the bug was related to debug I chose to put it in "updateDebugTaskArgs" :/

That said, if you wish to mix jvmArgs from build file, we will have to fix this behavior prior to applying any jvmArgs updates. It will certainly be in the new "updateJavaExec" function.

I'll wait for your reply to update the code accordingly.

[kelemen]

If there is a strange behaviour with the "application" plugin, why are you updating the property only in the case the "application" plugin is there? Also, as is now: When the debugee attaches to NB, your feature will be ignored. So, that is something which needs to be fixed. After this fix is done, I will merge your changes and will think about the other issue a little more. I think I understand the reason and benefit of your choice, my concern is that some system properties (or whatever arguments) might get calculated from the script and cannot be easily provided as constants in the IDE.

Regardless, once this is merged, I have decided that I will try to make this script saner and factor out most of the content into a separate subproject.

[kawas44]

Hi Kelemen,

There you have a new commit with a merge jvmargs policy. We also apply the fix for the application plugin as soon as we update jvmArg of the current run/debug task.

If you are ok with the pull request, would you like me to squash the commits of this branch before merge ?

[kelemen]

There is no need to squash, I prefer keeping the history (as what exactly was considered holds some value), I can see the squashed changes if I want to anyway.

Thank you for all your efforts.

ps.: I will try to have some major refactor of the whole build script this weekend or next week (though I have other things on my todo list as well).