Created attachment 152026 [details]
Connect at the end of Maven goal execution if a port is specified

The attached patch is enough to work with enhancement
https://github.com/simpligility/android-maven-plugin/pull/584
one only needs to specify following action in nbactions.xml:

<action>
  <actionName>debug</actionName>
  <goals>
    <goal>clean</goal>
    <goal>package</goal>
    <goal>android:deploy</goal>
    <goal>android:run</goal>
  </goals>
  <properties>
    <skipTests>true</skipTests>
    <android.run.debug>7543</android.run.debug>
    <jpda.listen>7543</jpda.listen>
  </properties>
</action>

There are some drawbacks with the current approach (fixed port, connecting only when goal execution is over), but I wanted to give you a chance for early feedback even right now.

The other usecase to support is RoboVM debugging as discussed at
https://groups.google.com/d/msg/robovm/1ZRNirtmjjI/cVNaImtuvv0J

TS1 regarding javadoc, ACTION_PROPERTY_JPDALISTEN semantics is a bit different

so maybe it would be possible and better to use also a different key (constant)/property instead

TS2 hardcoded name in JPDADebugger.attach("localhost", ...)

the ExecutionResultChecker.executionResult(...) implementation in DebuggerChecker is a generic part and it would be great if other potentials clients/users would also have the freedom to specify the host with a different name/format - (127.0.01 or maybe even their public host name)
(if for nothing else, then for robustness sake. Recently we run into a case when localhost:port was refused but 127.0.0.1:port accepted.)

Yes, I agree with Tomas, it would be better to use a different property, like jpda.attach.address of the form <host_name>:<port_number>.
Therefore you'd define:
<jpda.attach.address>localhost:7543</jpda.attach.address>

On Windows it's better to use a shared memory connector rather than socket connection, because of performance. I'm not sure if you're interested in Windows at all. If so, we can enhance it in the following way:

On Windows you'd specify:
<jpda.attach.transport>dt_shmem</jpda.attach.transport>
<jpda.attach.address>mysharedmemory</jpda.attach.address>

On other OSes:
<jpda.attach.transport>dt_socket</jpda.attach.transport> - or missing,
  dt_socket transport is the default one.
<jpda.attach.address>localhost:7543</jpda.attach.address>

I'm suggesting "jpda.attach...." names to indicate that this is a supplementary attach and if I get it right, there should be no relation with the existing jpda.listen property that is used by the NB maven project to initiate debugging. This is like post-attach, right?

