[Bug 53550] New: [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

          Priority: P2
            Bug ID: 53550
          Assignee: notifications@ant.apache.org
           Summary: [PATCH] extensionOf settings applied when task is
                    discarded because of previous declaration
          Severity: normal
    Classification: Unclassified
                OS: Mac OS X 10.4
          Reporter: tpokorny@gmail.com
          Hardware: PC
            Status: NEW
           Version: unspecified
         Component: Core
           Product: Ant

Created attachment 29061
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29061&action=edit
master build file that replicates the problem when invoking the compile target

OK, bare with me, it took a couple of attempts to try and write this so it
comes out correct :P

=== Brief ===
When an imported file contains a target that has the same name as an existing
target from the importing build file, the extensionOf="" settings on the target
are applied even though the target is ignored. The result is that Ant complains
that the target has a circular dependency:

BUILD FAILED
Circular dependency: compile <- all.compile <- compile

Two build files (master and module) are attached that reproduce this problem.

=== Full Story ===
I have a master build file that has a bunch of high-level targets (clean,
compile, release, etc...). I also have a number of "module" build files that
contain targets of the same name. In the master, these are imported (not
included) with a prefix to ensure the targets don't double up.

So in the master, I have a target declared as below, where "all.compile" is an
extension point that also resides in the master:

<target name="compile" depends="all.compile"/>

In the module build file I have a target declared like this:

<target name="compile" extensionOf="all.compile">

The intention is that calling compile on the master will cause all the module
compiles to run. Doing this results in the build failing with a circular
dependency as per the error in the brief description. Running "ant -v" shows
that when importing like this, ant will actually try and create two targets,
one with the prefix as defined in the import statement, and the other without.
The target without the prefix clashes with the one from the master, and results
in the following output:

"Already defined in main or a previous import, ignore compile"

That all makes sense and seems logical. The target is imported under the
prefixed name anyway, so life is all good. However, it seems that even though
the non-prefixed target is ignored, the "extensionOf" settings from the module
are still being applied. 

I checked out the latest trunk from SVN and it looks (in ProjectHelper2) as if
the check to see if there is a target name clash results in the target not
being added to the context and thus ignored. However, failing that check does
not stop the code at that point, and it proceeds on further. When it comes to
extensionOf processing, there is not a similar check to see if the target was
ignored, so the values of the attribute are applied, only they get attached by
the target name, thus linking them to the target from the MASTER file.

Apologies, it's a bit difficult to explain concisely in text.

To me, it seems like there are two options to solve this (patch for one
attached):
  1. If the target is found to clash, stop processing it at that point
  2. Further down, when the extensionOf attribute is processed, skip it if the
target was meant to be discarded.

I have attached a patch for the second option, as I wasn't sure if the decision
not to end processing when finding a clash was deliberate or not and would have
other flow on effects.

Cheers,
Tim

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 53550] [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

--- Comment #3 from Tim Pokorny <tp...@gmail.com> ---
Fail! Looking at it more closely without a screaming child now. The importing
didn't work as I thought.

The patch will cause extensionOf not to be processed if the target is a
duplicate, but because of the way a target with the prefixed name is added, it
also means that the extensions are not applied to it. Fail. Working on an
updated patch now.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 53550] [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

--- Comment #5 from Tim Pokorny <tp...@gmail.com> ---
Created attachment 29067
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29067&action=edit
[PATCH] More complete solution with prefix resolution

Another patch file, but, please take a careful look at it first.

The previous patch just provides a one line solution that will work for
imported build files. All good. But really I wanted to include files, not
import them. When importing with a prefix, it seems that targets only use that
prefix if their names clash with a target that already exists. Sounds sane and
one of the fundamental import/include differences.

I wanted all targets to have the prefixed name, however, switching to use
include broke the build with a different error: the extension point from the
master build file couldn't be found because it was looking it up under the
prefixed name. So, this patch also fixes the core problem of this bug, but it
goes further and adds some additional checks that make it work for imports as
well.

Basically, it passes the prefix name (if there is one) into the extension point
info stack as a fourth element in the array. When the parser resolves the
extension points name, if a fourth element is present, it knows that the target
must have come from an include file and will first look up the point being
extended on by its full, prefixed name, and then fall back to its non-prefixed
version. This means that it'll resolve to an extension point in the local file
in the first instance, and then out to one from the master if there isn't a
local one.

It's a little distasteful as it adds an extra element to the extension info
array that is or isn't present depending on whether the file is or isn't
brought in as an include, but it works. I provide for you guys to
hack/ignore/laugh at, whatever floats your boat :)

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 53550] [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

Nicolas Lalevée <ni...@hibnet.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |1.9.0

--- Comment #6 from Nicolas Lalevée <ni...@hibnet.org> ---
The extension point name resolution is indeed improved with you patch. Patch
applied. Thank you.
There is still some work to do though. Now we try the extension point which
might be in the same file, and then the one in the root file. But there might
be intermediate file in the import stack which we would to resolve the name
against. We cannot do that yet since the ProjectHelper doesn't hold the prefix
stacking.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 53550] [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

Tim Pokorny <tp...@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29063|0                           |1
        is obsolete|                            |

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 53550] [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

--- Comment #1 from Tim Pokorny <tp...@gmail.com> ---
Created attachment 29062
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29062&action=edit
module build file imported by master

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 53550] [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

--- Comment #2 from Tim Pokorny <tp...@gmail.com> ---
Created attachment 29063
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29063&action=edit
Patch file to ignore extensionOf processing for skipped targets

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 53550] [PATCH] extensionOf settings applied when task is discarded because of previous declaration

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=53550

--- Comment #4 from Tim Pokorny <tp...@gmail.com> ---
Created attachment 29066
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29066&action=edit
[PATCH] Updated patch file (second attempt)

OK, so looking harder at the problem there is a simple one-line fix for the
immediate issue. When importing, if the target overlaps with an existing one,
at a point right before extension processing the internal Target object is
replaced by a new one that has the prefixed name.

After this, if the target has an extensionOf value, the basic information is
bundled up and stored in the extension point stack for later processing, the
the relevant target identified by name (as a String). However, when this
happens, the original name of the target is used, even if the target has been
"replaced" by a prefixed on. The second patch contains a one-line fix that gets
the name of the target from the target itself, rather than a previous stored
local variable, thus keeping things happy whether the target has been replaced
with a prefixed-version or not.

Apologies. Too much typing for a one-line patch! :P

-- 
You are receiving this mail because:
You are the assignee for the bug.