Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #259414+++ We should be getting rid of the warnings in SshServiceCommandShell line 187, which are issued because of using internal non-API from TerminalHostShell.getPromptCommand() The ServiceCommandShell needs to know the prompt command in order to filter it out from responses. This whole concept seems broken, because the prompt command does not depend on the SSH transport for the shell, or the fact that a TerminalHostShell is being used; it really depends on the kind of remote shell in use, which depends on the systemType. We should add API for querying the kind of remote shell in use, which includes the kind of prompt Command to be used; and, API for setting the kind of remote shell in use (such that auto-discovery can set this Property in case it can be auto-detected). One idea for achieving this might simply be adding Property Constants for IRSESystemType, e.g. IRSESystemType.getProperty(IRSESystemType.PROPERTY_SHELL_TYPE) IRSESystemType.getProperty(IRSESystemType.PROPERTY_PROMPT_COMMAND) The prompt command / shell type is one of the properties needed for auto-detection of more shell features as per bug 244404.
Created attachment 127224 [details] first draft
(In reply to comment #0) > We should be getting rid of the warnings in SshServiceCommandShell line 187, > which are issued because of using internal non-API from > TerminalHostShell.getPromptCommand() > > The ServiceCommandShell needs to know the prompt command in order to filter it > out from responses. This whole concept seems broken, because the prompt command > does not depend on the SSH transport for the shell, or the fact that a > TerminalHostShell is being used; it really depends on the kind of remote shell > in use, which depends on the systemType. > > We should add API for querying the kind of remote shell in use, which includes > the kind of prompt Command to be used; and, API for setting the kind of remote > shell in use (such that auto-discovery can set this Property in case it can be > auto-detected). > > One idea for achieving this might simply be adding Property Constants for > IRSESystemType, e.g. > IRSESystemType.getProperty(IRSESystemType.PROPERTY_SHELL_TYPE) > IRSESystemType.getProperty(IRSESystemType.PROPERTY_PROMPT_COMMAND) I was able to get rid of using internal non-API from TerminalHostShell by introducing new API in IRSESystemType (see attached patch), but we still have hardcoded prompt command in TerminalHostShell.getPromptCommand() and I do not see how we can make it use new API. Martin, do you have any ideas?
Regarding the patch: I'm against having IRSESystemType#getPromptCommand() IRSESystemType#getShellType() IRSESystemType#DEFAULT_PROMPT_COMMAND I think that the default prompt command should be hardcoded in the subsystem (as today), and not in the systemType. That said, the property constants should be sufficient on IRSESystemType and no getter methods should be needed. Regarding TerminalServiceHostShell#getPromptCommand(): Your problem is that you cannot access the IRSESystemType from the services, right? - I actually think that the services shouldn't know or care for the prompt command. If possible, you should try moving the sending of prompt commands from the Service into the Subsystem (ServiceCommandShell). In case that should not be possible, we'd need to think about adding support for Properties in the IService, and push the IRSESystemType properties into the IService when the IService is instantiated.
Created attachment 127347 [details] second draft
(In reply to comment #3) > Regarding the patch: I'm against having > > IRSESystemType#getPromptCommand() > IRSESystemType#getShellType() > IRSESystemType#DEFAULT_PROMPT_COMMAND > > I think that the default prompt command should be hardcoded in the subsystem > (as today), and not in the systemType. That said, the property constants should > be sufficient on IRSESystemType and no getter methods should be needed. Agreed. See if you like this variant better. > Regarding TerminalServiceHostShell#getPromptCommand(): > > Your problem is that you cannot access the IRSESystemType from the services, > right? - I actually think that the services shouldn't know or care for the > prompt command. If possible, you should try moving the sending of prompt > commands from the Service into the Subsystem (ServiceCommandShell). So, getPromptCommand() occurs in 2 places in TerminalServiceHostShell: 1) in writeToShell method; 2) in constructor. I see the only way to pass prompt command there by adding one more variable to the constructor, which requires knowledge of prompt command in TerminalShellService.
I cannot see why the Service should be responsible for producing a special kind of prompt that is to be consumed by the subsystem later on. That's a design flaw IMO. The Service should only be responsible for bringing up a shell channel on which commands can be sent and response can be obtained. The subsystem should be responsible for creating a prompt and processing its output. Therefore, I'd be in favor of tossing the whole prompt command story from IHostShell implementations, and moving it into the subsystem. I'm not 100% sure if this is possible but it's worth trying. At the moment I'm actually more worried about this in the TerminalServiceHostShell constructor: writeToShell("cd " + PathUtility.enQuoteUnix(initialWorkingDirectory)); Why should the Service know that "Unix style quoting" is needed here? Why should it know that "cd" is a valid command for changing directory? The Constructor requires the ability to set an initial working directory, but this is problematic IMHO. If we can, we should try and move that concept out of the service as well (but we'll likely not be able to because it is API so we'll need to find some other solution...)
While I completely agree with you on design, I have a feeling it'll be way too many changes in the code.
Will work on it after 3.1 is out.
