ExecutableTask refactoring

[David Schmitt]

Hi all,

while working on the 504 patch (process priority) I noticed that those
tasks have quite a bit of duplication in terms of functionality and
attributes. At the same time the have quite divergent coding styles.

1) Is there any agreed upon coding style that should be applied?

2) Is there a possibility with the NetReflector attributes to move them
to a common parent? (I couldn't find useful info on the NetReflector wiki.)

3) Would someone apply such a refactoring if I create such a patch?

4) Would someone please apply the patch from CCNET-504?

Seasons Greetings,
David Schmitt

[Ruben Willems]

Hi


1) what do you mean by different coding styles? (they implement all ITask, and most inherit from TaskBase)
    can you give a short example of 2 tasks where it is shows the difference ?

2) you mean the following
    Taskbase has a reflector name attribuut
    all class inheriting from TaskBase will also have this attribute

    what can be a problem is that a netreflector attribute is set at Class level
    and that this is inherited

3) if the patch makes the code more clear, by all means  : yes

4)sure, but I can not garantuee a time, I'm now slowly catching up again with ccnet coding
     (busy time at work and home)


all of the above apply to the 1.5 code base
in the 1.4.4 branch we do not do any refactoribgs any more
(only small bugfixes / improvements)


with kind regards
Ruben Willems

[David Schmitt]

On 29.12.2009 12:47, Ruben Willems wrote:
> Hi
>
>
> 1) what do you mean by different coding styles? (they implement all
> ITask, and most inherit from TaskBase)
> can you give a short example of 2 tasks where it is shows the
> difference ?

ExecutableTask uses public consts with all caps names, fields for
attributes, no regions and mixed tabs and spaces for indentation.

NDependTask uses private consts with mixed camel case names, properties
for attributes, regions for every other line and 4 spaces.

NUnitTask uses public consts with proper camel case names, fields for
attributes, regions for some parts and mixed tabs and spaces for
indentation.

I won't go into the various different styles of formatting.

Overall it makes it harder to get into the code-base as one has to get
to grips with the different styles and conventions on a file-by-file basis.

> 2) you mean the following
> Taskbase has a reflector name attribuut
> all class inheriting from TaskBase will also have this attribute
>
> what can be a problem is that a netreflector attribute is set at
> Class level
> and that this is inherited

Yes, that's exactly the point. Most of the Tasks that use the
ProcessExecutor to call an external binary need to have the various
attributes required by BaseExecutableTask and ProcessInfo: executable,
baseDirectory, environment, successExitCodes, timeout, etc.

Moving those attributes to the BaseExecutableTask class would reduce the
source code size significantly.

One problem I already found is the version documentation. But I guess
that info is not that important and in the worst case can be merged from
the various child classes.

> 3) if the patch makes the code more clear, by all means : yes

I'll see what I can come up with.

> 4)sure, but I can not garantuee a time, I'm now slowly catching up again
> with ccnet coding
> (busy time at work and home)

Great. That's all I wanted to hear. I hope y'all had nice and comfy
holidays :)

> all of the above apply to the 1.5 code base
> in the 1.4.4 branch we do not do any refactoribgs any more
> (only small bugfixes / improvements)

Of course. Our local CCNet server is running from HEAD with the 504
patch applied. So I'm in no position anyways to talk about 1.4.x.

Seasons greetings,
David Schmitt

[Ruben Willems]

Hi

ok for the refactor part (item 1), I have no problems there.
I just do not know what 'style' would be best

So I leave that to the other devs(or you)

For point 2
if for example Nunit has the timeout property on version 1.3
and the ExecTask got it from version 1.0, the docs can get messed up :-(

now the point is : we just added this version info field, and every release has its own documentation published.
So I personally do not mind if the history gets mesed up for this.
If that really reduces a lot of code, and makes it more consistent, please do.
(Me wondering why those attributes are not on the interface, so every task must have them)


for point 4
it was not holiday :-(,
really hard work at the house : renovating :-(

so I'm glad it's almost done. :-) :-) :-)

with kind regards
Ruben Willems

[David Schmitt]

On 29.12.2009 15:06, Ruben Willems wrote:
> Hi
>
> ok for the refactor part (item 1), I have no problems there.
> I just do not know what 'style' would be best
>
> So I leave that to the other devs(or you)

Preferrably the same everywhere ;-)

