Bug 206196 - SPI for JVM options passed to SE program or EE server
SPI for JVM options passed to SE program or EE server
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Generic Infrastructure
7.1
All All
: P2 (vote)
: 7.2
Assigned To: Petr Hejl
issues@projects
: API, API_REVIEW_FAST
Depends on: 107071 177340
Blocks: 203519
  Show dependency treegraph
 
Reported: 2011-12-09 15:29 UTC by Petr Hejl
Modified: 2012-06-06 18:10 UTC (History)
10 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
the api change (27.72 KB, patch)
2011-12-15 12:37 UTC, Petr Hejl
Details | Diff
the patch with VM mode (32.25 KB, patch)
2011-12-15 20:22 UTC, Petr Hejl
Details | Diff
the patch with instanceid and VM mode (32.52 KB, patch)
2011-12-16 14:34 UTC, J Bachorik
Details | Diff
profiler vm args provider prototype (4.37 KB, patch)
2011-12-16 14:40 UTC, J Bachorik
Details | Diff
the patch with instanceid and VM mode (81.93 KB, patch)
2011-12-16 17:13 UTC, J Bachorik
Details | Diff
updated patch (99.48 KB, patch)
2011-12-19 21:12 UTC, Petr Hejl
Details | Diff
updated patch (101.82 KB, patch)
2011-12-20 19:50 UTC, Petr Hejl
Details | Diff
updated patch (107.78 KB, patch)
2011-12-22 09:52 UTC, J Bachorik
Details | Diff
fixed module version (105.74 KB, patch)
2012-01-04 20:13 UTC, Petr Hejl
Details | Diff
lazy patch (112.88 KB, patch)
2012-01-06 22:15 UTC, Petr Hejl
Details | Diff
clean lazy patch with tests (126.63 KB, patch)
2012-01-09 13:15 UTC, Petr Hejl
Details | Diff
patch with singular and one more test (127.17 KB, patch)
2012-01-10 10:46 UTC, Petr Hejl
Details | Diff
patch with JG01 - JG05 fixed (127.49 KB, patch)
2012-01-13 12:53 UTC, Petr Hejl
Details | Diff
Complete testing plugin (4.92 KB, application/octet-stream)
2012-01-20 18:00 UTC, Jesse Glick
Details
Revised testing plugin (8.05 KB, application/octet-stream)
2012-01-21 00:15 UTC, Jesse Glick
Details
Current testing plugin (8.77 KB, application/octet-stream)
2012-02-01 17:21 UTC, Jesse Glick
Details
Sample plugin, uses JavaPlatform where available (9.69 KB, application/octet-stream)
2012-02-01 17:52 UTC, Jesse Glick
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Petr Hejl 2011-12-09 15:29:15 UTC
As requested by Anton Arhipov via email the server infrastructure should provide a way to enhance the JVM command line of server. This would allow third party plugins to hook in and to provide additional features (such as JRebel).

Suggested SPI could look like this (subject to change for now):

