Property expansion in macrodef attributes

[Stefan Bodewig <bo...@apache.org>]

Hi all,

for those who don't follow bugzilla comment threads (I know I seldomly
do):

Until Ant 1.8.2 ${} sequences in macrodef attributes were expanded once
before @{} sequences are expanded and once in the context where the @{}
is expanded.  I.e. with

<macrodef name="propertycopy">
  <attribute name="name"/>
  <attribute name="from"/>
  <sequential>
    <property name="@{name}" value="${@{from}}"/>
  </sequential>
</macrodef>

<property name="choice" value="2"/>
<property name="thing.1" value="one"/>
<property name="thing.2" value="two"/>
<property name="thing.3" value="three"/>
<propertycopy to="thing" from="thing.${choice}"/>

@{from} becomes thing.2 and the second expansion does what the FAQ says.

Ant's trunk has removed the ${} expansion that happens before @{}
expansion, breaking the example of our own FAQ and breaking some other
existing macrodefs out there (for example in Netbeans).
https://issues.apache.org/bugzilla/show_bug.cgi?id=52621

Double expansion is unexpected and leads to subtle issues:
https://issues.apache.org/bugzilla/show_bug.cgi?id=42046
https://issues.apache.org/bugzilla/show_bug.cgi?id=41400 

In particular it made some AntUnit tests pass in Ant's own code base
even though the code was actually broken.

Jesse suggests to introduce an attribute to macrodef (or even at the
granularity of the individual attribute definition of a macrodef) to
control ${} expansion.

I'm not sure whether the macrodef writer will always know whether she
wants double-expansion or not.  I also fear it will be quite difficult
to explain in the manual of a macrodefed task ("this attribute may have
${} sequences expanded twice, this other one will not").

Another option would be to not fix the 1.8.2 behavior at all but call it
a feature.  Macrodef writers then can say something like "foo is
implemented as a macrodef so ${} will be expanded twice, see
http://ant.apache.org/faq.html#macrodef-property-expansion - not sure
whether this a good option.

Any other ideas?

Stefan

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

Re: Property expansion in macrodef attributes

[Stefan Bodewig <bo...@apache.org>]

On 2012-02-24, Stefan Bodewig wrote:

> On 2012-02-23, Jesse Glick wrote:

>> On 02/21/2012 10:40 AM, Stefan Bodewig wrote:

>>>> can do the implementation if you agree.

>>> If you can beat me to it, please do.

>> Done in trunk, please review.

> Will do, many thanks.

Merged, I'll build a new release candidate tomorrow morning (European
time).

>> (Especially whether your changes to property-test.xml and
>> propertyhelper-test.xml still stand - I did not understand what was
>> going on with these.)

> I'll try to explain it once I recall the details.

property-test:

This is a pretty complex setup of property files that are loaded with a
prefix and refer to properties that have been defined previously.  There
is a test that used assertPropertyEquals to assert property bar.y has
the value "y".  In reality the value is "${bar.x}" which when expanded
is "y", here double expansion of the property attribute in
assertPropertyEquals (hidden in the chain of macros that make up this
assertion) was hiding the test actually failed.

The code hasn't been fixed but the test disabled.

property-helper-test:

We have a custom property helper and want to assert it works by
comparing two different properties set via the same property helper that
always returns the same Object.  assertPropertyEquals is used for the
test and without double expansion we end up comparing the object to the
result of calling toString in the same object (which is false)

Double expansion turned the Object into its string representation before
invoking equals so the test passed.

In both cases I've added alternatives that prove the code is still
broken in the first case or the code works in the second case (by using
strings rather than objects) and IMHO the tests should stay as they are
for now.

Stefan

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

Re: Property expansion in macrodef attributes

[Stefan Bodewig <bo...@apache.org>]

On 2012-02-23, Jesse Glick wrote:

> On 02/21/2012 10:40 AM, Stefan Bodewig wrote:

>>> can do the implementation if you agree.

>> If you can beat me to it, please do.

> Done in trunk, please review.

Will do, many thanks.

> (Especially whether your changes to property-test.xml and
> propertyhelper-test.xml still stand - I did not understand what was
> going on with these.)

8-)

I'll try to explain it once I recall the details.  One of them actually
failed but double expansion was hiding the failure while in the other
case double expansion led to a different kind of comparison in <equals>
than without it.

Stefan

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

Re: Property expansion in macrodef attributes

[Jesse Glick <je...@oracle.com>]

On 02/21/2012 10:40 AM, Stefan Bodewig wrote:
>> can do the implementation if you agree.
>
> If you can beat me to it, please do.

