*** Issue 126570 has been marked as a duplicate of this issue. ***

Created attachment 64953 [details]
current javadoc

I'd like to make this module (extexecution) public under development. It is placed in extexecution directory.

-it should be moved to ide cluster
-package refactorings intended:
org.netbeans.modules.extexecution.api -> org.netbeans.api.extexecution
org.netbeans.modules.extexecution.api.input -> org.netbeans.api.extexecution.input
org.netbeans.modules.extexecution.api.print -> org.netbeans.api.extexecution.print
-contrib module languages.execution should be removed

Interested clients I know about (except current friends): serverplugins, erlang, ruby.

Please review.

MK1: Are the Java Home env var related methods necessary in a generic execution API?

Re MK1: No, they are not. I'll remove it. Thanks for catching this.

[JG01] ExecutionDescriptor.getOut/ErrProcessorFactory Javadoc says "ExecutionService automatically uses the printing
one." - vague, what is "the printing one"? Use @link etc. where appropriate.


[JG02] getOut/ErrConvertorFactory says "(that used by ExecutionService automatically." which is not even a sentence - typo?


[JG03] The Javadoc for ED and ED.Builder do not seem to make clear what the defaults for the various options are.


[JG04] ED.Builder could be folded into ED itself, which might be easier to understand. Example:

public class ED {
  public ED();
  public ED controllable(boolean);
  // ...
}

There would be no particular need for getter methods, since the descriptor is just passed to ExecutionService in the
same package anyway, and I don't see any likely need to pass a descriptor around to other code before then.

The class Javadoc for ED is also misleading; it says

"To build the most common kind of descriptor use ExecutionDescriptor.Builder."

when in fact the *only* way to make a descriptor is to use the builder.


[JG05] Suggest avoiding nested classes such as ED.InputProcessorFactory - harder to type and manage imports for.


[JG06] ExecutionService.run Javadoc is vague. I assume the Integer is a process exit status but this is not documented.
And it says

"This method can be invoked multiple times returning the different and unrelated Futures."

which is unexplained - is the processCreator called again each time? What would be the point of calling this method
multiple times, when you could just make a newService for each distinct process?


[JG07] What if the Callable<Process> cannot create a Process? (IOException is common e.g. if the executable path cannot
be found.) Is it supposed to wrap IOException in RuntimeException? What happens to the Future<Integer> in such a case?


[JG08] ExternalProcessBuilder would seem to be more convenient if it actually implemented Callable<Process>, rather than
forcing you to write

new Callable<Process>() {public Process call() {return myEPB.create();}}

which ties into JG07 since create() throws IOException. Perhaps a new

interface ProcessCreator {
  Process create() throws IOException;
}

(to be implemented by EPB) would be a good replacement for Callable<Process>.


[JG09] EPB's constructor should clarify that the "command" is intended to be a single executable ("echo"), not a
shell-parsed compound of command and arguments ("echo hello").


[JG10] "pwd" stands for "print working directory" and is inappropriate for a setter. Perhaps use "cwd", or better,
"workingDirectory".


[JG11] What would pwd(null) mean? Remove this possibility unless it does something useful.


[JG12] I am not sure why you would ever call pwdToPath but it seems this could be removed without causing any great
inconvenience.


[JG13] Given the semantics of addPath, "prependPath" might be a better name.


[JG14] "HTTP proxy settings are configured (http.proxyHost and http.proxyPort variables)." is vague. Do you mean you set
some environment variables for the subprocess based on the abovenamed Java system properties in the caller?

[JG15] InputProcessor.reset() should explain what it is that might be reset. It is hard to guess from the Javadoc what
this would be used for.


[JG16] InputReaderTask class Javadoc shows method executorService.submit(runnable) and executorService.shutdownNow()
which do not appear to exist.


[JG17] InputReaders.forFileInputProvider is not very clear. Is the fileProvider going to be called several times as each
file is read in turn? How do you signal that there are no more files to read?


[JG18] LineConvertors.filePattern refers to "extPattern" but I guess you mean "filePattern".


[JG19] If fileLocator is null, is some default impl used that e.g. looks for absolute path? or file:/some/path?


[JG20] Do file lines begin at 0 or 1? Document it.


[JG21] Is there any facility for matching column numbers, as the Ant module does?

Wow, thanks Jesse.

Re JG01: Should be fixed in main.
Re JG02: I couldn't find "ExecutionService automatically" anywhere.
Re JG03: It is described in Builder constructor's javadoc.
Re JG04: I'll consider it/work on it tomorrow. Fixed javadoc.
Re JG07 & JG08: Callable declares call() as "public V call() throws Exception". Future throws ExecutionException on
get(). It's a good idea for EPB to implement Callable<Process>. I'll do it.
Re JG09: Fixed in main.
Re JG15: Fixed in main.
Re JG16: JDK's ExecutorService != ExecutionService. input.* api does not depend on execution one. Improved sample code.
Re JG18: Fixed in main.

