Bug 52706 - [PATCH] Make Execute class extensible to allow custom CommandLauncher implementation
[PATCH] Make Execute class extensible to allow custom CommandLauncher impleme...
Status: RESOLVED FIXED
Product: Ant
Classification: Unclassified
Component: Core
1.8.2
PC All
: P3 enhancement (vote)
: 1.9.0
Assigned To: Ant Notifications List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-02-19 11:01 UTC by Vimil
Modified: 2014-01-01 10:49 UTC (History)
1 user (show)



Attachments
A patch to implement the enhancement (14.58 KB, application/x-zip-compressed)
2012-02-19 11:01 UTC, Vimil
Details
patch for 52706 (93.03 KB, patch)
2012-06-11 17:57 UTC, Vimil
Details | Diff
correcting defaults.properties (801 bytes, patch)
2012-06-25 02:26 UTC, Vimil
Details | Diff
test for commandlaunchertask (3.11 KB, patch)
2012-06-25 02:37 UTC, Vimil
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vimil 2012-02-19 11:01:12 UTC
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.
Comment 1 Stefan Bodewig 2012-06-09 17:53:18 UTC
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.
Comment 2 Vimil 2012-06-11 16:29:51 UTC
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?
Comment 3 Stefan Bodewig 2012-06-11 16:44:20 UTC
(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.
Comment 4 Vimil 2012-06-11 17:57:14 UTC
Created attachment 28910 [details]
patch for 52706
Comment 5 Vimil 2012-06-11 18:01:07 UTC
(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 :)
Comment 6 Stefan Bodewig 2012-06-13 04:39:10 UTC
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-)
Comment 7 Vimil 2012-06-13 17:00:32 UTC
> 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.
Comment 8 Stefan Bodewig 2012-06-15 04:29:00 UTC
a slightly massaged version of your patch is in with svn revision 1350460
Comment 9 Stefan Bodewig 2012-06-15 04:34:20 UTC
Sorry, forgot to save some files, the "real" revision is 1350461
Comment 10 Vimil 2012-06-15 16:06:30 UTC
(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 :)
Comment 11 Stefan Bodewig 2012-06-20 04:25:52 UTC
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.
Comment 12 Vimil 2012-06-20 12:38:19 UTC
(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?
Comment 13 Vimil 2012-06-25 02:26:48 UTC
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
Comment 14 Vimil 2012-06-25 02:37:01 UTC
Created attachment 28993 [details]
test for commandlaunchertask
Comment 15 Stefan Bodewig 2012-06-26 04:14:32 UTC
Great, thanks, patches applied with svn revision 1353811