Merge 641903 from trunk to 1.7 branch?

[Stefan Bodewig]

> Author: peterreilly
> Date: Thu Mar 27 10:09:53 2008
> New Revision: 641903
>
> URL: http://svn.apache.org/viewvc?rev=641903&view=rev
> Log:
> Bugzilla 44689: NPE with multiple targets and id's in task

IMHO the bug is serious enough to be fixed in 1.7.1 final and should
be merged into the branch.

> --- ant/core/trunk/src/main/org/apache/tools/ant/UnknownElement.java
> (original)
> +++ ant/core/trunk/src/main/org/apache/tools/ant/UnknownElement.java Thu Mar 27 10:09:53 2008
> @@ -288,11 +288,14 @@
>                  ((Task) realThing).execute();
>              }
>          } finally {
> -            // Finished executing the task, null it to allow
> +            // Finished executing the task
> +            // null it (unless it has an ID) to allow
>              // GC do its job
>              // If this UE is used again, a new "realthing" will be made
> -            realThing = null;
> -            getWrapper().setProxy(null);
> +            if (getWrapper().getId() == null) {
> +                realThing = null;
> +                getWrapper().setProxy(null);
> +            }
>          }
>      }

Looks pretty save to me.  This means we might be having a few
tasks/types living longer than needed, but it seems to be the only way
people can use ids in their build files and run multiple targets at
the same time.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Steve Loughran]

Stefan Bodewig wrote:

>> Author: peterreilly
>> Date: Thu Mar 27 10:09:53 2008
>> New Revision: 641903
>>
>> URL: http://svn.apache.org/viewvc?rev=641903&view=rev
>> Log:
>> Bugzilla 44689: NPE with multiple targets and id's in task
>
> IMHO the bug is serious enough to be fixed in 1.7.1 final and should
> be merged into the branch.

+1. We could always do a quick beta 3 release with this change to see
that people are happy.

>
>> --- ant/core/trunk/src/main/org/apache/tools/ant/UnknownElement.java
>> (original)
>> +++ ant/core/trunk/src/main/org/apache/tools/ant/UnknownElement.java Thu Mar 27 10:09:53 2008
>> @@ -288,11 +288,14 @@
>>                  ((Task) realThing).execute();
>>              }
>>          } finally {
>> -            // Finished executing the task, null it to allow
>> +            // Finished executing the task
>> +            // null it (unless it has an ID) to allow
>>              // GC do its job
>>              // If this UE is used again, a new "realthing" will be made
>> -            realThing = null;
>> -            getWrapper().setProxy(null);
>> +            if (getWrapper().getId() == null) {
>> +                realThing = null;
>> +                getWrapper().setProxy(null);
>> +            }
>>          }
>>      }
>
> Looks pretty save to me.  This means we might be having a few
> tasks/types living longer than needed, but it seems to be the only way
> people can use ids in their build files and run multiple targets at
> the same time.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@...
> For additional commands, e-mail: dev-help@...
>

--
Steve Loughran                  http://www.1060.org/blogxter/publish/5
Author: Ant in Action           http://antbook.org/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Xavier Hanin]

On Fri, Mar 28, 2008 at 12:11 PM, Steve Loughran <stevel@...> wrote:

> Stefan Bodewig wrote:
> >> Author: peterreilly
> >> Date: Thu Mar 27 10:09:53 2008
> >> New Revision: 641903
> >>
> >> URL: http://svn.apache.org/viewvc?rev=641903&view=rev
> >> Log:
> >> Bugzilla 44689: NPE with multiple targets and id's in task
> >
> > IMHO the bug is serious enough to be fixed in 1.7.1 final and should
> > be merged into the branch.
>
> +1. We could always do a quick beta 3 release with this change to see
> that people are happy.

+1 too

Xavier

[Peter Reilly-2]

+1 from me.

Peter

On Fri, Mar 28, 2008 at 11:18 AM, Xavier Hanin <xavier.hanin@...> wrote:

> On Fri, Mar 28, 2008 at 12:11 PM, Steve Loughran <stevel@...> wrote:
>
>  > Stefan Bodewig wrote:
>  > >> Author: peterreilly
>  > >> Date: Thu Mar 27 10:09:53 2008
>  > >> New Revision: 641903
>  > >>
>  > >> URL: http://svn.apache.org/viewvc?rev=641903&view=rev
>  > >> Log:
>  > >> Bugzilla 44689: NPE with multiple targets and id's in task
>  > >
>  > > IMHO the bug is serious enough to be fixed in 1.7.1 final and should
>  > > be merged into the branch.
>  >
>  > +1. We could always do a quick beta 3 release with this change to see
>  > that people are happy.
>
>  +1 too
>
>  Xavier
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Gilles Scokart]

