You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Peter Reilly <pe...@apache.org> on 2005/05/12 16:44:36 UTC
Re: [Bug 28444] - Import: Target Handling Bug
Stefan Bodewig wrote:
>>Be that as it may, the current behaviour is a bit silly -
>>i.e. inconsistent.
>>
>>
>
>big +1
>
>But we should discuss it here instead of in bugzilla - nobody's going
>to follow it there. I know that I don't.
>
>
No problem.
The bug is : http://issues.apache.org/bugzilla/show_bug.cgi?id=28444
The example given is:
<project name="A">
<target name="x"/>
</project>
<project name="B">
<import file="A.xml"/>
<target name="x" depends="A.x"/>
</project>
<project name="C">
<import file="A.xml"/>
<import file="B.xml"/>
</project>
Succeeds:
ant -f A.xml x
ant -f B.xml x
Fails:
ant -f C.xml x
BUILD FAILED
Target `A.x' does not exist in this project. It is used from target `B.x'.
My comment is:
I never liked the target renaming stuff - it seems a bit strange.
Be that as it may, the current behaviour is a bit silly - i.e. inconsistent.
The target gets renamed if there another target of the same name, but
not otherwise - how can one write a proper reusable import file using the
rename feature in this case?
The fix will have a small overhead - the target object needs to be
cloned and given a a new name, whereas the current code just renames the
target object.
The changes to ant for this is:
Index: src/main/org/apache/tools/ant/Target.java
===================================================================
RCS file: /home/cvs/ant/src/main/org/apache/tools/ant/Target.java,v
retrieving revision 1.58
diff -u -3 -p -r1.58 Target.java
--- src/main/org/apache/tools/ant/Target.java 10 Mar 2005 12:50:57 -0000 1.58
+++ src/main/org/apache/tools/ant/Target.java 12 May 2005 14:41:25 -0000
@@ -52,12 +52,28 @@ public class Target implements TaskConta
/** Description of this target, if any. */
private String description = null;
- /** Sole constructor. */
+ /** Default constructor. */
public Target() {
//empty
}
/**
+ * Cloning constructor.
+ * @param other the Target to clone.
+ */
+ public Target(Target other) {
+ this.name = other.name;
+ this.ifCondition = other.ifCondition;
+ this.unlessCondition = other.unlessCondition;
+ this.dependencies = other.dependencies;
+ this.location = other.location;
+ this.project = other.project;
+ this.description = other.description;
+ // The children are added to after this cloning
+ this.children = other.children;
+ }
+
+ /**
* Sets the project this target belongs to.
*
* @param project The project this target belongs to.
Index: src/main/org/apache/tools/ant/helper/ProjectHelper2.java
===================================================================
RCS file: /home/cvs/ant/src/main/org/apache/tools/ant/helper/ProjectHelper2.java,v
retrieving revision 1.54
diff -u -3 -p -r1.54 ProjectHelper2.java
--- src/main/org/apache/tools/ant/helper/ProjectHelper2.java 26 Apr 2005 11:55:18 -0000 1.54
+++ src/main/org/apache/tools/ant/helper/ProjectHelper2.java 12 May 2005 14:41:26 -0000
@@ -815,38 +815,39 @@ public class ProjectHelper2 extends Proj
+ "a name attribute", context.getLocator());
}
- Hashtable currentTargets = project.getTargets();
+ // Check if this target is in the current build file
+ if (context.getCurrentTargets().get(name) != null) {
+ throw new BuildException(
+ "Duplicate target '" + name + "'", target.getLocation());
+ }
- // If the name has already been defined ( import for example )
- if (currentTargets.containsKey(name)) {
- if (context.getCurrentTargets().get(name) != null) {
- throw new BuildException(
- "Duplicate target '" + name + "'", target.getLocation());
- }
- // Alter the name.
- if (context.getCurrentProjectName() != null) {
- String newName = context.getCurrentProjectName()
- + "." + name;
- project.log("Already defined in main or a previous import, "
- + "define " + name + " as " + newName,
- Project.MSG_VERBOSE);
- name = newName;
- } else {
- project.log("Already defined in main or a previous import, "
- + "ignore " + name, Project.MSG_VERBOSE);
- name = null;
- }
+ if (depends.length() > 0) {
+ target.setDepends(depends);
}
- if (name != null) {
+ Hashtable projectTargets = project.getTargets();
+ boolean usedTarget = false;
+ // If the name has not already been defined define it
+ if (projectTargets.containsKey(name)) {
+ project.log("Already defined in main or a previous import, "
+ + "ignore " + name, Project.MSG_VERBOSE);
+ } else {
target.setName(name);
context.getCurrentTargets().put(name, target);
project.addOrReplaceTarget(name, target);
+ usedTarget = true;
}
- // take care of dependencies
- if (depends.length() > 0) {
- target.setDepends(depends);
+ if (context.isIgnoringProjectTag() && context.getCurrentProjectName() != null
+ && context.getCurrentProjectName().length() != 0) {
+ // In an impored file (and not completely
+ // ignoring the project tag)
+ String newName = context.getCurrentProjectName()
+ + "." + name;
+ Target newTarget = usedTarget ? new Target(target) : target;
+ newTarget.setName(newName);
+ context.getCurrentTargets().put(newName, newTarget);
+ project.addOrReplaceTarget(newName, newTarget);
}
}
Cheers,
Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Stefan Bodewig <bo...@apache.org>.
On Fri, 13 May 2005, Nicola Ken Barozzi <ni...@apache.org> wrote:
> Stefan Bodewig wrote:
>> On Thu, 12 May 2005, Dominique Devienne <dd...@gmail.com>
>> wrote:
> ...
>>>And targets from different imported build files that conflict
>>>(multiple inheritance) should raise an error unless explicitly
>>>imported "as" given by the importer (not the name of the imported
>>>project as is the case now).
>> Can't we have this as an addition anyway?
>
> Why?
To give the importer control over the prefix and make it a bit more
independent from the author of the imported file. I may even want to
import to files coming from two different authors but having the same
project name.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Nicola Ken Barozzi <ni...@apache.org>.
Stefan Bodewig wrote:
> On Thu, 12 May 2005, Dominique Devienne <dd...@gmail.com> wrote:
...
>>And targets from different imported build files that conflict
>>(multiple inheritance) should raise an error unless explicitly
>>imported "as" given by the importer (not the name of the imported
>>project as is the case now).
>
> Can't we have this as an addition anyway?
Why?
--
Nicola Ken Barozzi nicolaken@apache.org
- verba volant, scripta manent -
(discussions get forgotten, just code remains)
---------------------------------------------------------------------
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Stefan Bodewig <bo...@apache.org>.
On Thu, 12 May 2005, Dominique Devienne <dd...@gmail.com> wrote:
> The imported build names should never appear in the importer, we
> should have an explicit override attribute (or something) that tells
> Ant the target is meant to override a target in an imported build
> (which fails the build if it does not override any target),
This implies that you'd know all target names of all files you import.
With the ideas of importing files you shipped within jars floating
around, I start to wonder whether this will hold true in the future.
> And targets from different imported build files that conflict
> (multiple inheritance) should raise an error unless explicitly
> imported "as" given by the importer (not the name of the imported
> project as is the case now).
Can't we have this as an addition anyway?
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Dominique Devienne <dd...@gmail.com>.
> I never liked the target renaming stuff - it seems a bit strange.
> Be that as it may, the current behaviour is a bit silly - i.e. inconsistent.
> The target gets renamed if there another target of the same name, but
> not otherwise - how can one write a proper reusable import file using the
> rename feature in this case?
I never liked target renaming either. Sure, renaming all targets is
more consistent, and I'm +1 for it, but it's still very flawed. The
imported build names should never appear in the importer, we should
have an explicit override attribute (or something) that tells Ant the
target is meant to override a target in an imported build (which fails
the build if it does not override any target), and have a "super"
target to possibly rely on, instead of "renamed" target. And targets
from different imported build files that conflict (multiple
inheritance) should raise an error unless explicitly imported "as"
given by the importer (not the name of the imported project as is the
case now). The "as" name used would then be supplied to the "super"
keyword.
Sorry, I couldn't resist bring all that up again. I know it's water on
the bridge, and that I lost this argument a long time ago, but it
still bugs me.
But eh, I gave my +1 to systematic target renaming ;-) --DD
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Stefan Bodewig <bo...@apache.org>.
On Fri, 13 May 2005, Peter Reilly <pe...@apache.org> wrote:
> Stefan Bodewig wrote:
>>Do we really need to create a complete clone? Can't we achieve the
>>same with a more lazy approach?
>>
>>Turning Matt's idea around:
>
> I do not see what that will buy us much in terms of efficeny.
Frankly, neither do I. I don't have a real problem with your cloning
approach either.
> However, there should be no need to replace the placeholder as the
> overridding targets are processed first.
Yes, Matt already told me that my memory was failing on me.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Peter Reilly <pe...@apache.org>.
Stefan Bodewig wrote:
>On Thu, 12 May 2005, Peter Reilly <pe...@apache.org> wrote:
>
>
>
>>The target gets renamed if there another target of the same name,
>>but not otherwise - how can one write a proper reusable import file
>>using the rename feature in this case?
>>
>>
>
>I'm strongly +1 for at least providing renamed aliases for all
>targets, independent on how we achieve that.
>
>
>
>>The fix will have a small overhead - the target object needs to be
>>cloned and given a a new name, whereas the current code just renames
>>the target object.
>>
>>
>
>Do we really need to create a complete clone? Can't we achieve the
>same with a more lazy approach?
>
>Turning Matt's idea around:
>
>(1) Target "foo" is in project "bar".
>(2a) There already is a target "foo" from the file that imported
> "bar", use the current code, we are ready, "bar.foo" is there.
>(2b) There is no other target "foo" yet. Create an empty placeholder
> target "bar.foo" that depends on "foo".
>
> If then later a target "foo" is found in the importing buildfile,
> replace the placeholder "bar.foo" with the initial "foo" target.
>
>
I do not see what that will buy us much in terms of efficeny. However,
there should be
no need to replace the placeholder as the overridding targets are
processed first.
>Wouldn't that work, stay backwards compatible and hide "bar." whenever
>possible?
>
>
Yes.. I think so.
Peter
>Stefan
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
>For additional commands, e-mail: dev-help@ant.apache.org
>
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Stefan Bodewig <bo...@apache.org>.
On Thu, 12 May 2005, Peter Reilly <pe...@apache.org> wrote:
> The target gets renamed if there another target of the same name,
> but not otherwise - how can one write a proper reusable import file
> using the rename feature in this case?
I'm strongly +1 for at least providing renamed aliases for all
targets, independent on how we achieve that.
> The fix will have a small overhead - the target object needs to be
> cloned and given a a new name, whereas the current code just renames
> the target object.
Do we really need to create a complete clone? Can't we achieve the
same with a more lazy approach?
Turning Matt's idea around:
(1) Target "foo" is in project "bar".
(2a) There already is a target "foo" from the file that imported
"bar", use the current code, we are ready, "bar.foo" is there.
(2b) There is no other target "foo" yet. Create an empty placeholder
target "bar.foo" that depends on "foo".
If then later a target "foo" is found in the importing buildfile,
replace the placeholder "bar.foo" with the initial "foo" target.
Wouldn't that work, stay backwards compatible and hide "bar." whenever
possible?
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Bugzilla or dev-list (was Re: [Bug 28444] - Import: Target
Handling Bug)
Posted by Stefan Bodewig <bo...@apache.org>.
On Thu, 12 May 2005, Matt Benson <gu...@yahoo.com> wrote:
> Here's my comment from the bug since Stefan wants to
> be on-list: :)
I'll go back to the original thread in a minute, just wanted to
explain why I asked Peter to discuss it on the list.
When I see bugzilla reports that look as if some committer has talen
care of it, I tend to ignore subsequent mails from the report, This
means I could be missing a discussion thread.
Furthermore I live and breathe in my mail client all day, I hate it
when I have to leave it to respond to a mail. Some bug tracking
systems can be used by email as well, I even think Bugzilla could, but
not our installation. If I have to start a brwoser in order to write
the two-character sequence "+1", I may just not do it.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: [Bug 28444] - Import: Target Handling Bug
Posted by Matt Benson <gu...@yahoo.com>.
Here's my comment from the bug since Stefan wants to
be on-list: :)
<quote>
I also support fixing this. However, instead of
cloning the entire target,
couldn't we:
a) unconditionally rename the imported target
b) if the target has NOT been overridden, create a new
target that depends on
the imported target
?
-Matt
</quote>
The only issue with this is logging. I guess it
really doesn't matter; the cloning approach shouldn't
be much heavier since it's as shallow as it is...
:)
--- Peter Reilly <pe...@apache.org> wrote:
> Stefan Bodewig wrote:
>
> >>Be that as it may, the current behaviour is a bit
> silly -
> >>i.e. inconsistent.
> >>
> >>
> >
> >big +1
> >
> >But we should discuss it here instead of in
> bugzilla - nobody's going
> >to follow it there. I know that I don't.
> >
> >
> No problem.
>
> The bug is :
>
http://issues.apache.org/bugzilla/show_bug.cgi?id=28444
> The example given is:
>
> <project name="A">
> <target name="x"/>
> </project>
>
> <project name="B">
> <import file="A.xml"/>
> <target name="x" depends="A.x"/>
> </project>
>
> <project name="C">
> <import file="A.xml"/>
> <import file="B.xml"/>
> </project>
>
> Succeeds:
> ant -f A.xml x
> ant -f B.xml x
>
> Fails:
> ant -f C.xml x
>
> BUILD FAILED
>
>
> Target `A.x' does not exist in this project. It is
> used from target `B.x'.
>
>
> My comment is:
>
> I never liked the target renaming stuff - it seems a
> bit strange.
> Be that as it may, the current behaviour is a bit
> silly - i.e. inconsistent.
> The target gets renamed if there another target of
> the same name, but
> not otherwise - how can one write a proper reusable
> import file using the
> rename feature in this case?
>
> The fix will have a small overhead - the target
> object needs to be
> cloned and given a a new name, whereas the current
> code just renames the
> target object.
>
> The changes to ant for this is:
> Index: src/main/org/apache/tools/ant/Target.java
>
===================================================================
> RCS file:
>
/home/cvs/ant/src/main/org/apache/tools/ant/Target.java,v
> retrieving revision 1.58
> diff -u -3 -p -r1.58 Target.java
> --- src/main/org/apache/tools/ant/Target.java 10
> Mar 2005 12:50:57 -0000 1.58
> +++ src/main/org/apache/tools/ant/Target.java 12
> May 2005 14:41:25 -0000
> @@ -52,12 +52,28 @@ public class Target implements
> TaskConta
> /** Description of this target, if any. */
> private String description = null;
>
> - /** Sole constructor. */
> + /** Default constructor. */
> public Target() {
> //empty
> }
>
> /**
> + * Cloning constructor.
> + * @param other the Target to clone.
> + */
> + public Target(Target other) {
> + this.name = other.name;
> + this.ifCondition = other.ifCondition;
> + this.unlessCondition =
> other.unlessCondition;
> + this.dependencies = other.dependencies;
> + this.location = other.location;
> + this.project = other.project;
> + this.description = other.description;
> + // The children are added to after this
> cloning
> + this.children = other.children;
> + }
> +
> + /**
> * Sets the project this target belongs to.
> *
> * @param project The project this target
> belongs to.
> Index:
>
src/main/org/apache/tools/ant/helper/ProjectHelper2.java
>
===================================================================
> RCS file:
>
/home/cvs/ant/src/main/org/apache/tools/ant/helper/ProjectHelper2.java,v
> retrieving revision 1.54
> diff -u -3 -p -r1.54 ProjectHelper2.java
> ---
>
src/main/org/apache/tools/ant/helper/ProjectHelper2.java
> 26 Apr 2005 11:55:18 -0000 1.54
> +++
>
src/main/org/apache/tools/ant/helper/ProjectHelper2.java
> 12 May 2005 14:41:26 -0000
> @@ -815,38 +815,39 @@ public class ProjectHelper2
> extends Proj
> + "a name attribute",
> context.getLocator());
> }
>
> - Hashtable currentTargets =
> project.getTargets();
> + // Check if this target is in the
> current build file
> + if
> (context.getCurrentTargets().get(name) != null) {
> + throw new BuildException(
> + "Duplicate target '" + name +
> "'", target.getLocation());
> + }
>
> - // If the name has already been defined
> ( import for example )
> - if (currentTargets.containsKey(name)) {
> - if
> (context.getCurrentTargets().get(name) != null) {
> - throw new BuildException(
> - "Duplicate target '" + name
> + "'", target.getLocation());
> - }
> - // Alter the name.
> - if (context.getCurrentProjectName()
> != null) {
> - String newName =
> context.getCurrentProjectName()
> - + "." + name;
> - project.log("Already defined in
> main or a previous import, "
> - + "define " + name + " as "
> + newName,
> -
> Project.MSG_VERBOSE);
> - name = newName;
> - } else {
> - project.log("Already defined in
> main or a previous import, "
> - + "ignore " + name,
> Project.MSG_VERBOSE);
> - name = null;
> - }
> + if (depends.length() > 0) {
> + target.setDepends(depends);
> }
>
> - if (name != null) {
> + Hashtable projectTargets =
> project.getTargets();
> + boolean usedTarget = false;
> + // If the name has not already been
> defined define it
> + if (projectTargets.containsKey(name)) {
> + project.log("Already defined in
> main or a previous import, "
> + + "ignore " + name,
> Project.MSG_VERBOSE);
> + } else {
> target.setName(name);
>
> context.getCurrentTargets().put(name, target);
> project.addOrReplaceTarget(name,
> target);
> + usedTarget = true;
> }
>
> - // take care of dependencies
> - if (depends.length() > 0) {
> - target.setDepends(depends);
> + if (context.isIgnoringProjectTag() &&
> context.getCurrentProjectName() != null
> + &&
> context.getCurrentProjectName().length()
=== message truncated ===
Discover Yahoo!
Stay in touch with email, IM, photo sharing and more. Check it out!
http://discover.yahoo.com/stayintouch.html
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org