This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Native execution has too large list of frienda. Also it going to be used by ide cluster (see Bug #199806). Please start api review process for module.
Created attachment 122823 [details] javadoc of proposed API
Created attachment 122824 [details] arch.html
Team, I would like to propose a new API/SPI for local/remote execution that could be made public. The idea is to provide a module with small number of dependencies that could be used for low-level execution tasks (connect to a host and start a process there). I would like to start with a minimal set of features. The proposed module doesn't provide any UI - it's going to be another module that should provide own API for managing connections, getting common UI, etc.. The API module doesn't provide a real implementation - but there is an impl module that uses described API/SPI to provide execution functionality for local hosts and remote hosts (accessed via ssh with the aid of jsch). The proposed API is not fully compatible with the dlight.nativeexecution's one. The plan is to gradually migrate from that friends-public API to the new one. The main fundamental difference is a switch from the ExecutionEnvironment to a Connection (identified by an URI) for specifying a target execution environment.
It is great to see this effort. I also appreciate the conservative approach. However I have a couple of notes. [PH01] I don't see any interoperability with existing extexecution. At least certain parts are inspired by it. I would expect the nativeexecution could be part of richer extexecution. I understand that might not be possible. However in such case it should be interoperable with extexecution. Otherwise there will be two separate "universes". For me this would be TCR. Looks like certain aspects of org.netbeans.modules.nativeexecution.util could be refactored this way. [PH02] I don't see any reason for exceptions being inner classes of their base class. [PH03] Connection.State and ConnectionStateChangeEvent.State. Can't you merge it to one enum? [PH04] Statefull events are bit tricky in multithreaded enviroment. Would it be possible to use ChangeListener instead of ConnectionStateChangeListener? [PH05] ConnectionUtil is a bit strange (well every class ending Util being in API is strange :) ). Could it be replaced with non static methods on Connection? Or static methods on ConnectionManager? [PH06] Is the NativeProcess.getPID() really needed by clients? I'm sure it is needed internally, but is there a use case for API client? [PH07] ProcessStateChangeEvent.State should be refactored to NativeProcess.State. [PH08] Would it be possible to replace ProcessStateChangeListener with ChangeListener? I'm not sure who is using nanotime though. So it might not be possible. [PH09] I believe the convention in NB is to use SomethingImplementation rather than SomethingImpl as a name for SPI interface.
Pert, great to see you comments! Thanks! > [PH01] I don't see any interoperability with existing extexecution. Give me some time to think about this once again... > [PH02] I don't see any reason for exceptions being inner classes of their base class. you mean moving ConnectionCancelledException and others? Yes, I can refactor this. > [PH03] Connection.State and ConnectionStateChangeEvent.State. Can't you merge it to one enum? Originally there was only one State enum.. But then my idea was to make SPI implementor's life easier by just expecting a binary answer from them - either connection is active or not... Infrastructure (it's part where it handles connections), in turn, produces 3 different state changed events - connection established/closed/lost... Maybe (as long as there are just 2 states for a connection) we can just remove the Connection.State at all (along with Connection.getState() method)? > [PH04] Statefull events are bit tricky in multithreaded enviroment. Would it be possible to use ChangeListener instead of ConnectionStateChangeListener? The order of state change notifications is guaranteed by the infrastructure. And I wanted to have a distinguish between connection_lost and connection_closed events.. This difference (now) is 'visible' as a state of the event only... > [PH05] ConnectionUtil is a bit strange (well every class ending Util being in API is strange :) ). Could it be replaced with non static methods on Connection? Or static methods on ConnectionManager? Yes, I'll do that. Thanks. Originally I took this approach from the FileUtil, but after a while the only methods left were add/remove listeners... I'll move them to the ConnectionManager. > [PH06] Is the NativeProcess.getPID() really needed by clients? I'm sure it is needed internally, but is there a use case for API client? Hm.. I would say 'yes'.. We use pids for process selection UI, for processes profiling UI, for sending signals, some others... > [PH07], [PH08] same as [PH03], [PH04] ? > [PH09] I believe the convention in NB is to use SomethingImplementation rather than SomethingImpl as a name for SPI interface. Oh, I see. Will do. Thanks.
(In reply to comment #5) > > [PH06] Is the NativeProcess.getPID() really needed by clients? I'm sure it is > needed internally, but is there a use case for API client? > Hm.. I would say 'yes'.. We use pids for process selection UI, for processes > profiling UI, for sending signals, some others... Why not to use the NativeProcess object itself? BTW I haven't gone through the entire API so I may post some additional comments once I 'll have some more time.
(In reply to comment #6) > > Why not to use the NativeProcess object itself? > .. to display a PID we need to get it from somewhere ;) > > BTW I haven't gone through the entire API so I may post some additional > comments once I 'll have some more time. SURE!
Created attachment 122879 [details] javadoc of proposed API Addressed comments: [PH02] inner classes moved to outer level [PH03] (use isConnected() instead of getState()) [PH05] (add/remove listeners moved to the ConnectionManager) [PH09] renamed to *Implementation
(In reply to comment #5) > Pert, > > great to see you comments! Thanks! > > > [PH01] I don't see any interoperability with existing extexecution. > > Give me some time to think about this once again... Looking in the code - as a starting point we might try to return ProcessBuilder (extexecution class) from Connection.newProcessBuilder(). Of course that might need some changes on both sides. Would be nice if you could provide a full patch so I can play with that. > > > [PH02] I don't see any reason for exceptions being inner classes of their base > class. > > you mean moving ConnectionCancelledException and others? Yes, I can refactor > this. Yes please. > > > [PH03] Connection.State and ConnectionStateChangeEvent.State. Can't you merge > it to one enum? > > Originally there was only one State enum.. But then my idea was to make SPI > implementor's life easier by just expecting a binary answer from them - either > connection is active or not... Infrastructure (it's part where it handles > connections), in turn, produces 3 different state changed events - connection > established/closed/lost... Maybe (as long as there are just 2 states for a > connection) we can just remove the Connection.State at all (along with > Connection.getState() method)? I would keep Connection.getState(), but use only one enum in both places as is the case with NativeProcess. Is that doable?
(In reply to comment #7) > (In reply to comment #6) > > > > Why not to use the NativeProcess object itself? > > > > .. to display a PID we need to get it from somewhere ;) Oh, I see. > > > > > BTW I haven't gone through the entire API so I may post some additional > > comments once I 'll have some more time. > > SURE!
Regarding PH01. I think it could be possible to replace the NativeProcess, NativeProcessBuilder and NativeProcessImplementation with similar classes in extexecution. Of course it is not possible as it is now. I believe we could slightly modify extexecution in compatible way to achieve that and benefit from things extexecution provides. In general it could be something like this: NativeProcess -> API final class Process (new one) extending java.lang.Process NativeProcessBuilder -> existing API class ProcessBuilder (we would need to check what is really needed and how to provide that, change the builder to work with the new class Process) NativeProcessImplementation -> SPI class ProcessImplementation (new one) which would be used by updated ProcessBuilderImplementation Other thing using java.lang.Process in extexecution API could stay and user could use them even with the new Process/ProcessBuilder. That's the idea - I'm sure there will small and bigger things to solve along the way to achive that. What do you think?
Andrew can you send me a full patch, so I can play with it? I think I have an idea how to stick with java.lang.Process in the API. In this way we could possibly enhance existing ProcessBuilder in extexecution so there would be no need for NativeProcess, NativeProcessImplementation and NativeProcessBuilder. Wouldn't that be a great integration?
(In reply to comment #9) > > Originally there was only one State enum.. But then my idea was to make SPI > > implementor's life easier by just expecting a binary answer from them - either > > connection is active or not... Infrastructure (it's part where it handles > > connections), in turn, produces 3 different state changed events - connection > > established/closed/lost... Maybe (as long as there are just 2 states for a > > connection) we can just remove the Connection.State at all (along with > > Connection.getState() method)? > I would keep Connection.getState(), but use only one enum in both places as is > the case with NativeProcess. Is that doable? Sure it is. But this requires other modifications as well... (let's discuss and the come up with what we will do) - if we give a way for SPI implementor to return CONNECTION_LOST state in getState() method of connection then we need either check periodically for this state and notify listeners or give a way for SPI implementor to notify listeners by itself.. I would prefer not to expose listeners to SPI implementors. Currently there is already a loop that queries for connection's state and if the connection was expected to be in connected state (no implicit disconnect was called), then CONNECTION_LOST event is generated.. We may use the same loop and react on CONNECTION_LOST connection state returned from getState(), but for me, it is a way for un-synchronization.. Please, take a look at the ConnectionListeners class... (in attached patch)
Created attachment 122920 [details] patch with api/impl
Unfortunately I'm not able to compile with the patch applied. So following statements might not be accurate. FYI the compilation error is : [nb-javac] Compiling 123 source files to /home/sickboy/workspace/netbeans-web/dlight.nativeexecution/build/classes [cleanall] /home/sickboy/workspace/netbeans-web/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/JschSupport.java:58: class JSchSupport is public, should be declared in a file named JSchSupport.java [cleanall] public final class JSchSupport { [cleanall] ^ [cleanall] /home/sickboy/workspace/netbeans-web/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/jsch/JSchChannelsSupport.java:68: duplicate class: org.netbeans.modules.nativeexecution.impl.jsch.JSchChannelsSupport [cleanall] public final class JSchChannelsSupport { [cleanall] ^ [cleanall] /home/sickboy/workspace/netbeans-web/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/api/util/ConnectionManager.java:66: cannot access org.netbeans.modules.nativeexecution.jsch.JSchChannelsSupport [cleanall] bad class file: RegularFileObject[/home/sickboy/workspace/netbeans-web/dlight.nativeexecution/src/org/netbeans/modules/nativeexecution/jsch/JSchChannelsSupport.java] [cleanall] file does not contain class org.netbeans.modules.nativeexecution.jsch.JSchChannelsSupport [cleanall] Please remove or make sure it appears in the correct subdirectory of the classpath. [cleanall] import org.netbeans.modules.nativeexecution.jsch.JSchChannelsSupport;
[PH10] There does not seem to be any reason for EnvironmentMap/MacroMap hierarchy. I think these should be merged to EnvironmentMap. [PH11] Are all methods in EnvironmentMap/MacroMap required? For example I do not see any usage of clear(). Should be removed from API. [PH12] NativeProcessBuilder.getSupportedProperties() does not seem to be used anywhere. Should be removed. [PH13] NativeProcessBuilder use two approaches. One is setSomething() and the other is getEnvironmentMap().put() I think this should be unified to setEnvironmentMap(). [PH14] I checked for usage NativeProcessBuilder.addProcessListener() and is seems to be used to check the process state (and it uses ChangeListener which I like ;) ). For me it would be much more logical if addProcessListener would be on NativeProcess itself.
(In reply to comment #16) > [PH10] There does not seem to be any reason for EnvironmentMap/MacroMap > hierarchy. I think these should be merged to EnvironmentMap. Well... MacroMap is more 'generic' and can be used not only for Environment setup.. It could have own keys comparator, while environment map (which is used for setting an environment where process is executed (in sense of OS) has some kind of 'restrictions' and 'specific features'.. If you believe that the difference doesn't worth 2 classes, then, well.. I'm OK with merging them together > [PH11] Are all methods in EnvironmentMap/MacroMap required? For example I do > not see any usage of clear(). Should be removed from API. If we want to have some parity with java's ProcessBuilder, which has environment() method that returns a map that has clear() method, then we may want to leave this functionality... IMO this is expected.. And in some cases this could be useful - users may want to start a process in a minimal possible environment... What do you think? > [PH12] NativeProcessBuilder.getSupportedProperties() does not seem to be used > anywhere. Should be removed. Not now, but there could be users of this API. For example, there could be usage like: if (npb.getSupportedProperties().contains("outputCharset")) { // create UI for selecting a charset // or just set charset to some-value } This is especially could be useful when creating some UI for configuring how processes will be started.. > [PH13] NativeProcessBuilder use two approaches. One is setSomething() and the > other is getEnvironmentMap().put() I think this should be unified to > setEnvironmentMap(). You mean setProperty()? this is used for builder-specific configuration.. Like, say, if scheme is "hudson://", which creates and starts some hudson job may have "timeout" property, this timeout could be not applicable for ssh:// scheme... From the other hand configuring everything using properties is not handy.. There are some very generic (applicable in most cases) parameters: setCommand, setWorkingDir, redirectError. (all are taken form java). More interesting (questionable) method is setShellScript() method... This was really a hard call.. because this introduces two ways for setting up what to execute (either via setCommand() or via this setShellScript()). setShellScript() may look strange for people who are on Windows.. But it really solves many problems with passing arguments, doing pipes and re-directions that are just not configurable via setCommand(). One may argue that it is possible to use setCommand("sh", "-c", "a script with any re-rirection"), but this approach could have more side effects when there is non trivial argument (spaces, quotes, $ signs, dashes, etc). > [PH14] I checked for usage NativeProcessBuilder.addProcessListener() and is > seems to be used to check the process state (and it uses ChangeListener which I > like ;) ). For me it would be much more logical if addProcessListener would be > on NativeProcess itself. In some cases it is handy to have this listener created *before* process exists. In this case we get notifications when process was started. Also in this case we get notifications when process was not able to be started.
(In reply to comment #15) > Unfortunately I'm not able to compile with the patch applied. So following > statements might not be accurate. > FYI the compilation error is : > [nb-javac] Compiling 123 source files to > /home/sickboy/workspace/netbeans-web/dlight.nativeexecution/build/classes > [cleanall] Hm.. strange. And your errors are in dlight.nativeexecution... ( which is not touched in the patch) What is your repo/tip?
(In reply to comment #12) > Andrew can you send me a full patch, so I can play with it? I think I have an > idea how to stick with java.lang.Process in the API. In this way we could > possibly enhance existing ProcessBuilder in extexecution so there would be no > need for NativeProcess, NativeProcessImplementation and NativeProcessBuilder. > Wouldn't that be a great integration? Right, that would be great. But I would like not to lose the flexibility for ProcessBuilder configuration ;)
(In reply to comment #17) > (In reply to comment #16) > > [PH10] There does not seem to be any reason for EnvironmentMap/MacroMap > > hierarchy. I think these should be merged to EnvironmentMap. > > Well... MacroMap is more 'generic' and can be used not only for Environment > setup.. It could have own keys comparator, while environment map (which is used > for setting an environment where process is executed (in sense of OS) has some > kind of 'restrictions' and 'specific features'.. > > If you believe that the difference doesn't worth 2 classes, then, well.. I'm OK > with merging them together Well, if it is not used in API (MacroMap) it should not be visible. So I would vote for merging or composition. > > > [PH11] Are all methods in EnvironmentMap/MacroMap required? For example I do > > not see any usage of clear(). Should be removed from API. > > If we want to have some parity with java's ProcessBuilder, which has > environment() method that returns a map that has clear() method, then we may > want to leave this functionality... IMO this is expected.. And in some cases > this could be useful - users may want to start a process in a minimal possible > environment... What do you think? Please do not put things into the API which might be useful. There should be real usage. I learned myself that such guess is rarely right. In API designed for evolution you can add it anytime later. > > > [PH12] NativeProcessBuilder.getSupportedProperties() does not seem to be used > > anywhere. Should be removed. > > Not now, but there could be users of this API. For example, there could be > usage like: > if (npb.getSupportedProperties().contains("outputCharset")) { > // create UI for selecting a charset > // or just set charset to some-value > } Same as PH011. I would also think that might be placed elsewhere though I'm not really sure. > > This is especially could be useful when creating some UI for configuring how > processes will be started.. > > > [PH13] NativeProcessBuilder use two approaches. One is setSomething() and the > > other is getEnvironmentMap().put() I think this should be unified to > > setEnvironmentMap(). > > You mean setProperty()? this is used for builder-specific configuration.. Like, > say, if scheme is "hudson://", which creates and starts some hudson job may > have "timeout" property, this timeout could be not applicable for ssh:// > scheme... > From the other hand configuring everything using properties is not handy.. > There are some very generic (applicable in most cases) parameters: > setCommand, setWorkingDir, redirectError. (all are taken form java). Not really. What I meant is there are methods directly setting up things like setProperty, setCommand, setWorkingDir, redirectError and on the other hand method returning some inner state you later modify (getEnvironmentMap()). I would use the same approach across the class so I would expect to see setEnvironmentMap or better setEnvironment. > > More interesting (questionable) method is setShellScript() method... > This was really a hard call.. because this introduces two ways for setting up > what to execute (either via setCommand() or via this setShellScript()). > setShellScript() may look strange for people who are on Windows.. But it really > solves many problems with passing arguments, doing pipes and re-directions that > are just not configurable via setCommand(). One may argue that it is possible > to use setCommand("sh", "-c", "a script with any re-rirection"), but this > approach could have more side effects when there is non trivial argument > (spaces, quotes, $ signs, dashes, etc). Yep. Something I would like to think about more with your help. > > > [PH14] I checked for usage NativeProcessBuilder.addProcessListener() and is > > seems to be used to check the process state (and it uses ChangeListener which I > > like ;) ). For me it would be much more logical if addProcessListener would be > > on NativeProcess itself. > > In some cases it is handy to have this listener created *before* process > exists. In this case we get notifications when process was started. Also in > this case we get notifications when process was not able to be started. Ok.
(In reply to comment #20) > > If we want to have some parity with java's ProcessBuilder, which has > > environment() method that returns a map that has clear() method, then we may > > want to leave this functionality... IMO this is expected.. And in some cases > > this could be useful - users may want to start a process in a minimal possible > > environment... What do you think? > Please do not put things into the API which might be useful. There should be > real usage. I learned myself that such guess is rarely right. In API designed > for evolution you can add it anytime later. > OK, let's hide this from public for now. > > > > > [PH12] NativeProcessBuilder.getSupportedProperties() does not seem to be used > > > anywhere. Should be removed. > > > > ... > > } > Same as PH011. I would also think that might be placed elsewhere though I'm not > really sure. I bet this will be needed very soon ;) But let's hide this for now as well > > .. > > From the other hand configuring everything using properties is not handy.. > > There are some very generic (applicable in most cases) parameters: > > setCommand, setWorkingDir, redirectError. (all are taken form java). > Not really. What I meant is there are methods directly setting up things like > setProperty, setCommand, setWorkingDir, redirectError and on the other hand > method returning some inner state you later modify (getEnvironmentMap()). I > would use the same approach across the class so I would expect to see > setEnvironmentMap or better setEnvironment. Oh, I see. I thought about this, but at the end came up with this "getMap and modify it" approach (as in java's builder). I'm not insisting on it, but just a note, that users will not only add their variables to the processBuilder's env, but also will need to remove some (and maybe test for existence). But for sure the most common use is just put some variable into the env... So.. setEnvironment(String varName, String varValue), then?
(In reply to comment #21) > (In reply to comment #20) > So.. setEnvironment(String varName, String varValue), then? I original meant setEnvironment(EnvironmentMap), but if setEnvironment(String varName, String varValue) is enough that would be great. Another specific detail (EnvironmentMap) would be hidden which would make API easier to understand imo. Perhaps removal of variable could be solved by setting variable value to null - setEnvironment(varName, null). What do you think?
FYI look into org.netbeans.api.extexecution.ProcessBuilder. If we get signatures and semantics of the NativeProcessBuilder close enough to ProcessBuilder it could be an SPI implementation of this one. Of course also the org.netbeans.api.extexecution.ProcessBuilder has to be extended somehow. Just to give you the whole picture...
(In reply to comment #22) > > > So.. setEnvironment(String varName, String varValue), then? > > I original meant setEnvironment(EnvironmentMap), but if setEnvironment(String > varName, String varValue) is enough that would be great. Another specific > detail (EnvironmentMap) would be hidden which would make API easier to > understand imo. Perhaps removal of variable could be solved by setting variable > value to null - setEnvironment(varName, null). What do you think? Hm.. I just realized that in this case we will lost appendPathVariable/prependPathVariable methods of EnvironmentMap, which are very handy and useful! And, actually, that was one of the strongest argument for using the current design...
(In reply to comment #23) > FYI look into org.netbeans.api.extexecution.ProcessBuilder. If we get > signatures and semantics of the NativeProcessBuilder close enough to > ProcessBuilder it could be an SPI implementation of this one. Of course also > the org.netbeans.api.extexecution.ProcessBuilder has to be extended somehow. > Just to give you the whole picture... Please keep in mind one requirement that I had when designed nativeexecution - the minimal possible dependency on NB.. I like the idea of integration with extexecution, but it will be great if we could make it so that extexecution depends on nativeexecution, but not vice versa.. Now nativeexecution is almost self-contained module..
(In reply to comment #24) > (In reply to comment #22) > > > > > > So.. setEnvironment(String varName, String varValue), then? > > > > I original meant setEnvironment(EnvironmentMap), but if setEnvironment(String > > varName, String varValue) is enough that would be great. Another specific > > detail (EnvironmentMap) would be hidden which would make API easier to > > understand imo. Perhaps removal of variable could be solved by setting variable > > value to null - setEnvironment(varName, null). What do you think? > > Hm.. I just realized that in this case we will lost > appendPathVariable/prependPathVariable methods of EnvironmentMap, which are > very handy and useful! And, actually, that was one of the strongest argument > for using the current design... Maybe it could be solved separately: see org.netbeans.api.extexecution.ProcessBuilder.setPaths() and org.netbeans.api.extexecution.ProcessBuilder.setEnvironmentVariables(). It seems to be quite close. BTW looking into prependPath() and appendPath() why these have parameter name? One would expect just one parameter.
(In reply to comment #26) > > Maybe it could be solved separately: see > org.netbeans.api.extexecution.ProcessBuilder.setPaths() and > org.netbeans.api.extexecution.ProcessBuilder.setEnvironmentVariables(). It > seems to be quite close. > > BTW looking into prependPath() and appendPath() why these have parameter name? > One would expect just one parameter. This is because this method is about helping to append/prepend value to any PATH-like variable (like LD_PRELOAD, LO_LIBRARY_PATH, etc...) So setPath() is not quite the same...
I am glad you are working on providing official API for remote execution (which was missing so far). I like that you are both cooperating on finding the best way to merry the existing extexecution API with the new one. I'd like to see Y01 Single official API for (local) execution in the ide cluster Of course, it would be even better if you find a way to merge the remote functionality into the local one, so there would be only single execution API for both cases (with possibly multiple implementations). Thus I'd recommend Y02 Make the single execution API flexible to support both local and remote execution I've noted a request for the execution API to be lightweight. I am not sure what is the driver for this request, but it is obvious that the current extexecution API has richer set of dependencies than necessary. I contribute that to the fact that it also contains implementation. But the set of modules used by API only seem to be just org.netbeans.api.annotations.common, org.openide.io, org.openide.util.lookup, org.openide.filesystems - that seems relatively lightweight. Anyway I support Y03 Lightweight API with possibly heavier independent implementation modules
I was able to apply the patch and compile the IDE (my mistake). However in the patch I do not see any client really using the new API. So from my point of view the API is not yet really ready as: 1) We can't be sure what's missing in the API. 2) We can't be sure what's redundant in the API. 3) We don't have test cases on which we could tune the API and bring it closer to extexecution. Andrew please take your time and use the new API instead of old one for usecases the new API should solve. Please attach the patch one this is done and we can continue our iterations.
Jaroslav, thanks for joining this thread. I absolutely support the idea that API should be a module without implementation (and, for sure, it will be done this way). As for light-weight, I thing that even not org.openide.io nor org.netbeans.api.annotations.common are needed in the API. I would like to come up with the module that don't have any UI. And have another module (extexecution?) that provides UI related things (O/I windows, 'hosts' management/configuration, etc...).. Ideally it would be great to end up with a jar that could do remote execution and is ready to be used in not NB-based applications without a need to take too many other modules... (some of our teams asked many times for this...)
(In reply to comment #29) > I was able to apply the patch and compile the IDE (my mistake). However in the > patch I do not see any client really using the new API. So from my point of > view the API is not yet really ready as: > > 1) We can't be sure what's missing in the API. > 2) We can't be sure what's redundant in the API. > 3) We don't have test cases on which we could tune the API and bring it closer > to extexecution. > > Andrew please take your time and use the new API instead of old one for > usecases the new API should solve. Please attach the patch one this is done and > we can continue our iterations. OK, I will. There are several (few) unit tests that use this API, but of course, they do not cover everything.. So what you think is the best way for doing this? Do you think that it's a time to switch (at least some of our modules (cnd) to use this API)? Maybe we should create a branch for this work?
(In reply to comment #31) > (In reply to comment #29) > OK, I will. > There are several (few) unit tests that use this API, but of course, they do > not cover everything.. A test tests that a part of the API works as expected. It can't test whether such part is needed at all (if not it should not be present at all) or even there are some missing pieces some client needs. > > So what you think is the best way for doing this? Do you think that it's a > time to switch (at least some of our modules (cnd) to use this API)? > Maybe we should create a branch for this work? Branch is ok for me.
(In reply to comment #30) > And have another module > (extexecution?) that provides UI related things (O/I windows, 'hosts' > management/configuration, etc...).. TM01 What does "hosts" and "management/configuration" mean exactly?
(In reply to comment #30) > Jaroslav, > > thanks for joining this thread. I absolutely support the idea that API should > be a module without implementation (and, for sure, it will be done this way). > As for light-weight, I thing that even not org.openide.io nor > org.netbeans.api.annotations.common are needed in the API. I would like to come > up with the module that don't have any UI. And have another module > (extexecution?) that provides UI related things (O/I windows, 'hosts' > management/configuration, etc...).. > > Ideally it would be great to end up with a jar that could do remote execution > and is ready to be used in not NB-based applications without a need to take too > many other modules... (some of our teams asked many times for this...) In that case I would suggest this: - Evaluate possibility of splitting extexecution to limit dependencies (Me with Jarda's help I guess ;) ) - Create the non-NB related JAR library which you aim for. - extexecution will use the library - extexecution will provide access to remote/native functionality while preserving API client investments I guess from such design everybody would benefit. What do you gyus think? Just as a note we really must not harm extexecution users in any way. It is for sure used by products out of our control.
(In reply to comment #33) > TM01 What does "hosts" and "management/configuration" mean exactly? You may take a look at C/C++ pack - it has remote support. There are several UI elements for establishing a new connection, for configuring 'host' properties, etc.. (add/remove host from connections list, look at recently used connections, initiate a connection) Actually, the API/implementation for all that stuff exists in dlight.nativeexecution and is used for several years. The problem is that it has some conceptual drawbacks (i.e. ssh-orientation on an API level.. several others).
(In reply to comment #34) > > In that case I would suggest this: > - Evaluate possibility of splitting extexecution to limit dependencies (Me with > Jarda's help I guess ;) ) > - Create the non-NB related JAR library which you aim for. > - extexecution will use the library > - extexecution will provide access to remote/native functionality while > preserving API client investments > > I guess from such design everybody would benefit. > What do you gyus think? I think this is a good way. > > Just as a note we really must not harm extexecution users in any way. It is for > sure used by products out of our control.
(In reply to comment #32) > > There are several (few) unit tests that use this API, but of course, they do > > not cover everything.. > A test tests that a part of the API works as expected. It can't test whether > such part is needed at all (if not it should not be present at all) or even > there are some missing pieces some client needs. Actually this API provides functionality that is present in dlight.nativeexecution and is used widely in cnd modules. All these methods a proved to be useful ;) > > > > > So what you think is the best way for doing this? Do you think that it's a > > time to switch (at least some of our modules (cnd) to use this API)? > > Maybe we should create a branch for this work? > Branch is ok for me. Could someone on your side create the branch, then? (will it be a separate repo?)
TM01 So, it means that now, we have very similar list of remote hosts on (AFAIK) 3 different places; these are: - CND - PHP - Java ME Could we merge them please? What type of remote host (SSH, FTP etc.) do you plan to manage?
(In reply to comment #38) > TM01 So, it means that now, we have very similar list of remote hosts on > (AFAIK) 3 different places; these are: > - CND > - PHP > - Java ME > > Could we merge them please? What type of remote host (SSH, FTP etc.) do you > plan to manage? That would be GREAT to have a single management for this (I understand that this is a rather long way to go, though). In cnd we use ssh access. In the proposed API I switched to URI for identifying a connection target - this is more flexible than ExecutionEnvironment we use in cnd.. and allows to specify any kind of connection.. I want to see a single hosts management.. But before doing that I would like to get in agreement on a low-level execution api.
(In reply to comment #39) > (In reply to comment #38) > > TM01 So, it means that now, we have very similar list of remote hosts on > > (AFAIK) 3 different places; these are: > > - CND > > - PHP > > - Java ME > > > > Could we merge them please? What type of remote host (SSH, FTP etc.) do you > > plan to manage? > > That would be GREAT to have a single management for this (I understand that > this is a rather long way to go, though). In cnd we use ssh access. In the > proposed API I switched to URI for identifying a connection target - this is > more flexible than ExecutionEnvironment we use in cnd.. and allows to specify > any kind of connection.. > > I want to see a single hosts management.. But before doing that I would like to > get in agreement on a low-level execution api. Guys I think with this we are getting out of scope of *execution API. I think host management would be good, but please file a separate issue for such API. Extexecution would be client of such API I guess.
(In reply to comment #40) > Guys I think with this we are getting out of scope of *execution API. I think > host management would be good, but please file a separate issue for such API. > Extexecution would be client of such API I guess. Yes, that is true. I just wanted to be sure that we will unify our remote hosts management once we get to it. I will submit an issue for it.
(In reply to comment #41) > I will submit an issue for it. Issue #216660, currently assigned to Andrew but I would help with it of course, at least with the PHP part. Thanks.
(In reply to comment #31) > > > > Andrew please take your time and use the new API instead of old one for > > usecases the new API should solve. Please attach the patch one this is done and > > we can continue our iterations. > > So what you think is the best way for doing this? For io/terminal work I created lib.terminalemulator/demosrc/examples in addition to unit tests. Incidentally it is a simple client of nativeexecution and shows various ways of doing i/o plumbing.
Sigh, I have a fair # of comments written down but I'm leaving on vacation for a week so I don't want to post them and then not be able to deal with the fallout.
Andrew wrote in comment #30: > Ideally it would be great to end up with a jar that could do remote execution > and is ready to be used in not NB-based applications without a need to take too > many other modules... (some of our teams asked many times for this...) OK, so the goal is to be able to use the API without NetBeans Runtime Container around, just by putting JARs on classpath. That should be achievable, as that is the environment usually used in unit testing. > As for light-weight, I thing that even not org.openide.io nor > org.netbeans.api.annotations.common are needed in the API. We need to balance between independence and reinventing a wheel. > I would like to come > up with the module that don't have any UI. And have another module > (extexecution?) that provides UI related things (O/I windows, 'hosts' > management/configuration, etc...).. Right no UI. In this context I should however mention that org.openide.io is just an API to abstract I/O visualization. The actual implementations are in core.output2 and terminal. I'd rather reuse the existing I/O abstractions then create new duplicate in the execution API Petr Hejl wrote: > - Evaluate possibility of splitting extexecution to limit dependencies OK. > - Create the non-NB related JAR library which you aim for. I'd hope the non-NB JAR is a regular NetBeans module just executed in classpath mode without NetBeans Runtime Container. Btw. Maintaining embedded J2SE projects inside of hg.netbeans.org repository is usually too much pain with no gain. Let's avoid that.
To put this API into perspective consider this page: http://bugs.sun.com/top25_bugs.do The #1 bug on that list is 4109888 Semantics of external process is not defined in JLS It was created in 1998 and is a kind of an umbrella bug for many other bugs. Tho many of the issues have been resolved it is _still_ open! Many of the issues described pertain to Windows but there are just as many that pertain to Unix and are just not referenced. The age, # of votes and persistence of this bug indicates that it's important. So I'm very glad to see this API as part of NB. -------------------------------------------------------------------------------- I'm only reviewing the API here. [IS01] IMO this API should be part of platform, not ide. It's "system level" and just too basic. [IS02] SignalUtils I'd provide an escape route with the ability to send integer signals. But this can be added later. [IS03] SignalUtils.Signal.getID(OSFamily) - What happens if the signal name is not available on the given OS? E.g. SIGJVM[12] is not available on Linux. [IS04] ProcessUtils.destroy() UNIX apllications use SIGTERM for "graceful shutdown". This is why the default value for kill(1) is SIGTERM. So far ProcessUtils.destroy() does the right thing. However not all applications prefer SIGTERM. Shell's for example prefer SIGHUP and are in fact immune to SIGTERM: bash@A: sh $ /bin/kill -term $$ $ /bin/kill -hup $$ Hangup bash@A: So I'd suggest using SIGHUP as a backup instead of SIGINT (which shells and many other apps, especially interactive ones, _don't_ treat as termination). SIGKILL should be used as a last resort because it's an extremely crude way of terminating stuff. And I'd rather see it in a separate method. So ... ProcessUtils.terminate() SIGTERM followed by SIGHUP ProcessUtils.kill() SIGKILL Guarantees process termination on return. I realise there's an issue here where deciding to use kill() after terminate() fails ends up complicating things for the client. [IS05] Missing proper handling of wait status. This is the last remaining process control feature that "Java should've done but didn't" In unix, processes can exit with an exit value or "die" because of a signal like SIGSEGV. java.lang.Process.exitValue() doesn't distinguish these two cases and conflates exit values with signals. As a result under CND, when novice programmers have a bug in their program that causes a SIGSEGV they get: Program exit with value 11. Instead of Program dies because of signal SEGV. It's a true FAQ on users.cnd and I wish we could do something to help the novices. Implementing this efficiently is not easy though. One way would be to create a proxy process that does the waiting and sends info back but this would break interoperability with java.lang.Process. And you know what? I don't mind seeing that interoperability break! [IS06] Connection.isConnected() Does it make sense in an MT world to ask questions about mutable properties? Suppose you have: if (c.isConnected()) { doSomething(); doSomethingElse(); } and the connection vanishes between doSomething() and doSomethingElse(). [IS07] connection state. We have CLOSED, ESTABLISHED, LOST. What is the initial state? When ConnectionManager.connect() returns a Connection what are the possible states? Will it not always return an ESTABLISHED connection? In other words, can I start playing with a Connection as soon as I get it or do I have to wait for the ESTABLISHED event? My favorite way of doing this kind of stuff is to use "continuation"s: ...connect(ConnectContinuation) interface ConnectContinuation { void conenctionSucceeded(ConnectionInfo); void conenctionFailed(ConnectionInfo); } or some other variation. [IS08] I don't see a Connection.close() or ConnectionManager.close() so how does the state end up being CLOSED? [IS09] Is there any value in keeping ne.api and ne.api.process as separate packages? Sort of funny to have ne.api.process do most of the referencing to EnvironmentMap which is defined in ne.api. And one can get a better overview of just the API w/o jumping btw package docs. [IS10] NativeProcessBuilder.setProperty(). Some "motivating examples" for the kinds of properties would be useful: I imagine one use might be as follows? setProperty("USE_PTY", true); [IS11] NativeProcess.getProperty(). Doesn't exist but perhaps it could be used for solving the exit vs terminating signal problem (assuming the implementation problems are solved)? getProperty("EXIT_VALUE"); getProperty("TERMSIG"); [IS12] NativeProcessBuilder.setCommand() - Does 'arguments' include argv[0] or is 'executable' assigned to argv[0]? Consider lost flexibility for applications which change behaviour based on argv[0]. - Any value to separating setCommand() into setExecutable() and setArguments()? extexecution went even further and bound the executable to the builder/factory as an immutable construction parameter and I sort of like that too. - Re the name "command". To me it evokes something we give a shell and is usually of the form: "X=Y cat -i foo < in >& out". - Often the _user_ input is just a long string: "cat -i foo". At some point, before it reaches execv(2), it has to be split into words. Who should do that? - Java client -> need setArguments(Collection<String>) - Helper shell ("sh -c etc") -> need setArguments(String line); Here setCommand() may be more appropriate. And then need to be able to set the shell? (Not the same as setShellScript) [IS13] Allow arguments (e.g. in setCommand()) as a Collection as well. [IS14] A motivating reason for setShellScript() would help. I was going to question it's need because, after all, one does this by putting "#!/bin/sh" as the first line of shell scripts. But that would require that one upload the said script which is tedious. So setShellScript() will feed a composed-on-the-spot script via input streams and not upload any files, right? [IS15] Mixing of immutable setters style and mutable properties. I don't really know what the conventions are for such situations. Consider this code: NativeProcessBuilder npb = ...; EnvironmentMap em = npb.getEnvironmentMap(); npb = npb.redirectErrorStream(true); sm.set...(); // won't be applied to the latest npb. Ergo all methods of EnvironmentMap shouldreally be immutable mathods of NativeProcessBuilder? [IS16] EnvironmentMap.appendPath()/prependPath()? What is a "path"? Would I say appendPath("PATH", "/bin:/usr/bin:/usr/openwin/bin:"); or appendPath("PATH", "/bin"); appendPath("PATH", "/usr/bin"); appendPath("PATH", "/usr/openwin/bin:"); If the latter then one should use the word pathElement. [IS17] EnvironmentMap.createCasePreserving/Sensitive(). - Is it the case that all PATH-like variables will use the same pathSeparator? - What is the default pathSeparator when we get an EnvironmentMap from NativeProcessBuilder.getEnvironmentMap()? - Shouldn't these be in the SPI? [IS18] Re use of clone (e.g. in MacroMap). I thought the general recommendation (Bloch et al "Effective Java") was to avoid clone() and use a "copy constructor" idiom X newX = new X(oldX) [IS19] If NativeProcessBuilder gets it's copy of EnvironmentMap from a Connectio n, why can't I access and modify a Connections EnvironmentMap? [IS20] MacroMap vs expandMacros vs EnvironmentMap My understanding is that "macro"s are syntax used in CND like this: ${working-dir}/a.out and are visible in user editable fields. They have nothing to do with UNIX environment variables so I don't think MacroExpander belongs in this API. Also that EnvironmentMap inherits from MacroMap is somewhat conceptually confusing. Would you pass an EnvironmentMap to a MacroExpander? [IS21] expandMacros() takes a String and returns a String, so how does one then convert the String to a series of arguments to pass to NPB.setCommand? [IS22] ProcessUtils.ExitStatus - Can you add a teeny reference to ProcessUtil.execute() in the javadoc for ExitStatus? - Under what circumstances are 'error' and 'output' available? I.e. is what gets sent to an ProcessOutputProcessor also get copied to 'error' and 'output'? If the ProcessOutputProcessor's are null will 'error' and 'output' still be filled? - Are the field values for 'error', 'exitCode' and 'output' final? [IS23] ProcessOutputProcessor The output received from the process has to go _somewhere_ but the way ProcessUtils.execute() starts new threads there's no way to pass some info to the ProcessOutputProcessor regarding what should be done with the output which means one will have to depend on global data? [IS24] Don't TasksCachedProcessor and Computable belong to nativeexecution.spi.support or something? [IS25] You're "done" with a process not only when it has exited (e.g. waitFor()) has returned but also after all output which is still in the pipe has been drained and processed. For an example see org.netbeans.lib.terminalemulator.StreamTerm.connect() and disconnect(). Hmm maybe this should stay in the realm of IO and terminals.
Ivan, thank you for your comments! > [IS01] ide vs. platform I don't mind ;) > [IS02] int signals > [IS03] SignalUtils.Signal.getID(OSFamily) Do you know if we could really face the problem when proposed enum is not enough? Maybe in this case it would be better get rid of this enum and to use Strings/ints on API level and let SignalSupportProviders deal with them as they need? > [IS04] ProcessUtils.destroy() Of course I meant SIGTERM! It's a typo... Thanks for catching! I can add SIGHUP as well. As for separate methods... Maybe (to make client's life easier) we can add a parameter to the destroy() method that tells whether process should be killed or not - some integer that could denote a time to for wait normal termination before killing? > [IS05] (exit value vs. signaled) Do you mean that exitValue() and waitFor() should return not just int? Do you think this is a common case when users of the API should be able to distinguish the=is situation? Perhaps in most cases they just need to get ok/"not ok" status? And for those who need this understanding we may provide something build on top of this execution API? (a way to start a process with a proxy process that will return detailed exit information)? > [IS06] Connection.isConnected() Yes, but this cannot be made atomically.. Connection could vanish at any moment... Exception will be thrown in this case. Any better idea? > [IS07] connection state. The idea is that ConnectionManager.connect() always returns ESTABLISHED connection. If it fails - an exception is thrown... So once you get a Connection object you can work with it. But, of course, there is no guarantee that it was not lost right after it's establishment... So even if it was established before you started to use it, you may get an exception... So from this perspective continuations are not much better - even if you get control in conenctionSucceeded() method, connection may already be down... > [IS08] Connection.close() Initially it was in the API... Later it was considered as a 'dangerous' operation ;) So the idea is to give access to this functionality to connections management subsystem only (using a friend API or smth similar) (so that user could disconnect from a host using some UI button)... The motivation for this is that closing connection means immediate termination of all processes that are runnings within it.. (which could be not what client expects).. We could consider 'reference counter', though... Or, maybe just open this method and put responsibility on API users.. What do you think? > [IS09] ne.api and ne.api.process I agree to merge these two ;) (I guess the same is for spi) > [IS10] NativeProcessBuilder.setProperty(). Sure, the USE_PTY is a good example (and I was sure that I put it somewhere! Seems that it was lost while refactoring)... Will add this to the javadoc, thanks! > [IS11] NativeProcess.getProperty(). What other properties you think user could expect here? Everything that was put using NativeProcessBuilder.setProperty()? In 'old' api there is a ProcessInfo/ProcessInfoProvider API/SPI... Maybe we can use the same/similar approach here? > [IS12] NativeProcessBuilder.setCommand() >- Does 'arguments' include argv[0] or is 'executable' assigned to argv[0]? > Consider lost flexibility for applications which change behaviour > based on argv[0]. Now it is that argv[0] is set to executable. (BTW, I don't know how to create a process with argv[0] != executable not using native code)... I know the only executable (shell) that relies on this... Isn't it too deep-dive (and platform dependent)? > Any value to separating setCommand() into setExecutable() and > setArguments()? It was the original approach. But the usage was almost always the same - npb.setCommand("/bin/...").setArguments("/home", "/")... So it was decided to 'join' these two calls onto a single one... > executable in constructor .. I can imagine a situation that executable will be set depending on, say, a result of getSupportedProperties() of builder... Like: NPB npb = connection.newProcessBuilder(); if (npb.getSupportedProperties().contains("XForwarding")) { npb.setProperty("XForwarding", Boolean.TRUE); npb.setCommand("xcalc"); } else { npb.setCommand("calc"); } But, maybe it is too non-realistic scenario.. > [IS14] A motivating reason for setShellScript() would help. > So setShellScript() will feed a composed-on-the-spot script > via input streams and not upload any files, right? Right. But here is a dilemma: some interpreters read from stdin if invoked without any arguments, others require some special flags (like -s for shell)... So now I used 'shell' as executable, then add '-s' flag and feed script to it's stdin... And I'm not very happy with this solution.. > [IS15] Mixing of immutable setters style and mutable properties. You can configure environment at any time... But (one of) the first thing that is done when you invoke call() is cloning of this map... So after call() is invoked you cannot affect process' environment. But you can continue modify the environment of Builder... Subsequent calls will use 'latest' state of the map.. > [IS16] EnvironmentMap.appendPath()/prependPath()? Yes, I think that pathElement is a better naming.. But this is just a string concatenation with an appropriate delimiter (and duplicates elimination)... So appendPathElement("LD_LIBRARY_PATH", "/path1:/usr/path2") is also 'allowable', but on Windows it will not convert passed ':' to the ';' and will be considered as a single path element (while on Unix it will be two elements).. Well... not perfect.. (but this proved to be useful in our code) > [IS17] EnvironmentMap.createCasePreserving/Sensitive(). > - Is it the case that all PATH-like variables will use the same > pathSeparator? Yes. Do you know exceptions? >What is the default pathSeparator when we get an EnvironmentMap from > NativeProcessBuilder.getEnvironmentMap()? It's up to SPI implementor. On Unix you'll get ':', on Windows - ';'... > - Shouldn't these be in the SPI? But this is an SPI - actually environment is taken (cloned) from ConnectionInfo. And ConnectionInfoProvider (SPI) provides this info... > [IS18] Re use of clone (e.g. in MacroMap). I just didn't want to make public any constructor... Inside clone() a private copy constructor is used... I can change this.. > [IS19] If NativeProcessBuilder gets it's copy of EnvironmentMap from a > Connection, why can't I access and modify a Connections EnvironmentMap? Do you really need to (as an API user)? Existent SPI (ConnectionInfo) could be used for that.. > [IS20] MacroMap vs expandMacros vs EnvironmentMap > My understanding is that "macro"s are syntax used in CND like this: > ... > Also that EnvironmentMap inherits from MacroMap is somewhat conceptually > confusing.. The idea behind this is that MacroMap is almost a simple map (key => value), but: - uses a specified comparator to compare keys; - when it contains a record "VAR_NAME=>VAR_VALUE" adding a record "VAR_NAME=>${VAR_NAME}_NEW" results in VAR_NAME=>VAR_VALUE_NEW" - has additional 'dictionary', which is not a part of map itself, but has definitions of some 'macros'. For example it could contain "arch=>x86". In this case map.put("PATH", "$PATH:/my-bin/${arch}) results in /my-bin/x86 being appended to the PATH record. You are right, this has nothing to do with Unix environment - just strings map. But this can perfectly be used for configuring Unix environment - and having methods append/prependPath make this task even easier for users. So environment-specific functions/factories are in EnvironmentMap, but it is still a MacroMap... (and this is not about displaying '${working-dir}/a.out').. Still doesn't make sense? > [IS21] expandMacros() takes a String and returns a String, so how does one > then convert the String to a series of arguments to pass to NPB.setCommand? Hm.. don't understand the question... expandMacros() just takes a string and a map with macro definitions and substitute found entries with their values... > [IS22] ProcessUtils.ExitStatus > - Can you add a teeny reference ... Sure! > - Under what circumstances are 'error' and 'output' available? > I.e. is what gets sent to an ProcessOutputProcessor also get copied > to 'error' and 'output'? If the ProcessOutputProcessor's are null > will 'error' and 'output' still be filled? No... Every method that has own ProcessOutputProcessor returns int, not ExitStatus. So either you use 'simple' execute and in this case you'll get error/output in ExitStatus or you use 'slightly advanced' execute() where you can provide own processors... > - Are the field values for 'error', 'exitCode' and 'output' final? Yes, they are. ProcessUtils is an utility class for handling very simple cases - user may want to use it when he don't want to deal with threads, reading-out error/output, etc... > [IS23] ProcessOutputProcessor > The output received from the process has to go _somewhere_ but the way > ProcessUtils.execute() starts new threads there's no way to pass some > info to the ProcessOutputProcessor regarding what should be done with > the output which means one will have to depend on global data? Why? Could you give some example? Seems I don't understand the problem... If you pass your outputProcessor, you usually should know what to do with the data that comes from a process... If you pass 'null', then data is just read-out and disregarded... > [IS24] Don't TasksCachedProcessor and Computable belong to nativeexecution.spi.support? Frankly, I would like to see these classes in some more low-level utils API - I think they are very useful. This is why I didn't want to hide them under 'spi' package... > [IS25] You're "done" with a process not only when it has exited (e.g. > waitFor()) has returned but also after all output which is still in the pipe > has been drained and processed. Right, and in case of ProcessUtils utilization this is taken into account. But if user calls NPB.call() directly and deals with Process's I/O by himself, then it is his responsibility...
Sorry guys, but I think we are getting into details too early. We need at least one client of the API (the complete patch) to determine: - what is redundant - what is missing - proper class/method layout - integration/merge with extexecution (for example looks like these classes are similar/possible duplicates: NativeProcess->Process, NativeProcessBuilder->ProcessBuilder, ProcessUtils->ExternalProcessSupport, etc..) So imo it is too early to discuss particular methods as these may be changed/refactored/removed and new things may be added.
I'm closing this one in favor of #232434.