Done in trunk, please review. (Especially whether your changes to property-test.xml and propertyhelper-test.xml still stand - I did not understand what was going on with 
these.)


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

Re: Property expansion in macrodef attributes

[Stefan Bodewig <bo...@apache.org>]

On 2012-02-21, Jesse Glick wrote:

> Any updates on #52621 and what we want the behavior to be in 1.8.3?

> My suggestion still stands to restore the 1.8.2 behavior by default
> but add an optional parameter to implement the fix of #42046 where
> needed;

Works for me.  I wouldn't want us to issue a warning if the parameter
isn't set, just add the attribute and update the FAQ.

> can do the implementation if you agree.

If you can beat me to it, please do.  I can manage the required merging
for the 1.8.x branch before re-rolling the release.

Stefan

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

Re: Property expansion in macrodef attributes

[Jesse Glick <je...@oracle.com>]

Any updates on #52621 and what we want the behavior to be in 1.8.3? My suggestion still stands to restore the 1.8.2 behavior by default but add an optional parameter to 
implement the fix of #42046 where needed; can do the implementation if you agree.


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

Re: Property expansion in macrodef attributes

[Jesse Glick <je...@oracle.com>]

On 02/14/2012 07:39 AM, Stefan Bodewig wrote:
> Most likely you don't want double expansion in most cases.

Right, but how often would you even notice the difference? Usually property values do not themselves contain interpolable variables, as seen by the fact that this problem 
has just been reported twice since macrodef was introduced.

> The biggest problem I see is AntUnit.  Here the macro is defined in a
> different place and we'd need a new release that requires Ant 1.8.3 to
> fix it.

True; so release a new AntUnit - a much smaller user audience than Ant itself I suppose.

> add the attribute to trunk but make trunk's behavior
> default to not expand ${} twice (flagging it as breaking BWC)

This would cause a major problem for the many scripts out there which rely on the current behavior. They could not run unchanged on 1.9 (and few people will find this 
item in the release notes, much less understand its implications); nor could they be fixed in advance, since the attribute would be rejected by 1.8.x.


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

Re: Property expansion in macrodef attributes

[Stefan Bodewig <bo...@apache.org>]

On 2012-02-12, Jesse Glick wrote:

> On 02/12/2012 05:13 AM, Stefan Bodewig wrote:

>> I'm not sure whether the macrodef writer will always know whether she
>> wants double-expansion or not.

> I would say that if you come across a problem like that mentioned in
> #42046 then you know you do not want double expansion and should
> explicitly turn it off.

Most likely you don't want double expansion in most cases.

> In most cases the script calling the macro will be the same as the
> script defining it, or at least located in the same project, so I
> doubt documentation of the macrodef is an issue;

The biggest problem I see is AntUnit.  Here the macro is defined in a
different place and we'd need a new release that requires Ant 1.8.3 to
fix it.

Right now I'm inclined to revert to the behavior of Ant 1.8.2 for the
1.8.x branch, add the attribute to trunk but make trunk's behavior
default to not expand ${} twice (flagging it as breaking BWC).

> though discoverability of the flag in the macrodef task may be poor
> even with a FAQ entry.

Agreed.

> If this problem had been noticed when macrodef was first introduced,
> then single expansion would be the more intuitive behavior, especially
> if #29347 were fixed properly so that ${thing.${choice}} works the way
> people expect it to, but it is too late to change the default behavior
> of macrodef.

I'm with Matt in that property helpers already provide the foundation
for a solution and the props Antlib (anybody wants to cut a release?)
implements it.

I also understand Ant users don't like to add more dependencies,
something to keep in mind if we ever want to redesign Ant.

Stefan

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

Re: Property expansion in macrodef attributes

[Jesse Glick <je...@oracle.com>]

On 02/12/2012 05:13 AM, Stefan Bodewig wrote:
> I'm not sure whether the macrodef writer will always know whether she
> wants double-expansion or not.

I would say that if you come across a problem like that mentioned in #42046 then you know you do not want double expansion and should explicitly turn it off.

In most cases the script calling the macro will be the same as the script defining it, or at least located in the same project, so I doubt documentation of the macrodef 
is an issue; though discoverability of the flag in the macrodef task may be poor even with a FAQ entry.

If this problem had been noticed when macrodef was first introduced, then single expansion would be the more intuitive behavior, especially if #29347 were fixed properly 
so that ${thing.${choice}} works the way people expect it to, but it is too late to change the default behavior of macrodef.


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