Community
Participate
Working Groups
We should refactor the core UI-less scp IFileService implementation, such that it can be used in server-side environments without RSE. This should be easy, by just 1.) moving the 3 core classes into a new package inside services.ssh 2.) exporting that package as x-friends:=subsystems.files.scp 3.) Requiring services.ssh with the new version in subsystems.files.scp The drawback of doing so would be that the scp plugin requires the new (3.3) version of RSE, whereas today it might also work with the older (3.2 or even 3.1) version of TM. What do others think, should we move forward with the refactoring or wait?
For us it doesn't matter, we are fine with doing refactoring now.
Martin, can we move services.ssh to the 1.5? It's 1.4 now.
What value do we get from moving to 1.5 ?
I just thought maybe it's time. :) Our SCP classes that we need to move to services depend on 1.5 methods. We can fix them of course.
I'd like to follow the Eclipse Platform's way here: only upgrade the Bundle-RequiredExecutionEnvironment when there is a clear benefit in doing so. Much of Eclipse Platform (including core.resources) is 1.5 now; but I think that all the dependencies on the Services layer are still 1.4 or below, and there must be good reason for changing this. That being said, if you have any good reasons why you chose 1.5 in SCP I'll consider change.
Created attachment 185244 [details] The refactoring with plugin version updates Martin, Nikita, could you please review the patch? Thanks!
Patch looks good to me as a first cut. In a second step, you might want to unify some functionality, e.g. there could be a single implementation of ScpFileUtils#concat() . Also, ScpFileUtils#escapePath() looks to me like it won't properly escape the " character and $ character -- we have different implementations of escapePath() in RSE which are more adequate for UNIX. I can't remember whether the files.scp package re-uses anything from the files package and thus it's required to be a sub-package of files. I could also see a files package and scpfiles package reside in parallel. But I don't have a strong opinion on that, and files.scp might just be fine.
PS to be clear about the 2nd step, I suggest committing the first step before making the second step changes in a separate patch.
PPS on version ranges, I'm not yet entirely sure whether we want a 4.0.0) upper limit or 3.2.0) upper limit. At any rate, any MANIFEST.MF that you touched must up its micro version by 100 (unless it was uped in the 3.3 Stream already, you'll need to check what the version was like in R3_2_0). Corresponding features must also up their version number accordingly.
Created attachment 185266 [details] Added versions update for affected plugins and features
The terminals feature should go to 1.0.200.qualifier instead of 1.0.201. What about the ssh feature? It should get a minor version uprev since services.ssh gets a minor version uprev. The rse.tests bundle and feature should go to 3.3 I think -- tests don't provide any API, so rather than using the "API Rules" for version update I'd suggest always using the major.minor version of the main TM Release which is 3.3 now. I think that I did the same on the rse.sdk and rse.runtime features which should show the "overall product version".
Patch looks OK to me.
I checked in the patch. Martin, do you want to create additional bugs for the step 2, or can we continue here?
Martin, did you mean we need to replace ScpFileUtils#escapePath() with org.eclipse.rse.core.model.SystemEscapeCharHelper#getStringForFileName() method? Or is there any other method I should use?
Created attachment 207821 [details] Patch for step 2 Martin, could you please review?
I checked in the patch for step 2. Is there anything else needed in this bug?
Created attachment 210306 [details] Patch with version updates to 0.2.0 for scp and 3.2.0 for services.ssh Patches look good - given that this is new functionality for scp (being available in a UI-less environment), I decided to uprev the plugins as well as the feature by a minor increment. In Juno, we now have the scp feature and plugin at 0.2.0 and services.ssh at 3.2.0 Also added the minimum version dependency on subsystems.scp, upreved the update site URL in the feature, and fixed the "Copyright-to-year" that's visible in the features and branding plugins. I think we can consider this done, but I'd like somebody to test whether the actually desired effect is there (ie is it possible to have a UI-less service with SCP now). Do we have any unittest for this ?
(In reply to comment #17) > I think we can consider this done, but I'd like somebody to test whether the > actually desired effect is there (ie is it possible to have a UI-less service > with SCP now). Do we have any unittest for this ? I don't think we have it. Will copying something be a sufficient unit test? I can try to add something like this.
Sure. Anything that uses _any_ functionality from SCP would do.
Created attachment 210378 [details] Unit test for scp file subsystem Martin, will something like this do?
That kind of test is great and you should definitely add it (plus we could look at doing the RSEFileStoreTest to use scp connectivity in addition to local, ssh, ftp, dstore). But that still wouldn't check that the ScpFileService can be used without any UI dependencies. I can't remember the exact history of this, but I think the desire was to be able and directly use the ScpFileService for transferring files (without the subsystem layer). I'm not sure what the idea was regarding how to get hold of the ScpFileService instance (would one just instantiate it although it's not API or would we have some clever trick using adapters to get to it). But that doesn't matter for a Unittest since a real unittest operates on as small a component as possible. I'd think that you could write a plain Unittest (not PDE Plugin test) which just instantiates the ScpFileService and exercises the methods to do some stuff like login, and transfer a file.
(In reply to comment #21) > That kind of test is great and you should definitely add it (plus we could look > at doing the RSEFileStoreTest to use scp connectivity in addition to local, > ssh, ftp, dstore). > > But that still wouldn't check that the ScpFileService can be used without any > UI dependencies. I can't remember the exact history of this, but I think the > desire was to be able and directly use the ScpFileService for transferring > files (without the subsystem layer). > > I'm not sure what the idea was regarding how to get hold of the ScpFileService > instance (would one just instantiate it although it's not API or would we have > some clever trick using adapters to get to it). But that doesn't matter for a > Unittest since a real unittest operates on as small a component as possible. > I'd think that you could write a plain Unittest (not PDE Plugin test) which > just instantiates the ScpFileService and exercises the methods to do some stuff > like login, and transfer a file. How do I get an IHost in this case? I'd need to read it from properties, which will require PDE, won't it?
I don't think you need an IHost. Your Unittest could directly instanciate your ScpFileService. All you need is a minimal ISshSessionProvider. The session could be hardcoded to the local host. ScpFileService fs = new ScpFileService(new ISshSessionProvider() { public String getControlEncoding() {return "iso-8859-1";} public void handleSessionLost() {} public Session getSession() { //copy some code from SshConnectorService#createSession() to here } }); This is just for a Unittest so it doesn't need to be pretty .. fine to have a mock object for the ISshSessionProvider.
(In reply to comment #23) > I don't think you need an IHost. > > Your Unittest could directly instanciate your ScpFileService. All you need is a > minimal ISshSessionProvider. The session could be hardcoded to the local host. > > ScpFileService fs = new ScpFileService(new ISshSessionProvider() { > public String getControlEncoding() {return "iso-8859-1";} > public void handleSessionLost() {} > public Session getSession() { > //copy some code from SshConnectorService#createSession() to here > } > }); > > This is just for a Unittest so it doesn't need to be pretty .. fine to have a > mock object for the ISshSessionProvider. I am going to need the access to IJSchService to initialize the Session: session = service.createSession(hostname, port, username); It's usually done by tracker.getService(). Is this acceptable? I don't know how to access it in a different way...
Only unit test is left to mark this as done.
Anna, for cleanup I suggest marking this done and creating a new bug (against 3.4.1 ?) for the unittest.
Created bug 382536.