@VMArgsProvider
public class JRebelVMArgs implements VMArgsProvider {
   public List<String> getVMArgs(){
       return Arrays.asList("-javaagent:/path/to/jrebel.jar", "-Drebel.log=true", "-Drebel.log.trace=true");  //simplified
   }
}
Comment 1 Petr Hejl 2011-12-15 12:37:31 UTC
Created attachment 114223 [details]
the api change
Comment 2 Petr Hejl 2011-12-15 12:44:43 UTC
Please review.
Comment 3 J Bachorik 2011-12-15 13:13:05 UTC
JB01: This enhancement could easily replace the custom profiler startup when done right. Since all the server integration code uses "startup mode", in one form or another, I would recommend formalizing the startup mode enumeration and pass over the mode when asking the providers for extra JVM args. Then, each provider would be able to decide for which modes it will contribute the extra JVM args.
Comment 4 Petr Hejl 2011-12-15 20:22:44 UTC
Created attachment 114235 [details]
the patch with VM mode
Comment 5 Petr Hejl 2011-12-15 20:26:13 UTC
(In reply to comment #3)
> JB01: This enhancement could easily replace the custom profiler startup when
> done right. Since all the server integration code uses "startup mode", in one
> form or another, I would recommend formalizing the startup mode enumeration and
> pass over the mode when asking the providers for extra JVM args. Then, each
> provider would be able to decide for which modes it will contribute the extra
> JVM args.

Re JB01: The updated patch contains the change. Please could you enhance it with profiler implementation of VMArgumentsProvider as you suggested? This way we would assure the API fulfills the requirements for this use case. Thanks.
Comment 6 J Bachorik 2011-12-16 14:34:22 UTC
Created attachment 114267 [details]
the patch with instanceid and VM mode

For the purpose of generating proper profiler JVM args I need to know the instanceid of the server being started so I can take a look at eg. its registered platform and use os architecture for choosing the correct profiler agent binaries.
Comment 7 J Bachorik 2011-12-16 14:40:48 UTC
Created attachment 114268 [details]
profiler vm args provider prototype

The attached patch adds a new vm args provider generating profiler specific jvm arguments when a server instance is being started in profile mode.

I am attaching this patch separately because applying it will break the profiling functionality for application servers. The whole appserver<->profiler integration will need revisiting and cleanup once this API/SPI is ready.
Comment 8 Petr Hejl 2011-12-16 15:03:38 UTC
(In reply to comment #6)
> Created attachment 114267 [details]
> the patch with instanceid and VM mode
> 
> For the purpose of generating proper profiler JVM args I need to know the
> instanceid of the server being started so I can take a look at eg. its
> registered platform and use os architecture for choosing the correct profiler
> agent binaries.

This is what I've been afraid of. You need a serverInstanceId not just start mode. Note you are changing common server api. There is no such concept of instance id as this api is not restricted to Java or Java EE and has to be generic. I guess you could add a parameter with JavaPlatform, but definitely not any J2EE specific class or concept.

Does the patch actually work? I would rather see one patch - looks like that when your two patches are applied the profiler arguments would be added twice (old and new approach).
Comment 9 J Bachorik 2011-12-16 15:26:29 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > Created attachment 114267 [details]
> > the patch with instanceid and VM mode
> > 
> > For the purpose of generating proper profiler JVM args I need to know the
> > instanceid of the server being started so I can take a look at eg. its
> > registered platform and use os architecture for choosing the correct profiler
> > agent binaries.
> 
> This is what I've been afraid of. You need a serverInstanceId not just start
> mode. Note you are changing common server api. There is no such concept of
> instance id as this api is not restricted to Java or Java EE and has to be
> generic. I guess you could add a parameter with JavaPlatform, but definitely
> not any J2EE specific class or concept.
I was thinking about JavaPlatform but that class resides in the java cluster and and java cluster depends on ide cluster where the VMArgumentsProvider resides in turn, thus introducing a circular dependency.

What about passing a lookup instance for context? That way the J2EE specific implementation may directly insert JavaPlatform or J2eePlatform and profiler will use them.

> 
> Does the patch actually work? I would rather see one patch - looks like that
> when your two patches are applied the profiler arguments would be added twice
> (old and new approach).
As I am stating in the patch comment it kind-of works. It adds the proper jvm arguments but fails with duplicating them. I am not able to extract a working patch right now as the original profiler integration API was rather hackish (when compared to eg. debugger) due to the fact that profiler started as a separate pack and it will take significant effort to clean it up (it had been planned since 6.0 anyway ...) and the separate api change process for the profiler integration should not hold this SPI back.
Comment 10 Petr Hejl 2011-12-16 15:58:47 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > Created attachment 114267 [details]
> > > the patch with instanceid and VM mode
> > > 
> > > For the purpose of generating proper profiler JVM args I need to know the
> > > instanceid of the server being started so I can take a look at eg. its
> > > registered platform and use os architecture for choosing the correct profiler
> > > agent binaries.
> > 
> > This is what I've been afraid of. You need a serverInstanceId not just start
> > mode. Note you are changing common server api. There is no such concept of
> > instance id as this api is not restricted to Java or Java EE and has to be
> > generic. I guess you could add a parameter with JavaPlatform, but definitely
> > not any J2EE specific class or concept.
> I was thinking about JavaPlatform but that class resides in the java cluster
> and and java cluster depends on ide cluster where the VMArgumentsProvider
> resides in turn, thus introducing a circular dependency.
I see.
 
> What about passing a lookup instance for context? That way the J2EE specific
> implementation may directly insert JavaPlatform or J2eePlatform and profiler
> will use them.
Sounds bit like a "magic bag" pattern. I'll think about that. Can you confirm that the Java platform would be enough for profiler arguments integrated this way?
Comment 11 J Bachorik 2011-12-16 16:05:16 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #6)
> > > > Created attachment 114267 [details]
> > > > the patch with instanceid and VM mode
> > > > 
> > > > For the purpose of generating proper profiler JVM args I need to know the
> > > > instanceid of the server being started so I can take a look at eg. its
> > > > registered platform and use os architecture for choosing the correct profiler
> > > > agent binaries.
> > > 
> > > This is what I've been afraid of. You need a serverInstanceId not just start
> > > mode. Note you are changing common server api. There is no such concept of
> > > instance id as this api is not restricted to Java or Java EE and has to be
> > > generic. I guess you could add a parameter with JavaPlatform, but definitely
> > > not any J2EE specific class or concept.
> > I was thinking about JavaPlatform but that class resides in the java cluster
> > and and java cluster depends on ide cluster where the VMArgumentsProvider
> > resides in turn, thus introducing a circular dependency.
> I see.
> 
> > What about passing a lookup instance for context? That way the J2EE specific
> > implementation may directly insert JavaPlatform or J2eePlatform and profiler
> > will use them.
> Sounds bit like a "magic bag" pattern. I'll think about that. Can you confirm
> that the Java platform would be enough for profiler arguments integrated this
> way?
Yes, it will suffice. Since the agent arguments are jvm specific a JavaPlatform contains all the information necessary for correctly generating them.
Comment 12 J Bachorik 2011-12-16 17:11:45 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > Created attachment 114267 [details]
> > the patch with instanceid and VM mode
> > 
> > For the purpose of generating proper profiler JVM args I need to know the
> > instanceid of the server being started so I can take a look at eg. its
> > registered platform and use os architecture for choosing the correct profiler
> > agent binaries.
> 
> This is what I've been afraid of. You need a serverInstanceId not just start
> mode. Note you are changing common server api. There is no such concept of
> instance id as this api is not restricted to Java or Java EE and has to be
> generic. I guess you could add a parameter with JavaPlatform, but definitely
> not any J2EE specific class or concept.
> 
> Does the patch actually work? I would rather see one patch - looks like that
> when your two patches are applied the profiler arguments would be added twice
> (old and new approach).

I have a new patch ready which completely replaces the arbitrary profiler calls with the VMArgumentsProvider and it does work just fine (when I keep my one eye closed with regards to the not-so-nice profiler integration api itself :)

Now, the only thing left is to decide whether it is feasible to pass JavaPlatform directly, use Lookup or any other mean.
Comment 13 J Bachorik 2011-12-16 17:13:02 UTC
Created attachment 114271 [details]
the patch with instanceid and VM mode

Working profiler replacement. Still keeping the instanceid since nothing better has been decided yet.
Comment 14 Petr Hejl 2011-12-16 17:31:14 UTC
(In reply to comment #13)
> Created attachment 114271 [details]
> the patch with instanceid and VM mode
> 
> Working profiler replacement. Still keeping the instanceid since nothing better
> has been decided yet.

What about using SpecificationVersion of JavaPlatform, but not the JavaPlatform itself?
Comment 15 J Bachorik 2011-12-16 17:43:11 UTC
(In reply to comment #14)
> (In reply to comment #13)
> > Created attachment 114271 [details]
> > the patch with instanceid and VM mode
> > 
> > Working profiler replacement. Still keeping the instanceid since nothing better
> > has been decided yet.
> 
> What about using SpecificationVersion of JavaPlatform, but not the JavaPlatform
> itself?

That would be one part only. Unfortunately also the architecture (eg. x86,amd64) is required. A map of server instance properties might help but it would be the same "magic bag pattern" as lookup, having the additional disadvantage of being weakly typed.
Comment 16 J Bachorik 2011-12-16 18:03:10 UTC
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Created attachment 114271 [details]
> > > the patch with instanceid and VM mode
> > > 
> > > Working profiler replacement. Still keeping the instanceid since nothing better
> > > has been decided yet.
> > 
> > What about using SpecificationVersion of JavaPlatform, but not the JavaPlatform
> > itself?
> 
> That would be one part only. Unfortunately also the architecture (eg.
> x86,amd64) is required. A map of server instance properties might help but it
> would be the same "magic bag pattern" as lookup, having the additional
> disadvantage of being weakly typed.

Ok, I see one possibility. Let's use ServerInstance - a ServerInstanceProvider will then have to take care to make the JavaPlatform related information available through the lookup of ServerInstance.getFullNode().
The presence of eg. JavaPlatform in the ServerInstance.getFullNode() lookup will form a contract only between the profiler and the instance provider; the common server api will not be involved at all.
Comment 17 Petr Hejl 2011-12-16 19:34:58 UTC
(In reply to comment #16)
> Ok, I see one possibility. Let's use ServerInstance - a ServerInstanceProvider
> will then have to take care to make the JavaPlatform related information
> available through the lookup of ServerInstance.getFullNode().
> The presence of eg. JavaPlatform in the ServerInstance.getFullNode() lookup
> will form a contract only between the profiler and the instance provider; the
> common server api will not be involved at all.
getFullNode() is UI thing so I would like to avoid that. But I'm thinking of getLookup() method directly on ServerInstance and passing the ServerInstance as a method argument.
Comment 18 J Bachorik 2011-12-17 10:28:01 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > Ok, I see one possibility. Let's use ServerInstance - a ServerInstanceProvider
> > will then have to take care to make the JavaPlatform related information
> > available through the lookup of ServerInstance.getFullNode().
> > The presence of eg. JavaPlatform in the ServerInstance.getFullNode() lookup
> > will form a contract only between the profiler and the instance provider; the
> > common server api will not be involved at all.
> getFullNode() is UI thing so I would like to avoid that. But I'm thinking of
> getLookup() method directly on ServerInstance and passing the ServerInstance as
> a method argument.

JB02: Wouldn't this mean introducing an incompatible change? I would back up even an incompatible change as it would scratch my itch (and make the API more ready for future changes) but then you should remove API_REVIEW_FAST, wouldn't you?
Comment 19 Petr Hejl 2011-12-19 21:12:03 UTC
Created attachment 114326 [details]
updated patch

The patch with ServerInstance passed and ServerInstance.getLookup(). There are two missing things:
- the ProfilerArgsProvider does not do anything useful for now
- the GFv3 does not implement ServerInstanceImplementation2 and its lookup does not contain neither JavaPlatform nor J2eePlatform

The pure Java EE servers has J2eePlaform itself and content of J2eePlatform.getLookup() in ServerInstance.getLookup() so I guess ProfilerArgsProvider would be straightforward.

So one solution would be to somehow propagate J2eePlatform to GFv3 instance lookup or profiler API could define some interface to be implemented and placed in lookup. For the second option the contract would be much stronger - ProfilerArgsProvider would either found this interface or ignored the server instance completely.
Comment 20 Jaroslav Tulach 2011-12-20 08:34:56 UTC
Y01 I don't understand the domain, but still, it looks to me that name CommonBridge is too general.

Y02 I am not against it, but it is surprising that StartupMode is not enum.

Y03 You don't really need ServerInstanceImplementation2, it is enough to use "serverInstanceImpl1 instanceof Lookup.Provider"

Y04 Classical: no tests.
Comment 21 Petr Hejl 2011-12-20 09:55:28 UTC
(In reply to comment #20)
> Y01 I don't understand the domain, but still, it looks to me that name
> CommonBridge is too general.
The plugins using j2eeserver only should have access to the common representation of the server to be able to use StartupArguments and other APIs from common server.
 
> Y02 I am not against it, but it is surprising that StartupMode is not enum.
We'll I don't really think the enum is extensible when used in API.

> Y03 You don't really need ServerInstanceImplementation2, it is enough to use
> "serverInstanceImpl1 instanceof Lookup.Provider"
Fair enough ;) I'll fix that.

> Y04 Classical: no tests.
I'll do something about that once I'll be sure we are close enough. The patch evolved greatly recently. I don't plan to integrate it this year.
Comment 22 Petr Hejl 2011-12-20 19:50:07 UTC
Created attachment 114370 [details]
updated patch

Fixed Y01 and Y03. The J2eePlatform should be propagated to GlassFish common instance so usable in profiler impl of StartupArgumentsProvider.
Comment 23 Petr Hejl 2011-12-20 19:56:36 UTC
JardoB, could you please finish the patch by implementing the ProfilerArgsProvider (based on JavaPlatform for now as sketched)?
Comment 24 J Bachorik 2011-12-22 09:52:09 UTC
Created attachment 114404 [details]
updated patch

I've added the profiler specific implementation. 

However, I've found out that the current profiler implementation relies on an org.netbeans.modules.j2ee.deployment.plugins.api.InstanceProperties being accessible somehow and it is not easy to factor this dependency out without going through API change in the contract between J2EE deployment and the profiler. 
So I added the InstanceProperties to the lookup alongside the JavaPlatform (which should be used once the profiler API is straightened).

Also, I had to make several changes in the glassfish server plugin to make it work. I've run the unit tests and it seems nothing got broken. Anyway, Vince probably should take a look at the patch and comment.
Comment 25 Petr Hejl 2012-01-03 14:50:26 UTC
Vince, can you review the changes?
Comment 26 Petr Hejl 2012-01-04 20:13:00 UTC
Created attachment 114648 [details]
fixed module version
Comment 27 Vince Kraemer 2012-01-05 00:22:51 UTC
This looks good to me.
Comment 28 Jaroslav Tulach 2012-01-05 06:12:48 UTC
Y05 The registration could be made more lazy, if the StartupArguments.StartMode was made part of the Registration annotation[1]. Without this, any attempt to run or debug a server will initialize part of profiler infrastructure. That does not seem appropriate.

[1] The StartMode has to be enum then. The processor has to write the value of the mode to layer. There has to be a delegating StartupArgumentsProvider which will only activate the real delegate if the mode is correct.
Comment 29 Petr Hejl 2012-01-06 13:06:43 UTC
(In reply to comment #28)
> Y05 The registration could be made more lazy, if the StartupArguments.StartMode
> was made part of the Registration annotation[1]. Without this, any attempt to
> run or debug a server will initialize part of profiler infrastructure. That
> does not seem appropriate.
Nice. I'll do that.
 
> [1] The StartMode has to be enum then. The processor has to write the value of
> the mode to layer. There has to be a delegating StartupArgumentsProvider which
> will only activate the real delegate if the mode is correct.

Jardo what is the preferred way to store an array (of enums) in layer.xml? Is there any? Or should it be registered multiple times for each StartMode?
Comment 30 Jesse Glick 2012-01-06 17:57:11 UTC
(In reply to comment #29)
> what is the preferred way to store an array (of enums) in layer.xml?

Comma-separated lists are used in some layer formats, e.g. the instanceOf attribute.
Comment 31 Jaroslav Tulach 2012-01-06 19:07:00 UTC
I've recently used "value.1", "value.2", etc. attributes. It is also possible to generate multiple .instance files each with only one enum. 

E.g. nothing has been standardized. Once I reported bug 20414, but that was in a time when people edited the layer.xml manually. When the array encoding is just implementation detail, we don't new DTD for that.
Comment 32 Petr Hejl 2012-01-06 22:15:51 UTC
Created attachment 114693 [details]
lazy patch

Some more laziness.
Comment 33 Petr Hejl 2012-01-09 13:15:40 UTC
Created attachment 114731 [details]
clean lazy patch with tests

I would consider this patch final unless there are some other objections.
Comment 34 Jaroslav Tulach 2012-01-09 14:46:59 UTC
Y06 startModes vs. startMode. Often, when the expected case is that there will be just one value, we prefer singular (in spite of allowing an array of registrations). The cases that come to my mind are:
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-parsing-api/org/netbeans/modules/parsing/spi/indexing/ConstrainedBinaryIndexer.Registration.html
http://bits.netbeans.org/dev/javadoc/org-netbeans-core-multiview/org/netbeans/core/spi/multiview/MultiViewElement.Registration.html
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-projectuiapi/org/netbeans/spi/project/ui/support/NodeFactory.Registration.html
sometimes multiple values are expected and the a plural is used. Interesting example is
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-projectapi/org/netbeans/spi/project/LookupProvider.Registration.html
where both forms are used (probably to support compatibility).
Comment 35 Petr Hejl 2012-01-09 15:08:10 UTC
(In reply to comment #34)
> Y06 startModes vs. startMode. Often, when the expected case is that there will
> be just one value, we prefer singular (in spite of allowing an array of
> registrations).
I agree. The trouble is I don't really know the expected case :) The profiler is registered for one StartMode. I expect the JRebel to be registered for two StartModes. For future clients I can make a guess only - all start modes for simple provider, single start mode for specialized ones. Should I go with singular? No problem with that.
Comment 36 Petr Hejl 2012-01-10 10:46:18 UTC
Created attachment 114760 [details]
patch with singular and one more test

I'll integrate this patch on Friday.
Comment 37 igorzep 2012-01-12 14:58:28 UTC
Tried the patch. It works well for JRebel. Thanks for good work!

Would be good to have similar thing for plain Java (Ant/Maven) projects as well.
Comment 38 Petr Jiricka 2012-01-12 15:38:01 UTC
> Would be good to have similar thing for plain Java (Ant/Maven) projects as well.

You may want to request this via separate enhancement requests against the relevant categories: java/Project and projects/Maven. Or look at the sources of the relevant modules (java.j2seproject, maven) to see what's possible right now.
Comment 39 Jesse Glick 2012-01-12 16:05:24 UTC
Sorry, just noticed this.


[JG01] apichanges neglects to mention that ServiceInstance is now a Lookup.Provider; at a minimum it should use <class> to indicate that its signature changed.


[JG02] Calls to instanceAttribute should use the long form mentioning the annotation.


[JG03] Introduce a local var for element.getAnnotation(StartupArgumentsProvider.Registration.class) and silently skip the loop iteration if it is null, which will unfortunately happen when passing partially edited classes through the Java parser.


[JG04] Return false if roundEnv.processingOver.


[JG05] Use position(...), not intvalue(...), for position attributes.


(In reply to comment #37)
> Would be good to have similar thing for plain Java (Ant/Maven) projects as
> well.

Indeed; it seems much of the SPI would be identical, with 'ServerInstance instance' just being replaced by something else, such as a Lookup context in which a ServerInstance might be found, but additionally/instead a Project or something like this. (That would seem to obviate the need to make ServiceInstance a Lookup.Provider, since you could place e.g. InstanceProperties directly in the context.) Of course the API and SPI would need to clearly document what items should be expected in the context.

[JG06] Please at least think about this approach. I know it would be more work but the result would be more broadly useful, and I am pretty sure there are a lot of third-party plugins which want to inject JVM args across a bunch of different project types; their only current option is to use things like AntBuildExtender or LateBoundPrerequisitesChecker which are very specific to particular build systems and scripts. I could help with API calls from Ant projects (typically passing an expanded version of run.jvmargs) and Maven projects (RunJarPrereqChecker, maybe NetBeansRunParamsIDEChecker).

Of course a non-EE-oriented API/SPI could be added later, but we would lose the conceptual and maintenance benefits of sharing a common system.
Comment 40 Petr Hejl 2012-01-13 12:53:40 UTC
Created attachment 114871 [details]
patch with JG01 - JG05 fixed
Comment 41 Petr Hejl 2012-01-13 13:03:23 UTC
(In reply to comment #39)
> Indeed; it seems much of the SPI would be identical, with 'ServerInstance
> instance' just being replaced by something else, such as a Lookup context in
> which a ServerInstance might be found, but additionally/instead a Project or
> something like this.
Originally I thought the project integration would be different and more complex. Obviously there could be just minor signature change, right?

> (That would seem to obviate the need to make
> ServiceInstance a Lookup.Provider, since you could place e.g.
> InstanceProperties directly in the context.) Of course the API and SPI would
> need to clearly document what items should be expected in the context.
> 
> [JG06] Please at least think about this approach. I know it would be more work
> but the result would be more broadly useful,
If this is the case I absolutely agree.

> and I am pretty sure there are a
> lot of third-party plugins which want to inject JVM args across a bunch of
> different project types; their only current option is to use things like
> AntBuildExtender or LateBoundPrerequisitesChecker which are very specific to
> particular build systems and scripts. I could help with API calls from Ant
> projects (typically passing an expanded version of run.jvmargs) and Maven
> projects (RunJarPrereqChecker, maybe NetBeansRunParamsIDEChecker).
I do know nothing about the core Project APIs. I can certainly replace ServerInstance with Lookup, but the usage of the API in the project would be more or less up to you.

Is there any proper module where the generic StartupArguments API could be placed? Should I create new module?

> 
> Of course a non-EE-oriented API/SPI could be added later, but we would lose the
> conceptual and maintenance benefits of sharing a common system.
Comment 42 Jesse Glick 2012-01-13 13:56:06 UTC
(In reply to comment #41)
> there could be just minor signature change, right?

Perhaps. Would need to be prototyped in a named branch to see if it is really workable.

> I can certainly replace ServerInstance with Lookup,
> but the usage of the API in the project would be more or less up to you.

In non-EE-oriented projects (or run modes) I guess you mean.

> Is there any proper module where the generic StartupArguments API could be
> placed?

Hmm, glassfish.common is in the ide cluster, I suppose because you could use it with something like JRuby, so java.platform or java.project are not options. We have had similar confusion in the past picking a cluster for code which is specific to the JVM (which this arguably is) but not the JLS.

extexecution comes to mind as a somewhat related module. Some potential clients of the API (e.g. java.j2seproject) do not actually use extexecution to launch the program, but that would not stop them from using it for this purpose since they just get back a List<String> of arguments and do with it what they want.

api.java.classpath is tangentially related (consider ClassPath.BOOT) but probably not an intuitive fit.

projectapi is another candidate, since it does define ActionProvider.COMMAND_RUN and even COMMAND_DEBUG.
Comment 43 Petr Hejl 2012-01-17 16:06:12 UTC
I created named branch startup_arguments in web-main cfab36765b87.
The startup arguments API is placed in extexecution. There are no real functional changes compared to the last patch. I kept getLooukup() in ServerInstance for now. We can remove it later.

Jesse can you prototype non EE project integration?
Thanks,
P.
Comment 44 Petr Hejl 2012-01-18 13:29:46 UTC
Fixed since tags and package level javadoc.
cfab36765b87
Comment 45 Petr Hejl 2012-01-18 13:33:35 UTC
(In reply to comment #44)
> Fixed since tags and package level javadoc.
> cfab36765b87
The changeset is 9ff7055058a7.
Comment 46 Jesse Glick 2012-01-20 17:12:33 UTC
(In reply to comment #43)
> Jesse can you prototype non EE project integration?

Working on it. Leads me to

[JG07] Consider adding StartMode.TEST (maybe also DEBUG_TEST and PROFILE_TEST). Could be used, for example, by plugins which perform instrumentation of tests for purposes of code coverage.
Comment 47 Jesse Glick 2012-01-20 18:00:03 UTC
Created attachment 115133 [details]
Complete testing plugin

Attaching sources for a plugin which uses the SPI on a j2seproject.
Comment 48 Jesse Glick 2012-01-20 18:02:45 UTC
I pushed changes to the branch which lets j2seproject call the new API (both in CoS and build.xml mode). Seems to work. I will play with usage from Maven and apisupport.
Comment 49 Jesse Glick 2012-01-20 23:04:41 UTC
Branch now has support for jar-packaging Maven projects (with the default exec:exec bindings).


[JG08] Sorry for the scope creep, but perhaps the API should be called something like LaunchDecorator? Then it could initially have just List<String> arguments but if the need arose we could add other plausible adjustments of the kind that ProcessBuilder would allow, especially a Map<String,String> environment.

I am not seeing any particular reason why this API should be restricted to JVM-based programs; I think it would work equally well for, say, PHP or C++ programs, if there are standard flags or environment variables that it would make sense to add from a plugin (php -B 'some code...', or LD_PRELOAD=/some/hooks.so, etc.). Should not require any particular API changes, though the current Javadoc discusses "JVM arguments" and other JVM-specific terms which might be rephrased to say that this is a typical example, not the only possible use case. Of course the SPI implementor needs to _somehow_ know what kind of project it is adding arguments to, so that it can be sure the arguments can be interpreted; but I think this is adequately addressed via existing APIs, e.g. Projects/<TYPE>/Customizer in the example plugin.

Still unsure whether extexecution or projectapi is the most appropriate home. Had to add a dep on extexecution from java.api.common just for this API. Opinions from other reviewers solicited.
Comment 50 Jesse Glick 2012-01-21 00:08:04 UTC
[JG09] (only sensible if inside projectapi) Suggest static @CheckForNull StartMode StartMode.forCommand(String command) which would map well-known command names from ActionProvider into corresponding start modes. Otherwise it seems each project type has to more or less duplicate the same code. (Not all project types will support all of these commands, e.g. COMMAND_DEBUG_STEP_INTO, but that seems harmless.)
Comment 51 Jesse Glick 2012-01-21 00:15:52 UTC
Created attachment 115137 [details]
Revised testing plugin

Branch and test plugin now handle apisupport, both Ant- and Maven-based.
Comment 52 Petr Hejl 2012-01-21 09:50:12 UTC
(In reply to comment #49)
> Branch now has support for jar-packaging Maven projects (with the default
> exec:exec bindings).
> 
> 
> [JG08] Sorry for the scope creep, but perhaps the API should be called
> something like LaunchDecorator?
What about StartupExtender?

> Then it could initially have just List<String>
> arguments but if the need arose we could add other plausible adjustments of the
> kind that ProcessBuilder would allow, especially a Map<String,String>
> environment.
> 
> I am not seeing any particular reason why this API should be restricted to
> JVM-based programs; I think it would work equally well for, say, PHP or C++
> programs, if there are standard flags or environment variables that it would
> make sense to add from a plugin (php -B 'some code...', or
> LD_PRELOAD=/some/hooks.so, etc.).
I've been thinking about the same thing.

> Should not require any particular API
> changes, though the current Javadoc discusses "JVM arguments" and other
> JVM-specific terms which might be rephrased to say that this is a typical
> example, not the only possible use case. Of course the SPI implementor needs to
> _somehow_ know what kind of project it is adding arguments to, so that it can
> be sure the arguments can be interpreted; but I think this is adequately
> addressed via existing APIs, e.g. Projects/<TYPE>/Customizer in the example
> plugin.
> 
> Still unsure whether extexecution or projectapi is the most appropriate home.
> Had to add a dep on extexecution from java.api.common just for this API.
> Opinions from other reviewers solicited.
Originally I wasn't sure as well. However in case we made it JVM independent with generic usage. I would thing the extexecution is the right place. In that case I would also extend the integration with ProcessBuilder. It could have a method extend/decorate(Lookup, StartMode) which would simplify the usage for client using the extexecution to actually run the program.
Comment 53 Petr Hejl 2012-01-21 10:00:50 UTC
(In reply to comment #50)
> [JG09] (only sensible if inside projectapi) Suggest static @CheckForNull
> StartMode StartMode.forCommand(String command) which would map well-known
> command names from ActionProvider into corresponding start modes. Otherwise it
> seems each project type has to more or less duplicate the same code. (Not all
> project types will support all of these commands, e.g. COMMAND_DEBUG_STEP_INTO,
> but that seems harmless.)

In case we will make it generic I would not consider the api project related, but execution related. Could be used for any external program startup from my point of view.
Comment 54 Petr Hejl 2012-01-23 11:30:19 UTC
Fixed JG07 10a0f745f734.
Comment 55 Jesse Glick 2012-01-23 15:56:22 UTC
(In reply to comment #52)
>> [JG08] ...
>
> What about StartupExtender?

Works for me.

> I would also extend the integration with ProcessBuilder. It could have a
> method extend/decorate(Lookup, StartMode) which would simplify the usage for
> client using the extexecution to actually run the program.

Possible, but there are no potential users of such a method right now. (Ant execution uses <java>. Maven execution does use ProcessBuilder but very indirectly - the calls to this API go through other layers.)

(In reply to comment #54)
> Fixed JG07

When I get a chance I will extend launch code to check for these modes too.
Comment 56 Jesse Glick 2012-01-23 23:40:35 UTC
The change to ProfilerArgsProvider.java looks suspicious. The modes in @Registration do not match those checked for in getArguments.

Anyway why is getArguments checking the mode at all? It will not be called at all unless the mode matches one of those specified in the registration.

Finally, it seems unlikely that you would actually want TEST_PROFILE for this provider. A test would be started by the project type with a Project in the lookup, not an InstanceProperties, because you are not starting the EE server (for a true unit test at least). TEST_PROFILE would not be used for something like this AFAIK.


Used TEST_* some in web-main #f238d31c98be. Do not have an example of a plugin which would register under it at the moment. Would need to be something you can run optionally, like Cobertura/Emma, but not something intrinsically needed for the tests, like JMockit.
Comment 57 Petr Hejl 2012-01-24 09:08:43 UTC
(In reply to comment #56)
> The change to ProfilerArgsProvider.java looks suspicious. The modes in
> @Registration do not match those checked for in getArguments.
> 
> Anyway why is getArguments checking the mode at all? It will not be called at
> all unless the mode matches one of those specified in the registration.
Yep makes sense.
 
> Finally, it seems unlikely that you would actually want TEST_PROFILE for this
> provider. A test would be started by the project type with a Project in the
> lookup, not an InstanceProperties, because you are not starting the EE server
> (for a true unit test at least). TEST_PROFILE would not be used for something
> like this AFAIK.
I wasn't quite sure. I removed it. Jardo can you check?

web-main #e87d1b570aa4

 
> Used TEST_* some in web-main #f238d31c98be. Do not have an example of a plugin
> which would register under it at the moment. Would need to be something you can
> run optionally, like Cobertura/Emma, but not something intrinsically needed for
> the tests, like JMockit.
Comment 58 Petr Hejl 2012-01-24 10:16:56 UTC
(In reply to comment #55)
> (In reply to comment #52)
> >> [JG08] ...
> >
> > What about StartupExtender?
> 
> Works for me.
Now I'm not sure about naming as *Provider imo does not really describe the purpose. Does this refactoring make sense?

StartupArguments -> StartupExtender
    getStartupArguments() -> getExtenders()
StartupArgumentsProvider -> StartupExtenderImplementation

> 
> > I would also extend the integration with ProcessBuilder. It could have a
> > method extend/decorate(Lookup, StartMode) which would simplify the usage for
> > client using the extexecution to actually run the program.
> 
> Possible, but there are no potential users of such a method right now. (Ant
> execution uses <java>. Maven execution does use ProcessBuilder but very
> indirectly - the calls to this API go through other layers.)
When I wrote that I thought serverplugins could use that, but they actually have to set up arguments to JAVA_OPTS. So you are right and there is no client at the moment.
Comment 59 Jesse Glick 2012-01-26 15:45:05 UTC
(In reply to comment #58)
> Does this refactoring make sense?
> 
> StartupArguments -> StartupExtender
>     getStartupArguments() -> getExtenders()
> StartupArgumentsProvider -> StartupExtenderImplementation

Seems reasonable to me. "getArguments" would remain the same I suppose. Do not forget org.netbeans.spi.extexecution.startup.StartupArguments (confusing that it has the same simple name as the API class BTW).
Comment 60 Petr Hejl 2012-01-26 15:49:26 UTC
Rename refactoring in web-main #6ca669b35e5e
Comment 61 Petr Hejl 2012-01-26 15:52:36 UTC
(In reply to comment #59)
> (In reply to comment #58)
> > Does this refactoring make sense?
> > 
> > StartupArguments -> StartupExtender
> >     getStartupArguments() -> getExtenders()
> > StartupArgumentsProvider -> StartupExtenderImplementation
> 
> Seems reasonable to me. "getArguments" would remain the same I suppose. Do not
> forget org.netbeans.spi.extexecution.startup.StartupArguments (confusing that
> it has the same simple name as the API class BTW).
I've just done that. I also removed references to VM or JVM from the javadoc.

I would also liek to change the context lookup for Java EE to contain ServerInstance to be on same "level" with a project context. So implementor may expect Project/ServerInstance instead of Project/Lookup of ServerInstance.
Comment 62 Jesse Glick 2012-01-26 15:55:22 UTC
Allan (on CC) asked about support from freeform projects. This would not be straightforward to implement. By the nature of a freeform project, the IDE does not know what exactly is happening when an action like "Run" is invoked - it just calls a specified Ant target. The user's script will typically run <java fork="true"> at some point, but this task invocation may or may not accept a default-blank property with extra JVM run args, and even if it did the IDE would not know what the property was named. And of course the script might use <exec> or something else entirely to run the program.

It would be possible to expand the schema for freeform project.xml so that an <action> could have an optional subsection such as

  <extensions>
    <arguments-property>jvm.runargs</arguments-property>
    <mode>NORMAL</mode>
    <mode>DEBUG</mode>
  </extensions>

in which case the IDE could ask providers for arguments to add to -Drun.jvmargs=... when calling Ant. Whether many users would discover and successfully use such a feature is another question.


For those using autoprojects [1] instead, which use "Compile on Save" mode to implement Run File and similar actions, it should be straightforward to inject startup arguments, though it is not yet implemented. TBD whether such implementation should be in autoproject.java or in JavaRunner. Currently java.j2seproject does this itself even in CoS mode, but perhaps JavaRunner should automatically call this API when QUICK_RUN and related actions are used.


[1] http://wiki.netbeans.org/AutomaticProjects
Comment 63 Jesse Glick 2012-01-26 16:38:40 UTC
(In reply to comment #61)
> change the context lookup for Java EE to contain
> ServerInstance to be on same "level" with a project context. So implementor may
> expect Project/ServerInstance instead of Project/Lookup of ServerInstance.

Not quite sure I understand, but make sure StartupExtender Javadoc is explicit.

For SE-style API calls I thought about using project.getLookup() rather than Lookups.singleton(project) as the context, which would still contain the Project but would also offer its other services; but I did not see a compelling reason to do this.

Allan also indicated that it could be useful for a JavaPlatform to be in the context when a Java-based project is run, corresponding to the JVM expected to be running the program. This would be convenient for extender impls - otherwise they need to look up ClassPath.BOOT on the project directory, and after bug #107071 is implemented give preference to BOOT_EXECUTE; also projects might want to have multiple run configurations using different JVMs and it would be hard to predict via other APIs which is being used. I will try to implement this.
Comment 64 Jesse Glick 2012-01-26 23:26:18 UTC
(In reply to comment #62)
> perhaps JavaRunner
> should automatically call this API when QUICK_RUN and related actions are used

Done in branch; tested with both j2seproject and autoproject.

(In reply to comment #63)
> it could be useful for a JavaPlatform to be in the
> context when a Java-based project is run

Done for j2seproject, and for any project type using JavaRunner/CoS. Pending for Ant-based NBM, Maven-based NBM, Maven-based SE.
Comment 65 Petr Hejl 2012-02-01 10:25:48 UTC
Should we merge the branch to trunk?
Comment 66 J Bachorik 2012-02-01 10:30:21 UTC
Sounds good. I have no further comments.

(In reply to comment #65)
> Should we merge the branch to trunk?
Comment 67 Jesse Glick 2012-02-01 15:38:58 UTC
I have a couple of items I want to try doing today in the branch:

1. JavaPlatform in other project types.

2. Document that StartupExtender is to be called only when it is certain the values it returns will be used (yardus's request).


igorzep did you get a chance to check JRebel integration with the revised API?


I am on vacation Thu/Fri/Mon, but other than the above it is fine to merge to trunk as far as I am concerned; check dates in apichanges.xml, and make sure to close the branch [1] before merging.


[1] http://wiki.netbeans.org/HgHowTos#Using_a_named_branch
Comment 68 igorzep 2012-02-01 16:01:26 UTC
I haven't tried it with the revised API yet, where is it currently? The startup_arguments branch? I'll try to check it ASAP...
Comment 69 Petr Hejl 2012-02-01 16:06:36 UTC
(In reply to comment #68)
> I haven't tried it with the revised API yet, where is it currently? The
> startup_arguments branch? I'll try to check it ASAP...
Yes startup_arguments branch of web-main; extexecution module.
Comment 70 Jesse Glick 2012-02-01 16:12:10 UTC
(In reply to comment #69)
> extexecution module

...is where the API is. To be useful, you have to rebuild other modules.

$ hg di --stat -r 'ancestor(startup_arguments,default)' -r startup_arguments

will show you which were modified.
Comment 71 Jesse Glick 2012-02-01 17:21:36 UTC
Created attachment 115438 [details]
Current testing plugin
Comment 72 Jesse Glick 2012-02-01 17:52:29 UTC
Created attachment 115440 [details]
Sample plugin, uses JavaPlatform where available

Example text to type in expression box:

Thread.currentThread().contextClassLoader
Comment 73 Jesse Glick 2012-02-01 18:00:16 UTC
OK, I have made the changes I planned in the branch, mainly passing JavaPlatform from all supported non-EE project types.
Comment 74 Petr Hejl 2012-02-01 19:25:53 UTC
Ok, I'll wait for igorzep's opinion and I'll merge the branch afterwards.
Comment 75 igorzep 2012-02-05 20:43:01 UTC
Fails for me with:

java.lang.IllegalStateException: Delegate must not be null
	at org.netbeans.modules.extexecution.startup.ProxyStartupExtender.getDelegate(ProxyStartupExtender.java:96)
	at org.netbeans.modules.extexecution.startup.ProxyStartupExtender.getArguments(ProxyStartupExtender.java:81)
	at org.netbeans.api.extexecution.startup.StartupExtender.getExtenders(StartupExtender.java:107)
	at org.netbeans.modules.java.source.ant.ProjectRunnerImpl.computeProperties(ProjectRunnerImpl.java:240)
	at org.netbeans.modules.java.source.ant.ProjectRunnerImpl.execute(ProjectRunnerImpl.java:127)
	at org.netbeans.api.java.project.runner.JavaRunner.execute(JavaRunner.java:276)
	at org.netbeans.modules.java.api.common.project.BaseActionProvider.bypassAntBuildScript(BaseActionProvider.java:1389)
	at org.netbeans.modules.java.api.common.project.BaseActionProvider.access$1400(BaseActionProvider.java:153)
	at org.netbeans.modules.java.api.common.project.BaseActionProvider$1Action.run(BaseActionProvider.java:490)
	at org.netbeans.api.java.source.ui.ScanDialog.runWhenScanFinished(ScanDialog.java:153)
	at org.netbeans.modules.java.api.common.project.BaseActionProvider.invokeAction(BaseActionProvider.java:615)
	at org.netbeans.spi.project.support.LookupProviderSupport$MergedActionProvider.invokeAction(LookupProviderSupport.java:261)
	at org.netbeans.modules.project.ui.actions.FileAction.actionPerformed(FileAction.java:182)
	at org.netbeans.modules.project.ui.actions.LookupSensitiveAction.actionPerformed(LookupSensitiveAction.java:170)
	at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1995)
	at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2318)
	at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:387)
	at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:242)
	at javax.swing.AbstractButton.doClick(AbstractButton.java:357)
	at javax.swing.plaf.basic.BasicMenuItemUI.doClick(BasicMenuItemUI.java:1223)
	at javax.swing.plaf.basic.BasicMenuItemUI$Handler.mouseReleased(BasicMenuItemUI.java:1264)
	at java.awt.Component.processMouseEvent(Component.java:6263)
	at javax.swing.JComponent.processMouseEvent(JComponent.java:3267)
	at java.awt.Component.processEvent(Component.java:6028)
	at java.awt.Container.processEvent(Container.java:2041)
	at java.awt.Component.dispatchEventImpl(Component.java:4630)
	at java.awt.Container.dispatchEventImpl(Container.java:2099)
	at java.awt.Component.dispatchEvent(Component.java:4460)
	at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4574)
	at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4238)
	at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4168)
	at java.awt.Container.dispatchEventImpl(Container.java:2085)
	at java.awt.Window.dispatchEventImpl(Window.java:2478)
	at java.awt.Component.dispatchEvent(Component.java:4460)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:599)
	at org.netbeans.core.TimableEventQueue.dispatchEvent(TimableEventQueue.java:162)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
[catch] at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)

This trace is for running Ant project, but fails also when running Tomcat with the same message. Do I miss something?
Comment 76 igorzep 2012-02-05 21:18:45 UTC
Missed a dependency, strangely didn't get error from Maven nbm build, as far as I remember I got it in such cases previously...

Now it works well with all my cases, so it can be merged.
Comment 77 Petr Hejl 2012-02-07 11:18:19 UTC
Merged to trunk #e4409499a9f4.
Comment 78 Quality Engineering 2012-02-17 11:02:19 UTC
Integrated into 'main-golden', will be available in build *201202170400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/5a41ff24a35b
User: Jesse Glick <jglick@netbeans.org>
Log: #206196 support for netbeans.org modules.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo