Created attachment 28353 [details] A patch to implement the enhancement Right now Execute uses a static CommandLauncher instance which is initialize inside the static initializer ("class constructor"). CommandLauncher's responsibility is to create the Process instances. Currently there isn't a way to make Execute use a different CommandLauncher I have attached a patch containing the changes I made to Execute java class. What I have basically done is move the code that initializes the CommandLauncher from the Execute class to to the CommandLauncher class. I have made CommandLauncher class public and all the subclasses of it public too. I have added two static methods getVMLauncher and getShellLauncher to the CommandLauncher class. These methods take a Project object as parameter and check if a reference to an existing CommandLauncher is present in the project. If present then that instance is returned. Otherwise the method checks if a system property containing the name of the launcher class has been set. If so then an instance of that class is created and returned, otherwise the default launcher is returned. I have also added setVMLauncher and setShellLauncher methods that add reference to a given CommandLauncher instance to the project. I have modified the Execute class to use these two methods to get the reference of the CommandLauncher class. I have also created a CommandLauncherTask class similar to the PropertyHelperTask that takes an instance of any CommandLauncher class and adds a reference to it to the project.
Would it make sense to extract CommadLauncher into a separate class or do you need access to internals of Execute? Other than that the changes look good to me. For the future, please provide a diff rather than complete classes since it helps with reviews and survives some code changes to the class in svn.
I think CommandLauncher can be extracted to a new class. The CommandLauncher class accesses 'environmentCaseInSensitive' which is internal to Execute class, but that can be moved to a static constructor block within the Execute class itself. However the sub-classes of CommandLauncher class i.e PerlScriptCommandLauncher, ScriptCommandLauncher and VmsCommandLauncher access FILE_UTILS singleton which is internal to Execute, So I am wondering whether they should remain within the Execute class itself. I wanted to do a diff, but I couldn't find a suitable tool in windows that generates a diff? Do you know of any tool that would do that?
(In reply to comment #2) > I think CommandLauncher can be extracted to a new class. The CommandLauncher > class accesses 'environmentCaseInSensitive' which is internal to Execute > class, but that can be moved to a static constructor block within the > Execute class itself. Yes, sounds good. > However the sub-classes of CommandLauncher class i.e > PerlScriptCommandLauncher, ScriptCommandLauncher and VmsCommandLauncher > access FILE_UTILS singleton which is internal to Execute, So I am wondering > whether they should remain within the Execute class itself. The FILE_UTILS is just a performance enhancement, it is backed by a Singleton instance in FileUtils so it wouldn't cause too much overhead to copy it around to each class (or add a protected one in CommandLauncher). > I wanted to do a diff, but I couldn't find a suitable tool in windows that > generates a diff? Do you know of any tool that would do that? Subversion ;-) svn diff inside a working copy.
Created attachment 28910 [details] patch for 52706
(In reply to comment #3) > (In reply to comment #2) > > > I think CommandLauncher can be extracted to a new class. The CommandLauncher > > class accesses 'environmentCaseInSensitive' which is internal to Execute > > class, but that can be moved to a static constructor block within the > > Execute class itself. > > Yes, sounds good. I have extracted CommandLauncher, CommandLauncherProxy, Java13CommandLauncher, MacCommandLauncher, OS2CommandLauncher, PerlScriptCommandLauncher, ScriptCommandLauncher, VmsCommandLauncher and WinNTCommandLauncher to new classes under org.apache.tools.ant.taskdefs package. I faced a minor issue when extracting MacCommandLauncher, it was referring to 'antWorkingDirectory' String field in Execute class. I replaced that reference with System.getProperty("user.dir") > > > However the sub-classes of CommandLauncher class i.e > > PerlScriptCommandLauncher, ScriptCommandLauncher and VmsCommandLauncher > > access FILE_UTILS singleton which is internal to Execute, So I am wondering > > whether they should remain within the Execute class itself. > > The FILE_UTILS is just a performance enhancement, it is backed by a > Singleton instance in FileUtils so it wouldn't cause too much overhead to > copy it around to each class (or add a protected one in CommandLauncher). > I've added FILE_UTILS as a static protected field in CommandLauncher. > > I wanted to do a diff, but I couldn't find a suitable tool in windows that > > generates a diff? Do you know of any tool that would do that? > > Subversion ;-) > > svn diff > > inside a working copy. I was able to use the svn diff tool :). It seems eclipse has built-in svn support and can generate patches. I have attached the patch to this reply :)
Vimil, I'm working on your patch. I want to move the launcher classes to a separate package (.... taskdefs.launcher), this is done in my working copy now. Currently I'm fixing the style changes Eclipse has made to the Execute class so the diff becomes smaller. I really don't want to play "fetch me a rock" so I'm doing this myself; will continue with this later today or tomorrow. In the meantime, a test or two for the new task and docs would be great 8-)
> Currently I'm fixing the style changes Eclipse has made to the Execute class > so the diff becomes smaller. I really don't want to play "fetch me a rock" > so I'm doing this myself; will continue with this later today or tomorrow. > > In the meantime, a test or two for the new task and docs would be great 8-) Sure I will work on writing tests and documentation for this enhancement.
a slightly massaged version of your patch is in with svn revision 1350460
Sorry, forgot to save some files, the "real" revision is 1350461
(In reply to comment #9) > Sorry, forgot to save some files, the "real" revision is 1350461 Thanks for merging this patch into the trunk, the changes look good to me :)
We'd need a small manual page like we have for the other tasks and maybe a cross-reference to it from exec. As tests I'd probably write an AntUnit test that builds a CommandLauncher which only echos a message and doesn't do anything else and then assert the message is there. Thinking about it, would it be a good idea to have an option to reset the CommandLauncher to the default one if it has been changed? Let me know if you need any help.
(In reply to comment #11) > We'd need a small manual page like we have for the other tasks and maybe a > cross-reference to it from exec. > I will work on that. > As tests I'd probably write an AntUnit test that builds a CommandLauncher > which only echos a message and doesn't do anything else and then assert the > message is there. I am not familiar with AntUnit but I can surely learn how it works :) I will let you know if I get stuck somewhere. > > Thinking about it, would it be a good idea to have an option to reset the > CommandLauncher to the default one if it has been changed? > > Let me know if you need any help. How about an attribute called 'reset' for the CommandLauncherTask that removes the project reference and the system property if they are set?
Created attachment 28992 [details] correcting defaults.properties while writing the ant-unit test, I found there is a typo in defaults.properties for the commandlauncher task. It should be commandlauncher=org.apache.tools.ant.taskdefs.CommandLauncherTask instead of commandlaucher=org.apache.tools.ant.taskdefs.CommandLauncherTask
Created attachment 28993 [details] test for commandlaunchertask
Great, thanks, patches applied with svn revision 1353811