+1 for me as well (let's put a little bit more presure from the Ivy guys ;-).

To come back to the impact on Ivy, I think we should put a note in our
doc saying that using the settings task with an id requires 1.7.1.

Or is there any workaround we could implement in ivy settings task ?

Gilles

On 28/03/2008, Peter Reilly <peter.kitt.reilly@...> wrote:

> +1 from me.
>
>
>  Peter
>
>
>  On Fri, Mar 28, 2008 at 11:18 AM, Xavier Hanin <xavier.hanin@...> wrote:
>  > On Fri, Mar 28, 2008 at 12:11 PM, Steve Loughran <stevel@...> wrote:
>  >
>  >  > Stefan Bodewig wrote:
>  >  > >> Author: peterreilly
>  >  > >> Date: Thu Mar 27 10:09:53 2008
>  >  > >> New Revision: 641903
>  >  > >>
>  >  > >> URL: http://svn.apache.org/viewvc?rev=641903&view=rev
>  >  > >> Log:
>  >  > >> Bugzilla 44689: NPE with multiple targets and id's in task
>  >  > >
>  >  > > IMHO the bug is serious enough to be fixed in 1.7.1 final and should
>  >  > > be merged into the branch.
>  >  >
>  >  > +1. We could always do a quick beta 3 release with this change to see
>  >  > that people are happy.
>  >
>  >  +1 too
>  >
>  >  Xavier
>  >
>
>
> ---------------------------------------------------------------------
>  To unsubscribe, e-mail: dev-unsubscribe@...
>  For additional commands, e-mail: dev-help@...
>
>

--
Gilles Scokart

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Xavier Hanin]

On Fri, Mar 28, 2008 at 1:23 PM, Gilles Scokart <gscokart@...> wrote:

> +1 for me as well (let's put a little bit more presure from the Ivy guys
> ;-).
>
> To come back to the impact on Ivy, I think we should put a note in our
> doc saying that using the settings task with an id requires 1.7.1.

We can put a reference to the bug, the problem occurs only when you call ant
with multiple targets, each depending on the same settings, so I think
people can use Ivy settings with with ant 1.7.0 without running into the
problem.

>
>
> Or is there any workaround we could implement in ivy settings task ?

I don't think so, I've already tried to find something but I haven't found.
But since ant 1.7.1 will be out pretty soon, if the fix is included I don't
think it's worth investigating more on Ivy side.

Xavier

>
>
> Gilles
>
> On 28/03/2008, Peter Reilly <peter.kitt.reilly@...> wrote:
> > +1 from me.
> >
> >
> >  Peter
> >
> >
> >  On Fri, Mar 28, 2008 at 11:18 AM, Xavier Hanin <xavier.hanin@...>
> wrote:
> >  > On Fri, Mar 28, 2008 at 12:11 PM, Steve Loughran <stevel@...>
> wrote:
> >  >
> >  >  > Stefan Bodewig wrote:
> >  >  > >> Author: peterreilly
> >  >  > >> Date: Thu Mar 27 10:09:53 2008
> >  >  > >> New Revision: 641903
> >  >  > >>
> >  >  > >> URL: http://svn.apache.org/viewvc?rev=641903&view=rev
> >  >  > >> Log:
> >  >  > >> Bugzilla 44689: NPE with multiple targets and id's in task
> >  >  > >
> >  >  > > IMHO the bug is serious enough to be fixed in 1.7.1 final and
> should
> >  >  > > be merged into the branch.
> >  >  >
> >  >  > +1. We could always do a quick beta 3 release with this change to
> see
> >  >  > that people are happy.
> >  >
> >  >  +1 too
> >  >
> >  >  Xavier
> >  >
> >
> >
> > ---------------------------------------------------------------------
> >  To unsubscribe, e-mail: dev-unsubscribe@...
> >  For additional commands, e-mail: dev-help@...
> >
> >
>
>
> --
> Gilles Scokart
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@...
> For additional commands, e-mail: dev-help@...
>
>

--
Xavier Hanin - Independent Java Consultant
http://xhab.blogspot.com/
http://ant.apache.org/ivy/
http://www.xoocode.org/