Adding these constants into the Constants class is an API change, but I hope that this is not a big problem. The patch would be modified accordingly (I'll update it...). Would you agree?

Also, shouldn't the new code in executionResult() be called only on some specific config.getActionName()?
In the current patch it looks strange and the code will be executed e.g. for "debug.fix" action, which is not what do you want.

Created attachment 152087 [details]
Suggested patch with attach constants.

The suggested modification of the patch. Please test if this suits your needs.

Created attachment 152088 [details]
Suggested patch with attach constants.

> Also, shouldn't the new code in executionResult() be called only on some specific config.getActionName()?
> In the current patch it looks strange and the code will be executed e.g. for "debug.fix" action, which is not what do you want.
wouldn't bind it to a specific action name(s) - that's up to the third party clients

but similar to debug.fix we should also test resultCode == 0 (goal execution failed, so why starting debugger - jarda please confirm!)
that, together with the check Constants.ACTION_PROPERTY_JPDAATTACH_ADDRESS  seems to be enough

Path #1: If we stick with doing the attach at the end of maven goal execution, then yes: Using different property is reasonable. Trying to attach only if the goal succeeds is reasonable. Thanks for new patch polishing my initial attempts, Martine.

The localhost&shared mem concerns haven't come to my mind - but we may need a platform independent way of expressing that in nbactions.xml, imho - nobody will spend time writing different nbactions.xml for Unix and different for Windows. Not mentioning it is probably impossible to have different action on Windows/Unix in nbactions.xml...

Path #2: If the RoboVM cooperation (e.g. https://groups.google.com/d/msg/robovm/1ZRNirtmjjI/cVNaImtuvv0J) goes well, we will be OK with solution #1 (as that will work on Android). If RoboVM fails to implement the server mode - we will need to attach the debugger before the maven goal execution is over - that would be completely diffent API...

So I am suggesting to focus on path #1, polish it, and give the RoboVM guys a bit of time to prove we won't need path #2. Anyway thanks for looking into my inquiry - I am sure providing solution to it will make NetBeans better!

O.K. dt_socket transport works on all platforms, thus using everywhere
<jpda.attach.address>localhost:7543</jpda.attach.address>
would be sufficient.

BTW in ANT's JPDAStart tak we use both socket and shared memory connector, depending on the OS, we should probably do the same in Maven's JPDAStart. But that'd be an enhancement for Maven debugger launching.

Yardo, can you confirm that we should add test for resultCode == 0 into the patch? Then as soon as someone test it with RoboVM and nobody objects, we can push the patch I think.

(In reply to Martin Entlicher from comment #10)
> Yardo, can you confirm that we should add test for resultCode == 0 into the
> patch? 

Yes. Checking for successful execution of the Maven goal makes sense.

One more thing for your consideration. The example in comment #1 hardcodes port number. That is not nice. Maybe we could enhance NetBeans to automatically find an open port in similar way jpda.listen=true does? Something like:

<action>
  <actionName>debug</actionName>
  <goals>
    <goal>clean</goal>
    <goal>package</goal>
    <goal>android:deploy</goal>
    <goal>android:run</goal>
  </goals>
  <properties>
    <skipTests>true</skipTests>
    <jpda.attach>true</jpda.attach>
    <android.run.debug>${jpda.attach.port}</android.run.debug>
  </properties>
</action>

The behavior would be: As soon as jpda.attach=true is specified, the NetBeans runner find out available port (let's assume 12345) and sets jpda.attach.port=12345 and jpda.attach.address=localhost:12345 as Maven properties. Those properties could then be used in other parts of the pom.xml - like a value of android.run.debug property.

Well, attach behaves the other way than listen.
Listen calls ListeningConnector.startListening() to choose a free port and start listening on it. The program being debugged then connects to that port.

In attach, the program is started first with a chosen port number via -Xrunjdwp:transport=dt_socket,server=y,address=xxxx
NetBeans then attaches to the port xxxx.
Therefore you probably need some separate task, that would find the free port before the program is started and before NetBeans tries to attach.

from the discussion I see that you're starting RoboVM via:
$ mvn -DskipTests=true -Drobovm.debug=true -Drobovm.debugPort=8880 package robovm:ipad-sim

Then you could probably use -Drobovm.debugPort=${jpda.attach.port} and set the jpda.attach.port in the project properties or have some task to set it up?

Created attachment 152124 [details]
Extended patch handling errorCode and finding free port

With the attached patch and with pull request https://github.com/simpligility/android-maven-plugin/pull/584 on the plugin side I can:

    <action>
        <actionName>debug</actionName>
        <goals>
            <goal>clean</goal>
            <goal>package</goal>
            <goal>android:deploy</goal>
            <goal>android:run</goal>
        </goals>
        <properties>
            <skipTests>true</skipTests>
            <android.run.debug>${jpda.attach.port}</android.run.debug>
            <jpda.attach>define</jpda.attach>
        </properties>
    </action>

Before execution the value of jpda.attach=define is found and replaced with localhost:XYZ where XYZ is free local port. Also jpda.attach.port and jpda.attach.address are defined (I need only the port) which is then passed to the execution of the android-maven task.

I have not spend time on documenting the properties, I assume Martin will want to rename them, modify their count and meaning a bit...

The Android plugin request is in:
https://github.com/simpligility/android-maven-plugin/pull/584
I am skiing now, but I'd like to merge my last patch on Monday Mar 9, 2015.

Any objections?

Created attachment 152507 [details]
final patch to be integrated tomorrow

Integrated as
https://hg.netbeans.org/core-main/rev/5c7a73f64724

It looks good to me, thanks.

Integrated into 'main-silver', will be available in build *201503110001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/5c7a73f64724
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #250459: Run with -Ddebug.attach=true to let the IDE find an empty port, pass it value into the goal execution as -Ddebug.attach.port and at the end of the execution attach to it