> For point 2
> if for example Nunit has the timeout property on version 1.3
> and the ExecTask got it from version 1.0, the docs can get messed up :-(

> now the point is : we just added this version info field, and every
> release has its own documentation published.
> So I personally do not mind if the history gets mesed up for this.
> If that really reduces a lot of code, and makes it more consistent,
> please do.

Now I have a patch on my disk which is 180k without and almost 500k with
the whitespace changes.

This patch pulls the <executable/>, <workingDirectory/>, <timeout/> and
<priority/> elements up into the BaseExecutableTask. To make this
possible, almost all workingDirectory attributes had to be renamed. They
were all over the place: baseDir, baseDirectory, and so. The naming of
the timeout attribute was inconsistent, so I renamed a few here too, but
that was not so widespread. Additionally I left the old fields in place
and let them log a warning message about the depriciation. That should
give people plenty time to fix their configuration after 1.5.

The second part of the work was excavating the different parts of the
execution process. In the end I have split that up into the following
methods which can - but don't have to be - overridden:

BeforeExecution
GetProcessArguments
GetProcessSuccessCodes
CreateProcessInfo
Execute
MergeStdout
MungeResult
PublishAdditionalResults

Additionally there are a few properties and methods that have to be
implemented, since they do not have obvious defaults:

DefaultExecutable
DefaultDescription

Since the patch is big and unwieldy, I've only attached the
BaseExecutableTask.cs so you can have a look at the top-level code.

On the down-side I had to disable the NDependTask tests since are quite
brittle and do not actually test the requirements of NDepend. According
to http://www.ndepend.com/NDependConsole.aspx the NDepend.Console
program only accepts absolute paths, but the tests only use and expect
relative paths. This has to be reddone by someone who has NDepend
available to cross-check that.

Finally there are quite a few Tasks that use a ProcessExecutor but do
not inherit from BaseExecutableTask. While I haven't looked too deeply
into those, I guess they should be ported too:

DevendTask
FinalBuilderTask
NUnitTask
PowerShellTask

> (Me wondering why those attributes are not on the interface, so every
> task must have them)

I'm all for it :)

> for point 4
> it was not holiday :-(,
> really hard work at the house : renovating :-(
>
> so I'm glad it's almost done. :-) :-) :-)

At least a happy end! :-)

good night, David Schmitt

>
>
>
> with kind regards
> Ruben Willems
>
>
>

[Ruben Willems]

Hi

thanks for the info
it will take some time to check this all, a lot of changes in this file alone

with kind regards
Ruben Willems

[David Schmitt]

On 30.12.2009 12:07, Ruben Willems wrote:
> Hi
>
> thanks for the info
> it will take some time to check this all, a lot of changes in this file
> alone

Yeah, for actually submitting the change I'll probably have to re-do the
patch anyways to cleanly split the whitespace changes and functionality
stuff, so I'm not overly concerned about merge-drift. Since I now know
what can or shouldn't be done, it'll be much easier the second time around.

It'd be great if one of the committers had resharper to use the
http://www.codeplex.com/StyleCopForReSharper plugin to batch-fix the
most annoying stuff. As mentioned elsewhere[1] I'd vote for disabling
the following rules:

* File Headers (SA1633-SA-1640)
* Code ordering (SA1200-SA1202)
* Requiring "this" (SA1101)

Regards, David Schmitt

[1]http://stackoverflow.com/questions/286180/alternative-to-stylecop-for-visual-studio/288833#288833

[Craig Sutherland]

Hi guys,

I've been away on holiday, so I've only just seen this thread.

I agree, se do have a number of inconsistencies, both in the code and the
configuration elements. I do agree that it would be nice to tidy them all up
so everything is consistent, but I do not think this is the right time for
it :-(

Basically I have two reasons why:
1) I've just gone through and updated all the documentation, both in the
code and the wiki. While it is easy enough to generate the documentation,
we'd still have to go through and publish it to the wiki - which does take
time!
2) We're trying to get our bug count down for an "official" release (Ruben
had another post about this).

Plus some of the changes that were added to the 1.5 release means we could
do an automatic conversion if these were changed in a future version (1.6
perhaps?) This would also give us time to go through and fix up some of the
other inconsistencies (like the way time-outs are configured, common source
control/trigger properties, etc.)

So, I do think it is an awesome idea, I would just like to see 1.5 released
first.

Craig

[David Schmitt]

Craig Sutherland wrote:
> Hi guys,
>
> I've been away on holiday, so I've only just seen this thread.
>
> I agree, se do have a number of inconsistencies, both in the code and the
> configuration elements. I do agree that it would be nice to tidy them all up
> so everything is consistent, but I do not think this is the right time for
> it :-(
>
> Basically I have two reasons why:
> 1) I've just gone through and updated all the documentation, both in the
> code and the wiki. While it is easy enough to generate the documentation,
> we'd still have to go through and publish it to the wiki - which does take
> time!
> 2) We're trying to get our bug count down for an "official" release (Ruben
> had another post about this).
>
> Plus some of the changes that were added to the 1.5 release means we could
> do an automatic conversion if these were changed in a future version (1.6
> perhaps?) This would also give us time to go through and fix up some of the
> other inconsistencies (like the way time-outs are configured, common source
> control/trigger properties, etc.)
>
> So, I do think it is an awesome idea, I would just like to see 1.5 released
> first.

Ah, I totally understand that! It'd be great to have 1.5 sooner than
later. Also, it makes my work safer in the sense that there'll be less
changes to the tasks while my patch floats around :) And as I said
elsewhere, it'd probably be great if someone would take out a Resharper
license (I believe there is a gratis version for open source
contributers) and mass-fix the formatting first. Perhaps late in the 1.5
release cycle, when there are few outstanding patches?

Would you still take a look at the BaseExecutableTask I posted to check
if you're in principle ok with the way I implemented it?

Regards, David