I'll look at the rest tomorrow (need to change clients).

Changeset c914201b3bf9.

Re MK01: Fixed in main.

Re JG05: I think it is the right usage of nested classes (such as Map.Entry) - these classes don't have any meaning
outside of the ED.

Re JG07 & JG08: Is there any benefit of having ProcessCreator instead of Callable<Process>? EPB can look like this:
public final class ExternalProcessBuilder implements Callable<Process> {
    ...
    public Process call() throws IOException {
        // create process
    }
    ...
}


Re JG10: Fixed in main.

Re JG11: Fixed in main.

Re JG12: Fixed in main.

Re JG13: Fixed in main.

Re JG19: Improved Javadoc. The default impl will try to find the file simply by new File(filename).isFile().

Re JG21: No. I'll look at ant.

Changeset e53fbd324b1e and 00cd8c124f7a.

BTW overall looking pretty nice; one of the better documented APIs I have seen be reviewed.


JG02: HTML Javadoc says

"getOutConvertorFactory

public ExecutionDescriptor.LineConvertorFactory getOutConvertorFactory()

    Returns the factory for convertor to use with processor printing the standard output (that used by ExecutionService
automatically.

    Returns:
        the factory for convertor to use with processor printing the standard output"


JG03 - OK, though I would have expected each property to specify its default; when I am looking to see what the default
value is for a property, I am probably looking at the Javadoc of that property.

BTW "properites" is a typo.


JG07/08 - Callable<Process> is fine; I did not realize it could throw Exception. But Javadoc should be explicit about
what the consequence of that is. And yes it would be nice for EPB to implement it.

Re JG02: Fixed in main.
Re JG07/08: In order to solve this:
-EPBuilder refactored to EPCreator
-it implements Callable<Process>
-it is immutable
-improved javadoc

Attaching the patch.

Created attachment 65258 [details]
EPB to EPC refactoring

Re JG07/08: Actually I'm not sure about the preferred name - Builder/Creator. Any opinion?

"Builder" was OK with me; "Creator" is OK too. No opinion.

BTW if you use --git (or [diff] git=1) then patches with renames are a lot easier to read.

vk01: ExternalProcessBuilder workingDirectory(File workingDirectory) looks like a setter... doesn't have set in the name.

vk02: the comment for ExternalProcessCreator workingDirectory(File workingDirectory) makes it sound like null is an
acceptable argument.. but the code checks that it is not.

vk03: the api "pattern" you are using in ExternalProcessCreator is not the pattern that most other APIs in the platform
seem to follow.  Why are you using this alternate pattern?

vk04: Why call() instead of startProcess() or exec()... the name of the method should expose the fact that the Process
is started.

vk05: it looks like you are missing unit tests for some of the methods that are non-trivial like workingDirectory() and
redirecterrorStream()... are these covered by qa-functional tests?

I am also confused by your comments... fixed in main. you have already committed this without review? oh well.

vk06: (correction to vk01) ExternalProcessCreator workingDirectory(File workingDirectory) looks like a setter... doesn't
have set in the name.

Re JG04: Fixed in main.
Re JG07/08: Fixed in main (I preserved ExternalProcessBuilder).
Re JG14: Fixed in main.

To make summary - still not done: JG17, JG20, JG21.

Changeset 04d727bccae4.

Re vk02: I'll improve it, however I'm bit afraid of talking explicitly about null in javadoc where null is not allowed -
while quick read this could be confusing.
Re vk03: Originally it was builder + immutable class. Jesse suggested that builder could me merged. So it is immutable
class. Immutable objects are inherently thread safe, you can easily define constants etc.
Re vk04: It was this way originally. On Jesse's suggestion I change the Builder to implement Callable<Process>. It is
more usable for typical usecase.
Re vk05: I'll improve it tomorrow.
Re vk06: It is not a setter.

>>I am also confused by your comments... fixed in main. you have already committed this without review? oh well.
We are not reviewing particular change. The purpose of this review is to make extexecution public. It is already in
trunk and it is friend and I'm still fixing it. This review is not about the patch in desc13 which was posted to make an
agreement on JG07/08 solution.

Re JG06: Fixed in main.
Re JG17: Fixed in main.
Re VK02: Fixed in main.
Re VK05: In fact these methods are pretty simple. The test would be one-liner and would need to increase access level.
I've added immutability test.

Changeset a79163f0c76c.

Re JG20: Fixed in main.

Changeset 3c57c9eb7026. Remaining - JG21.

*** Issue 57000 has been marked as a duplicate of this issue. ***

[JG22] Possibility for consideration - a flag to run the process with a special wrapper which sets stdout to be
unbuffered, since otherwise programs which print to stdout without flushing can see their output delayed for a long time
or even up to program exit. See issue #147099 for an example. Alternately, find a way to create a pseudotty; this would
also enable Console usage (see issue #68770).

If you're interested in pseudo-tty's (pt's) take a look at 
http://wiki.netbeans.org/TerminalEmulator

> [JG22] Terminal Emulator

We fight with this in Ruby (likely the same for all having-interactive-interpreters languages and for all alike cases).
We hope this will be solved by the fully-fledged Terminal Emulator, Ivan pointed to, for post-6.5. Thus features like
issue 133994 might be implemented easily.

There will likely be top-level 'terminalemulator' component in IZ. All after 6.5 is code-frozen and works on next
release starts, I guess.

JG21: As there is no client that would require column matching (ruby as upcoming client does not use it either) I would
preserve the current state as column matching can be added later in a compatible way.
JG22: Terminal emulator should be right choice for interactive console apps (once finished). For now the extexecution
provides the reasonable level of support (used for grails console). It provides processors which do not wait for EOL.
Although there are buffered stream on the way, this works reasonably. Afaik similar approach should work for ruby as well.

If there are no further objections, I will make it public by applying changes described in desc4.

Before making it public I would like to apply the following patch. Contains these changes (based on ruby experience with
the new api):

- semantic of LineConverter changed - allows null return value to signal that it does not handle the line. Chain
converter removed from factory methods for LineConverter. Added proxy(LineConverter...) wrapping up multiple converters.
This soultion provide better flexibility.
- ConvertedLine is a final class
- ExecutionDescriptor.InputProcessorFactory - method signature changed from "InputProcessor newInputProcessor()" to
"InputProcessor newInputProcessor(InputProcessor defaultProcessor)" providing access to infrastructure provided processor.

Created attachment 73122 [details]
proposed change

Apologies for coming into this late, but I'm also interested, as we use our own internal code to execute start/stop and
administer of MySQL server.

An enhancement request: often you need superuser access to execute the MySQL process.  It would be very useful to be
able to indicate that an execution needs to be run as superuser, and this API handles bringing up the appropriate
OS-specific UI to get the su password, if supported (e.g. gksu on Linux/Solaris and AppleScript's "with administrator
privileges" on Mac).

Hi David,
I do not think it is a proper place for such API. External execution is used just for executing the process in NetBeans
and handle output. I think it is a lot of system specific code so I wouldn't put it into public API at the first place.
However the API you suggest can be easily built on top of the execution API. You can make it friend and design it
properly. Later, if we would change our mind we can easily integrate it as support.

P.

If there are no objections I will integrate suggested patch tomorrow.

Patch applied to main ae3ebc384d4d.

Fixed friends in contrib 81fbadfe67b8.

Integrated into 'main-golden', will be available in build *200811061401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/ae3ebc384d4d
User: phejl@netbeans.org
Log: #136929 Make Execution API public (applied improvements in api)

One last (fully compatible) change I would like to push before integration. Allows client to select preferred output
window printing type (char - immediate, line - line by line).

Created attachment 73819 [details]
proposed change

I would like to push mentioned change tomorrow. Any objections?

To make a small summary:
The recent changes were valid use cases revealed during the ruby's migration (so nothing like making API better without
any reasonable justification ;) ). As the migration is finished I do not expect any further changes and I would like to
(finally) repackage and publish APIs this week.

Pushed the last patch to main 7bb873de7bb6.

Integrated into 'main-golden', will be available in build *200811200201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/7bb873de7bb6
User: phejl@netbeans.org
Log: #136929 Make Execution API public (desc 35)

Refactored to API packages, moved to the ide cluster - main ba1644ac124f.

Done.

Integrated into 'main-golden', will be available in build *200811220201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/ba1644ac124f
User: phejl@netbeans.org
Log: #136929 Make Execution API public (refactored to api packages)

In updating to the trunk I updated some code which uses the extexecution api.

I was really surprised by a couple of things.

I had code which did something like this:

     descriptor.foo(true);
     descriptor.bar(false);
     descriptor.baz(5);

Since the "setter" methods return an ExecutionDescriptor, I figured this was just the chaining pattern - I could do

    descriptor.foo(true).bar(false).baz(5);

In any case, neither the first alternative nor the second alternative worked! (The second alternative would have worked if I had assigned the result to 
descriptor).

That's because each "setter" method actually duplicates the entire object and passes a new object back!

I think that's pretty strange.  If you really want an immutable descriptor I think you should have a Builder object as an inner class; the Builder is mutable. 
You do something like new ExecutionDescriptor descriptor = ExecutionDescriptor.Builder().foo(true).bar(false).baz(5).build();
In other words, all the setter methods are on the Builder class; the ExecutionDescriptor has only getters. The Builder class has a build() method which 
constructs the immutable.

Right now, there's (a) a potential of incorrect code where users don't realize they have to copy out result and use it instead, e.g. you have to write code 
like this:
 
   desc = new ExecutionDescriptor()
   desc = desc.foo(true)
   desc = desc.bar(false)
   desc = desc.baz(5)

If you just call foo() and bar() the code won't work (I made that mistake).

(b) This is very inefficient. 

I looked again and the case I had a problem with was actually ExternalProcessBuilder. It has the same usage pattern -- you have to use the return value. I 
see some internal BuilderData state which I think deals with sharing data among these copies but it still has to be inefficient if you want to support 
isolation among the different partially constructed objects.

In my opinion, it would be much cleaner to pull this into a separate Builder innerclass.

The other thing I ran into, which is why I was poking in these classes in the first place, is that I need to support -multiple- post execution hooks. In the 
end I worked around it by making my own new post hook delegate to the previous post hook, but it would be better if these were lists such that I could 
simply add as many post hooks as I need.

a) This is just a matter of personal preference imo - see JG04. It is Jarda's "cumulative factory" pattern.

b) It's inefficient only when it is used inefficiently. The common case is you always execute something with the same
descriptor, perhaps changing one or two parameters. Because of the immutability you can use only one instance for the
any number of (even concurrent) executions.

