FlattenMojo constructs dependencies directly which breaks Maven extension

[wheene]

FlattenMojo obtains ModelBuilder instance by using new directly, making it impossible to use custom impl of this interface provided by extensions. I hit this problem when combined with jgitver - tool for dynamically synthesizing project version from git, implemented as maven extension. Flatten plugin does not see versions produced by the extension leading to build failures.

Problem occurs in versions 1.0.1, 1.1.0 and 1.2.0-SNAPSHOT from current master (ac9eddb)

Pull request which suggests solution exists here #86.
Reproducer case can be found here https://github.com/wheene/jgitver-flatten.

cc @McFoggy

[famod]

⚠️ PR #86 broke certain dependency setups:

I am using cxf-codegen-plugin to generate java classes from wsdl files. For this plugin, I defined the following jaxb dependencies:

<plugin>
    <groupId>org.apache.cxf</groupId>
    <artifactId>cxf-codegen-plugin</artifactId>
    <version>3.0.16</version>
    <configuration>
        <!-- ... -->
    </configuration>
    <dependencies>
        <dependency>
            <groupId>org.glassfish.jaxb</groupId>
            <artifactId>jaxb-runtime</artifactId>
            <version>2.2.11</version>
        </dependency>
        <dependency>
            <groupId>org.glassfish.jaxb</groupId>
            <artifactId>jaxb-xjc</artifactId>
            <version>2.2.11</version>
        </dependency>
        <!-- ... -->
    </dependencies>
</plugin>

When buliding the respective module with the current 1.2.0-SNAPSHOT version of flatten-plugin (which incorporates PR #86), everything works as expected.
But if other modules (without cxf-codegen-plugin) are built before this module, the binding process fails with:

java.lang.ClassNotFoundException: com.sun.xml.bind.v2.model.impl.ModelBuilderI

Running mvn -X ... revealed the following:

[INFO] --- cxf-codegen-plugin:3.0.16:wsdl2java (VidyoServices) @ middleware-common-wsdl ---
[WARNING] The POM for org.glassfish.jaxb:jaxb-runtime:jar:2.2.11 is invalid, transitive dependencies (if any) will not be available: 1 problem was encountered while building the effective model for org.glassfish.jaxb:jaxb-runtime:2.2.11
[ERROR] 'dependencyManagement.dependencies.dependency.systemPath' for com.sun:tools:jar must specify an absolute path but is ${tools.jar} @ 

[WARNING] The POM for org.glassfish.jaxb:jaxb-xjc:jar:2.2.11 is invalid, transitive dependencies (if any) will not be available: 1 problem was encountered while building the effective model for org.glassfish.jaxb:jaxb-xjc:2.2.11
[ERROR] 'dependencyManagement.dependencies.dependency.systemPath' for com.sun:tools:jar must specify an absolute path but is ${tools.jar} @ 

Both jaxb artifacts share the same root parent jaxb-parent which contains:

<!-- ... -->
    <dependencyManagement>
        <dependencies>
            <!-- ... -->
            <!--  JDK dependencies  -->
            <dependency>
                <groupId>com.sun</groupId>
                <artifactId>tools</artifactId>
                <version>1.6</version>
                <scope>system</scope>
                <systemPath>${tools.jar}</systemPath>
            </dependency>
        </dependencies>
    </dependencyManagement>
<!-- ... -->
        <profile>
            <id>default-tools.jar</id>
            <activation>
                <file>
                    <exists>${java.home}/../lib/tools.jar</exists>
                </file>
            </activation>
            <properties>
                <tools.jar>${java.home}/../lib/tools.jar</tools.jar>
            </properties>
        </profile>
        <profile>
            <id>default-tools.jar-mac</id>
            <activation>
                <file>
                    <exists>${java.home}/../Classes/classes.jar</exists>
                </file>
            </activation>
            <properties>
                <tools.jar>${java.home}/../Classes/classes.jar</tools.jar>
            </properties>
        </profile>
<!-- ... -->

This default-tools.jar profile is (obviously) not activated anymore and I am pretty sure that this is caused by the following block: https://github.com/mojohaus/flatten-maven-plugin/blob/master/src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java#L737-L773
As there is no new DefaultModelBuilder instance anymore per execution/module, the injector/selector installation now has a negative global effect.

I strongly recommend to revert df25d03 for now and to rethink the approach.

[wheene]

You are right it would have this effect as before there was one builder per invocation while with #86 there is one per session. I would dare to say though that if you depend on there being dedicated instance per invocation the problem goes deeper than PR 86 which just exposed the problem.

I'm talking about this line https://github.com/mojohaus/flatten-maven-plugin/blob/master/src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java#L188 where the whole MOJO is declared as executionStrategy = "once-per-session" whereas you'd need it to be "always".

@hohwille what is the reason for the Mojo to be "once-per-session"? Would something else break if it were changed to "always"?

@famod could you try if using "always" strategy would solve your problem?

[famod]

@wheene

could you try if using "always" strategy would solve your problem?

Unfortunately, this doesn't solve the problem.

PS:

Would something else break if it were changed to "always"?

At least all tests (including integration tests) are passing.

[McFoggy]

@famod @wheene @hohwille
Can someone states what's next on that issue?
Is changing the strategy to "always" enough to make it work and have a release with #86 included?

[famod]

@McFoggy

Is changing the strategy to "always" enough to make it work

No, this doesn't work for me. I just re-checked with Maven 3.6.2 with and without "always".

[famod]

I created a PR that resets ProfileSelector and ProfileInjector so that the flatten-plugin specific selector and injector do not leak into the rest of the build process.

I am asking myself whether the mutation of the apparently singleton (?) DefaultModelBuilder poses a thread safety issue as well.
One flatten-plugin execution might be in the process of building the model while another concurrently running execution might have already reset selector and injector.
This has to checked! Edit: As expected, concurrently running executions operate on the very same DefaultModelBuilder instance!
A simple synchronized would help in this scenario but what about aother plugins and components mutating DefaultModelBuilder?

At this point I am asking myself whether it would be more safe and enough for jgitver to switch back to the old solution of building a dedicated DefaultModelBuilder, but with the currently "active" ModelProcessor (via injection)?
@McFoggy & @wheene WDYT?