[Dominique Devienne-2]

On Fri, Mar 28, 2008 at 5:37 AM, Stefan Bodewig <bodewig@...> wrote:
>  > Bugzilla 44689: NPE with multiple targets and id's in task
>
>  IMHO the bug is serious enough to be fixed in 1.7.1 final and should
>  be merged into the branch.

+1. --DD

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Dominique Devienne-2]

On Fri, Mar 28, 2008 at 8:00 AM, Xavier Hanin <xavier.hanin@...> wrote:

> On Fri, Mar 28, 2008 at 1:23 PM, Gilles Scokart <gscokart@...> wrote:
>  > +1 for me as well (let's put a little bit more presure from the Ivy guys ;-).
>  >
>  > To come back to the impact on Ivy, I think we should put a note in our
>  > doc saying that using the settings task with an id requires 1.7.1.
>
>  We can put a reference to the bug, the problem occurs only when you call ant
>  with multiple targets, each depending on the same settings, so I think
>  people can use Ivy settings with with ant 1.7.0 without running into the
>  problem.

For the record, I think it's bad Ant style to use ids in tasks. This
is kept around for BC, but should be discouraged. Using ids on types
OTOH is OK, and essential to types in fact. If a task should refer to
part of another task's internal config, this hints to the config
needing to be its own type referred to by both tasks. It's even OK for
the type to be implicitly created by the first task, when it receives
for example a configid="foo" attribute, so that the second task can
use it using configrefid="foo" or a nested <config refid="foo" />.

I'm not sure how Ivy does it exactly, but somehow I got the feel from
the discussions that it's using the "deprecated" id'd task pattern,
which is a "bad" pattern IMHO. Hopefully I got the wrong feeling ;-)

--DD

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[foamdino]

Hi,

Sorry I'm late with this.

>  IMHO the bug is serious enough to be fixed in 1.7.1 final and should
>  be merged into the branch.

I'm happy to merge this into 1.7.1 and run a 3rd beta cycle (test,
push tarballs, vote etc) if it's a show-stopper for many people

Kev

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[antoinell]

Hello Kevin,


I will be happy if this change 641903 gets merged to 1.7.1 final, as my project is a heavy user of the Ant + Ivy combo.

Regards,

Antoine

-------- Original-Nachricht --------
> Datum: Thu, 3 Apr 2008 08:00:06 +0700
> Von: "Kevin Jackson" <foamdino@...>
> An: "Ant Developers List" <dev@...>
> Betreff: Re: Merge 641903 from trunk to 1.7 branch?

> Hi,
>
> Sorry I'm late with this.
>
> >  IMHO the bug is serious enough to be fixed in 1.7.1 final and should
> >  be merged into the branch.
>
> I'm happy to merge this into 1.7.1 and run a 3rd beta cycle (test,
> push tarballs, vote etc) if it's a show-stopper for many people
>
> Kev
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@...
> For additional commands, e-mail: dev-help@...

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Xavier Hanin]

On Thu, Apr 3, 2008 at 5:54 PM, Antoine Levy-Lambert <antoine@...> wrote:

> Hello Kevin,
>
>
> I will be happy if this change 641903 gets merged to 1.7.1 final, as my
> project is a heavy user of the Ant + Ivy combo.

Note that according to recent discussions on the subject, we will most
likely drop the use of 'id' on the task used to load the settings (which
will probably be 'configure' again, since I seem to be the only one who
don't like using configure where we have make a lot of effort to drop the
confusing 'configuration' in favour of 'settings'). Hence this bug shouldn't
hurt Ivy users in Ivy 2.0 final.

That doesn't prevent fixing the bug in Ant 1.7.1 final, but don't do it only
for Ivy users.

Xavier

>
>
> Regards,
>
> Antoine
>
> -------- Original-Nachricht --------
> > Datum: Thu, 3 Apr 2008 08:00:06 +0700
> > Von: "Kevin Jackson" <foamdino@...>
> > An: "Ant Developers List" <dev@...>
> > Betreff: Re: Merge 641903 from trunk to 1.7 branch?
>
> > Hi,
> >
> > Sorry I'm late with this.
> >
> > >  IMHO the bug is serious enough to be fixed in 1.7.1 final and should
> > >  be merged into the branch.
> >
> > I'm happy to merge this into 1.7.1 and run a 3rd beta cycle (test,
> > push tarballs, vote etc) if it's a show-stopper for many people
> >
> > Kev
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@...
> > For additional commands, e-mail: dev-help@...
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@...
> For additional commands, e-mail: dev-help@...
>
>

