repaint-io / maven-tiles

Injecting maven configurations by composition rather than inheritance
155 stars 32 forks source link

Plugin Execution Order #118

Closed bmlong137 closed 3 years ago

bmlong137 commented 3 years ago

The order of executions of the same plugin group/artifact appears to be happening based on some underlying characteristic instead of anything that can be configured. For instance:

tile1.xml (T1)

<project>
  <build>
    <plugins>
      <plugin>
        <groupId>G1</groupId>
        <artifactId>A1</artifactId>
        <version>V1</version>
        <executions>
          <execution>
             <id>E1</id>
             <phase>P1</phase>
          </execution>
        </executions>
      </plugin>
    </plugins>
  </build>
</project>

tile2.xml (T2)

<project>
  <build>
    <plugins>
      <plugin>
        <groupId>G1</groupId>
        <artifactId>A1</artifactId>
        <version>V1</version>
        <executions>
          <execution>
             <id>E2</id>
             <phase>P1</phase>
          </execution>
        </executions>
      </plugin>
    </plugins>
  </build>
</project>

pom.xml

<project>
  ...
  <tiles>
    <tile>group:T1:version</tile>
    <tile>group:T2:version</tile>
  </tiles>
  ...
</project>

No matter what I do with the tile order in the POM, or how I name the executions (IDs) alphabetically, the execution order does not change between E1 and E2.

I took a look at the source to see if an unordered data set could be at fault, causing a hash code to be the predictor, but I couldn't find it. However, it appears that it could still be something like a hash code dictating it. Maven itself guarantees the execution order to be in the order that execution elements show up in the POM...parent first. So I would have thought tile order would matter...

If I am write that this is unexpected behavior, I could create a test case. I might just hack away at it myself. It is causing me headaches.

bmlong137 commented 3 years ago

I have found that if I use different plugin artifacts on the same phase across multiple tiles, the execution order is the reverse of the declared tile order. I am currently assuming this is the case with the same plugin artifact, but with multiple executions across multiple tiles.

The problem seems to be when there is an execution declared inside a tile that is inside a tile. Those tile-in-tile declarations are going first no matter what. And that could be the heart of the issue.

talios commented 3 years ago

Hey @bmlong137 - thanks for the ticket. Mmmm, I'm in times (and have been for awhile) about those tiles-with-tiles being largely smells and if we should even support that.

If you can add a PR/branch somewhere with an invoker test that would be awesome and help out a lot.

bmlong137 commented 3 years ago

Created IT case with pull request #119. Have at it. :+1:

bmlong137 commented 3 years ago

In an attempt to flush this out, I did some research in the code. I am a Java developer; not groovy. And this is my first look into the code behind Maven Tiles.

The merging of execution blocks seems sequential. This is as one would expect, as the Maven spec calls for the execution of same-phase executions in the order in which the appear in the POM; parent first. The problem is centered on the processing order of the tiles, not the execution blocks.

I suspected the iteration of tiles is happening in the loadAllDiscoveredTiles method. Once I realized I should ignore tile-merge-source and tile-merge-target, I found that this is just recursively adding tiles to the processedTiles map...which I suspect is order (LinkedHashMap). This means the order in the map would be the order in the POM, which tiles-in-tiles after the declaring tile, but before the next.

For instance, the POM declares TA, TB, and TC. TB declares TM and TN and TM declares TZ. This would make the map order: TA, TB, TM, TZ, TN, and TC.

bmlong137 commented 3 years ago

Ok, I see now.

The processConfigurationTile method adds all the immediate tiles to the unprocessedTiles map. The loadAllDiscoveredtiles method is invoked afterwards, which then adds all the tiles-in-tiles to the unprocessedTiles map....while moving tiles form "unprocessed" to "processed". In the example above, this results in the following: TA, TB, TC, TM, TZ, and TN. The tiles-in-tiles are exactly as I would expect. It is the root tiles that are not.

bmlong137 commented 3 years ago

One of two ways to resolve this.

  1. We could inject the processing of tiles-in-tiles to the front of the unprocessedTiles map. This would be difficult as it would screw up the tiles-in-tiles order, which is currently correct.
  2. We could copy/clear the unprocessedTiles map at the beginning of loadAllDiscoveredTiles. Then process those in a loop. The current loop would be inside that loop. This would ensure the tiles-in-tiles are added to the processedTiles before the next root tile is added to it.

I am going to go ahead an make the change as (2) and throw up a pull request.

talios commented 3 years ago

Thanks for the PR.

bmlong137 commented 3 years ago

thanks for including it. 👍