private static final ExecutionDescriptor DESC = new ExecutionDescriptor().foo(true).bar(false).baz(5);

// no instance created ever
ExecutionService service = ExecutionService.newService(processBuilder, DESC, "command");

// or with changed parameter - one instance
ExecutionService service = ExecutionService.newService(processBuilder, DESC.baz(10), "command");

Anyway if this become a performance bottleneck we can introduce builder compatibly any time later.

+1 on tor's objection. The common pattern I've seen elsewhere (outside of netbeans codebase) is to always return the
same instance to allow for chaining. 

IMHO it's not about performance primarily but API usability. From my usecases I don't see why I would like to keep
around and immutable instance around that I change one or two parameters each time. It's always a "fire-and-forget"
approach. Construct and run. Next time do the same again.

You don't have to keep an instance around. You can use such approach in case you don't think generation collector is
fast enough. If we want to keep it immutable (and I hope so - there are too many threads in this api) we can't return
the same instance - only builder would help.

I don't fully understand your argument about the chaining - you can use it (and I would even recommend it):

ExecutionDescriptor desc = new ExecutionDescriptor()
    .foo(true)
       .bar(false)
          .baz(5);

I'm bit frustrated as it seems the api review process does not work - this issue was opened for months without any
objection on this topic (except Jesse's suggestion). Anyway feel free to file a separate api issue with builder patch.

The current API seems fine to me, though I would not strongly object to Tor's suggested syntax. (I would object to
having _both_ styles at once.) ExecutionDescriptor is already in a sense a builder for ExecutionService. Perhaps adding
@UseReturnValue (or whatever the right annotation is) to the existing ED methods would be helpful.