--
Xavier Hanin - Independent Java Consultant
http://xhab.blogspot.com/
http://ant.apache.org/ivy/
http://www.xoocode.org/

[foamdino]

Hi,

>  Note that according to recent discussions on the subject, we will most
>  likely drop the use of 'id' on the task used to load the settings (which
>  will probably be 'configure' again, since I seem to be the only one who
>  don't like using configure where we have make a lot of effort to drop the
>  confusing 'configuration' in favour of 'settings'). Hence this bug shouldn't
>  hurt Ivy users in Ivy 2.0 final.
>
>  That doesn't prevent fixing the bug in Ant 1.7.1 final, but don't do it only
>  for Ivy users.
>

Sorry this is a little late - no time at the moment :(

Does this mean that you would like to change anything or should I
merge this change into 1.7.1 now?  I'd rather merge something into
1.7.1 that fixes the problem for ant, and is going to require no
further changes in the future to support Ivy

Thanks,
Kev

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[Xavier Hanin]

On Thu, Apr 24, 2008 at 8:01 AM, Kevin Jackson <foamdino@...> wrote:

> Hi,
>
> >  Note that according to recent discussions on the subject, we will most
> >  likely drop the use of 'id' on the task used to load the settings
> (which
> >  will probably be 'configure' again, since I seem to be the only one who
> >  don't like using configure where we have make a lot of effort to drop
> the
> >  confusing 'configuration' in favour of 'settings'). Hence this bug
> shouldn't
> >  hurt Ivy users in Ivy 2.0 final.
> >
> >  That doesn't prevent fixing the bug in Ant 1.7.1 final, but don't do it
> only
> >  for Ivy users.
> >
>
> Sorry this is a little late - no time at the moment :(
>
> Does this mean that you would like to change anything or should I
> merge this change into 1.7.1 now?  I'd rather merge something into
> 1.7.1 that fixes the problem for ant, and is going to require no
> further changes in the future to support Ivy

We won't need any other change in Ant for Ivy 2.0.0, and don't even need
this fix to be applied. So don't do it only for Ivy.

Xavier


>
> Thanks,
> Kev
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@...
> For additional commands, e-mail: dev-help@...
>
>


--
Xavier Hanin - Independent Java Consultant
http://xhab.blogspot.com/
http://ant.apache.org/ivy/
http://www.xoocode.org/

[Stefan Bodewig]

On Thu, 24 Apr 2008, Kevin Jackson <foamdino@...> wrote:

>>  That doesn't prevent fixing the bug in Ant 1.7.1 final, but don't
>>  do it only for Ivy users.
>>
>
> Sorry this is a little late - no time at the moment :(
>
> Does this mean that you would like to change anything or should I
> merge this change into 1.7.1 now?  I'd rather merge something into
> 1.7.1 that fixes the problem for ant, and is going to require no
> further changes in the future to support Ivy

The problem showed up as a symptom in Ivy but Ivy isn't the only one
affected by it.  I'm all in favor of merging the change regardless.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[foamdino]

Hi,

>  The problem showed up as a symptom in Ivy but Ivy isn't the only one
>  affected by it.  I'm all in favor of merging the change regardless.

The change to UnknownElement is now merged back into BRANCH_17

I will test on windows/ubuntu at some point this week - then I suppose
a further vote on the new srcs before releasing a further beta

Thanks,
Kev

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...

[antoinell]

Hello Kevin,

great work.

I have switched my project to use the 1.7.1beta for all builds already and it is working great.

Regards,

Antoine
-------- Original-Nachricht --------
> Datum: Sun, 27 Apr 2008 09:50:13 +0700
> Von: "Kevin Jackson" <foamdino@...>
> An: "Ant Developers List" <dev@...>
> Betreff: Re: Merge 641903 from trunk to 1.7 branch?

> Hi,
>
> >  The problem showed up as a symptom in Ivy but Ivy isn't the only one
> >  affected by it.  I'm all in favor of merging the change regardless.
>
> The change to UnknownElement is now merged back into BRANCH_17
>
> I will test on windows/ubuntu at some point this week - then I suppose
> a further vote on the new srcs before releasing a further beta
>
> Thanks,
> Kev
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@...
> For additional commands, e-mail: dev-help@...

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@...
For additional commands, e-mail: dev-help@...