Community
Participate
Working Groups
For testing purposes it is sometimes required to spawn a host and a consumer in parallel. This should be supported by Bucky by e.g. taking a comma separated listed of .launch files as an argument the launch command.
Thinking about it, the host&consumer example might require something more sophisticated/different in that the host process can be seen as a support process for the consumer and thus be taken down once the consumer terminates. Looking at the current implementation org.eclipse.buckminster.core.commands.Launch.internalRun(IProgressMonitor), this might even be implemented more easily as in this scenario a single process return value is acceptable contrary to the case where two (or more) generic launch configs are executed concurrently.
As an alternate implementation strategy, the Job FW might be of interested as it comes with support for rules which in turn might help model support processes.
Created attachment 174223 [details] A new command "bglaunch" which spawns .launch in the background Terminates the spawned process on System.exit
One can see the command in action e.g. as part of the ECF build [0]. [1] shows the use case for "--ignoremissing". [0] https://build.ecf-project.org/hudson/job/C-HEAD-remoteservice.rosgi.feature/ [1] https://build.ecf-project.org/hudson/job/C-HEAD-discovery.jmdns.feature/
Created attachment 174833 [details] Same functionality as the background bundle, but written as a new command in buckminster.core
Created attachment 174834 [details] mylyn/context/zip
Comment on attachment 174833 [details] Same functionality as the background bundle, but written as a new command in buckminster.core Patch applied to helios-maintenance, rev 11513. Thanks!
-
I have some comments and questions regarding the new command: * The BackgroundLaunch command declares its own "-l" argument, but it also calls super.getOptionDescriptors(), so it inherits the "-l" argument from the Launch command. This is redundant. * In calling super.getOptionDescriptors(), it also inherits the --stdout and --stderr arguments, but it ignores them if they are given. So it is effectively not possible to redirect stdout/stderr for processes launched in the background. * It replicates some of the logic already found in the Launch command. Adding support for stdout/stderr capture will replicate even more code. Why did you choose to implement a separate command instead of adding a new flag (--background) to the regular Launch command? We can also add the --ignoremissing flag. Since we haven't officially relased the BackgroundLaunch command, I think there's still a chance to merge it into the regular Launch command. What do you think?
I think that sounds like a worth while improvement. Markus, what do you think?
(In reply to comment #9) > * The BackgroundLaunch command declares its own "-l" argument, but it also > calls super.getOptionDescriptors(), so it inherits the "-l" argument from the > Launch command. This is redundant. Definitely an inconsistency. > * In calling super.getOptionDescriptors(), it also inherits the --stdout and > --stderr arguments, but it ignores them if they are given. So it is effectively > not possible to redirect stdout/stderr for processes launched in the > background. We (ECF) don't need stdout/stderr functionality for a background launch. Do you guys think it's worth keeping? > * It replicates some of the logic already found in the Launch command. Adding > support for stdout/stderr capture will replicate even more code. > > Why did you choose to implement a separate command instead of adding a new flag > (--background) to the regular Launch command? We can also add the > --ignoremissing flag. Since we haven't officially relased the BackgroundLaunch > command, I think there's still a chance to merge it into the regular Launch > command. What do you think? I suggest we create a common abstract parent class like AbstractLaunchCommand instead and have both LaunchCommand as well as BackgroundLaunchCommand inherit from it. Separating background launch and launch buys us the freedom to add parameters that might not make sense for the other one. E.g. what does something like a "keeprunning" flag mean for the regular launch. Additionally it keeps the implementation free from to much control flow. Btw. IMO "--ignoremissing" should be made available for both bglaunch as well as launch. If this finds approval by you, I'd be willing to write a new patch.
IMO, launching a process in the background is not so much different from regular launching. In UNIX terms, it's just about adding the "&" at the end. Everything else, e.g. stdout/stderr redirection is the same in both cases. As I see it, the only difference code-wise is that the foreground version waits for the processes to finish. If something goes wrong, the launched processes must be terminated before returning from the Launch command. The background version performs process termination in the VM shutdown hook. Thomas, which version do you prefer? In either case, a new patch is highly appreciated! I'll reopen the issue to reflect the ongoing work.
(In reply to comment #12) > Thomas, which version do you prefer? > I'm in favor of one single launch command with a --background option since that seems to cover all needs. It's easier to document and easier to understand.
Created attachment 175712 [details] AbstractLaunch patch (In reply to comment #13) > I'm in favor of one single launch command with a --background option since that > seems to cover all needs. It's easier to document and easier to understand. But how do you address conflicting/inconsistent parameters between background and regular launch, e.g. "keeprunning"? Patch that adds: - a common superclass AbstractLaunch - "keeprunning" flag to BackgroundLaunch (untested)
(In reply to comment #14) > But how do you address conflicting/inconsistent parameters between background > and regular launch, e.g. "keeprunning"? > Add some code to the already existing consistency check at the beginning of internalPerform, i.e. if (launchName == null && !ignoreMissing) throw new UsageException(Messages.Launch_No_launch_config); if(keeprunning && !background) throw new UsageException(Messages.Launch_keeprunning_only_for_background);
(In reply to comment #15) > Add some code to the already existing consistency check at the beginning of > internalPerform, i.e. > > if (launchName == null && !ignoreMissing) > throw new UsageException(Messages.Launch_No_launch_config); > if(keeprunning && !background) > throw new UsageException(Messages.Launch_keeprunning_only_for_background); And once more parameters get added to launch the list of conditions will become longer and longer. Not to mention different permutations that might be acceptable. Anyway, you have a much better perspective WRT maintainability, thus I will rewrite my patch to use a single Launch class (tomorrow).
Created attachment 175758 [details] merges BackgroundLaunch and Launch Patch does not implement "keeprunning". Need to define the streamlistener behavior first when the background process continues running after the host process terminates.