JG04 was (a) a suggestion for making the API slimmer and simpler by omitting the separate builder class, (b) a request
to remove the public getters which served no clear function, (c) a request to correct the Javadoc. The difference in
opinion is I guess over (a) only.

Sorry, I didn't review this API. But I've gotta say I am not a believer in the "cumulative factory" pattern, at least
not as applied here.  I read the writeup (http://wiki.apidesign.org/wiki/Builder) and I don't think this is a good fit.
I think Builders tend to be stateful, and I haven't seen any uses around process execution for example where you have a
template that you keep instantiating. Rather than keeping a half-built instance around that you keep templating from, I
think it would be more natural to create a shared factory method (say RubyExecution.createRubyDescriptor()) which
constructs the basic descriptor before custom handling (say adding test output recognizers for unit test execution) are
added. My main concerns here is that (a) this is not a common way to do it, and (b) this can lead to unexpected errors
where you think you're chaining calls and you were supposed to handle the result value. Yes, we can use a findbugs
annotation to help detect this, but that won't help anybody, and I'm not sure I understand the benefits of this
approach. Immutable objects are good, but for an object builder (what else is an ExternalProcessBuilder?) I think
statefulness is expected, and I really don't think these objects will be used where immutability is most helpful
(sharing objects, concurrency, etc).

Anyway, the review is over etc. so do what you want with my feedback. I made a mistake using the API so I thought others
might have the same misunderstanding, and I wasn't sure how final you considered the API to be.  (I think it would be
really useful to add the second thing I asked for -- a way to register -multiple- post execution hooks - and for
symmetry, perhaps multiple pre execution hooks too.)