bnd-maven-plugin doesn't inherit properties from parent when a bnd file is present

[timothyjward]

If I have a Maven project with the following configuration in a parent pom:

<plugin>
    <groupId>biz.aQute.bnd</groupId>
        <artifactId>bnd-maven-plugin</artifactId>
        <version>4.0.0-SNAPSHOT</version>
        <configuration>
            <bnd><![CDATA[
Bundle-SymbolicName: ${project.groupId}.${project.artifactId}
-sources: true
-contract: *
]]></bnd>
        </configuration>
        <executions>
            <execution>
                <goals>
                    <goal>bnd-process</goal>
                </goals>
            </execution>
        </executions>
    </plugin>

Then that configuration is picked up by child modules, which is good.

If I then add a bnd.bnd file to the child module (so as to add some extra metadata) then this parent configuration is lost. For example if I want to write a persistence bundle using

Meta-Persistence: <<EMPTY>>

Then I have to respecify all of the inherited data. This is annoying, and not what I would expect to happen.

[bjhargrave]

I am not sure how that would be possible.

bnd/maven/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/BndMavenPlugin.java

Lines 284 to 289 in d9622f9

	private File loadProjectProperties(Builder builder, MavenProject project) throws Exception {
	// Load parent project properties first
	MavenProject parentProject = project.getParent();
	if (parentProject != null) {
	loadProjectProperties(builder, parentProject);
	}

The code recurses to the deepest ancestor and then adds the bnd properties from each project on the way back up. The only way a project's parent's bnd properties would be ignored is if a project did not have a parent.

[bjhargrave]

Run with --debug to see what debug messages are output by the bnd-maven-plugin. It should show debug messages for each project it loads bnd properties from.

[timothyjward]

I have worked out what the issue is, and it's not very pleasant...

The parent pom file is following maven best practices and declaring the bnd-maven-plugin in the <pluginManagement> section of its pom. This is then made part of the build by being declared in the <plugins> section of the leaf module.

This then becomes a problem because this call doesn't return the configuration in the parent pom (as the plugin isn't part of the build, just the plugin management).

bnd/maven/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/BndMavenPlugin.java

Line 292 in d9622f9

Xpp3Dom configuration = project.getGoalConfiguration("biz.aQute.bnd", "bnd-maven-plugin",

As a result the configuration is only loaded in the leaf module, at which point the local bnd.bnd file is processed and returned here, ignoring the inline declaration from the parent

bnd/maven/bnd-maven-plugin/src/main/java/aQute/bnd/maven/plugin/BndMavenPlugin.java

Line 311 in d9622f9

return bndFile;

I have put together an integration test case which shows this failing, but I'm not sure how to fix the failure without breaking other things. The commit with the failing test is available at timothyjward@9146d5e if that is helpful

[bjhargrave]

There were bugs in your test code. I have fixed them and have a change which makes this work. PR shortly.

[bjhargrave]

I am having second thoughts about my PR. Need to discuss tomorrow with @timothyjward.

[timothyjward]

There were bugs in your test code. I have fixed them and have a change which makes this work. PR shortly.

I’m not totally surprised - Maven tests are hard to validate by sight. I’m glad you came up with a fix, I couldn’t see anything obvious that wouldn’t break some of the existing tests!

[timothyjward]

I am having second thoughts about my PR. Need to discuss tomorrow with @timothyjward.

Having looked at the commit I think I can see why you are having second thoughts. There is a clear tension between the way that the bnd-maven-plugin tries to cope with a mixture of inline and external configuration.

From my understanding there are effectively three ways to get configuration into an execution of the bnd-maven-plugin:

1. Including a bnd.bnd file in the current project (i.e. the project being built). This file can be configured to have another name.
2. Using a <bnd> CDATA section to define configuration in the pom
3. Using a <bnd> CDATA section to include a .bnd file from the local project (i.e. not necessarily the project being built)

From a pure maven perspective plugins are expected to be configured using only option 2, although a variation of option 3 is also used where external files in the current project (i.e. the one being built) are used. When Maven plugins are configured the configurations are inherited through the parent and flattened. This includes configuration at the plugin level, and for the specific execution, with each child's changes are merged on top. This is the way that configuration available to inject into plugins is calculated, and it is applied once to the current project (not separately at each level in the hierarchy. From a bnd perspective this is a bit like every bnd file starting with -include: ~../pom.xml.

If we want the bnd-maven-plugin to follow "the maven way" then it should be interpreting its configuration once in the module currently being built, not programatically searching the hierarchy, then applying any files from the current project. This will give familiar behaviour for Maven users. Doing this would have the following side-effects:

• It would not be possible to use external bnd descriptors from parent projects, either as files, or as includes in a CDATA section. Any -include would search for a local file.
• It would not be possible to apply CDATA sections in-turn from the parent hierarchy. Maven can't merge the character content of the <bnd> element, so expressing it in configuration would always override a <bnd> from the parent.

We may be able to tweak around the edges, but we probably need to discuss this more widely, as it would be a moderate change from the current model.

[timothyjward]

@njbartlett, @rotty3000 and others may have a view about this

[njbartlett]

It would not be possible to use external bnd descriptors from parent projects, either as files, or as includes in a CDATA section. Any -include would search for a local file.

That's a disappointing loss of functionality, but I agree it is closer to what a Maven user expects since one is not supposed to have any visibility of resources in the parent project (and when the project is referenced on a repository the source resources are not present).

It would not be possible to apply CDATA sections in-turn from the parent hierarchy. Maven can't merge the character content of the element, so expressing it in configuration would always override a from the parent.

This is not clear to me. Does this mean you can only have one CDATA section anywhere in the parent hierarchy? And the closest one to the leaf project replaces anything in the parents?

[timothyjward]

This is not clear to me. Does this mean you can only have one CDATA section anywhere in the parent hierarchy? And the closest one to the leaf project replaces anything in the parents?

Yes - this is the way that maven configuration overriding works unfortunately.

[njbartlett]

Understood, and I think that's what we have to live with. So long as leaf projects are still able to customize (i.e. via merging rather than wholesale replacing) the bnd instructions using a bnd.bnd file.

In that case the usual pattern will be that a single parent project contains the default config, and leaf projects that need to extend the defaults provide a bnd.bnd file. Also ideally the cases in which bnd.bnd files are needed at all will be drastically reduced as more useful and standardized Java annotations become available.

[bjhargrave]

I have updated the PR to allow a leaf project to have a <bnd> or <bndfile> that merges with the <bnd> or <bndfile> in pluginManagement configuration.

[kwin]

@bjhargrave Can you add a milestone for that? I guess this will be released with 4.0? Also it would be great to add a section to the readme (https://github.com/bndtools/bnd/blob/master/maven/bnd-maven-plugin/README.md) covering the inheritance aspects.

[bjhargrave]

I guess this will be released with 4.0?

Yes, it would be in 4.0.

Also it would be great to add a section to the readme covering the inheritance aspects.

Feel like making a PR? :-)

[kwin]

Not sure I fully understand how inheritance is implemented after this commit. In the documentation we should add a link towards bnd property merging (is there an official documentation around that?) and elaborate which Maven configuration properties are evaluated in which order. Also we should make clear which maven configuration properties are not merged, i.e. are mutually exclusive (<bnd> vs <bndfile>).

[bjhargrave]

Configuration is loaded from parents first. If the parent does not have a configuration, we look for a configuration in pluginManagement. For each project, the bnd file is used, if present, rather than the element.

[kwin]

This is still not enough. Just think about the case where you have a parent pom like this

<build>
  <pluginManagement>
    <plugins>
      <plugin>
         <groupId>biz.aQute.bnd</groupId>
         <artifactId>bnd-maven-plugin</artifactId>
         <configuration>
            <bnd>my instructions</bnd>
         </configuration>
      </plugin>
    </plugins>
  </pluginManagement>
  <plugins>
    <plugin>
      <groupId>biz.aQute.bnd</groupId>
      <artifactId>bnd-maven-plugin</artifactId>
      <configuration>
         <manifestPath>${project.build.outputDirectory}/META-INF-FROM-BND/MANIFEST.MF</manifestPath>
      </configuration>
    </plugin>
  </plugins>
</build>

There you would still need to evaluate pluginManagement although the plugin is also configured in plugins because both configurations are not necessarily overlapping.

[kwin]

I made a proposal for the documentation of the inheritance in #2272.

[bjhargrave]

There you would still need to evaluate pluginManagement

Assuming the pluginManagement section applies to the same project that declares it, then Maven will have already included it in the plugins configuration and bnd-maven-plugin does not need to do anything special.

Assuming the pluginManagement section does NOT apply to the same project that declares it, then bnd-maven-plugin should not process it.

So in either case, we are ok.

[kwin]

Assuming the pluginManagement section applies to the same project that declares it, then Maven will have already included it in the plugins configuration and bnd-maven-plugin does not need to do anything special.

Are you sure about this? If I'm correct then MavenProject->getBuildPlugins()->get(0)->getConfiguration() will not merge pluginManagement with the current plugin configuration but rather only extracts the configuration of the first defined plugin (it will consider the effective pom, but again this is only doing merging on XML level (for same element names). Neither does it merge global configuration with execution configuration. To extract the plugin management you rather have to use MavenProject->getPluginManagement()->getPlugins()->get(0)->getConfiguration(). I am trying to find the reference implementation in Maven which does the merging when parameters are injected.

To help me with that, do you provide a SNAPSHOT repository containing biz.aQute.bnd:biz.aQute.bndlib:jar:4.0.0-SNAPSHOT?

[bjhargrave]

To help me with that, do you provide a SNAPSHOT repository containing biz.aQute.bnd:biz.aQute.bndlib:jar:4.0.0-SNAPSHOT?

Yes, the Cloudbees build has a maven shaped repo at https://bndtools.ci.cloudbees.com/job/bnd.master/lastSuccessfulBuild/artifact/dist/bundles/

[kwin]

@bjhargrave
Please check out the modified test at https://github.com/kwin/bnd/tree/inheritance-fails-when-mixing-pluginMgmt-and-plugins to see that Maven does not correctly merge pluginManagement configuration with plugins configuration for you. You can also check out https://github.com/apache/maven/blob/master/maven-model-builder/src/main/java/org/apache/maven/model/plugin/DefaultPluginConfigurationExpander.java to see how internally Maven is merging the configuration for you (separately on Plugin and PluginMgmt level and the merged configuration is written to the execution specific configuration)

[kwin]

I think the logic for merging plugins with pluginMgmt is in https://github.com/apache/maven/blob/eb6b212b567c287734a2dbbef3c113fe650f1def/maven-model/src/main/java/org/apache/maven/model/merge/ModelMerger.java#L2445.

[bjhargrave]

Please check out the modified test at https://github.com/kwin/bnd/tree/inheritance-fails-when-mixing-pluginMgmt-and-plugins to see that Maven does not correctly merge pluginManagement configuration with plugins configuration for you.

@kwin, I did checkout your modified test and the bnd-maven-plugin code is working correctly. Your test change added configuration to the parent pom, so the child pom inherited the Bundle-SymbolicName value from the parent which is why the test failed.

I added some debug statements to verify. Here is the execution configuration from the parent project:

[INFO] [INFO] >>> execution config for MavenProject: biz.aQute.bnd-test:test-in-build-pluginManagement-inheritance:0.0.1 @ /Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/pom.xml
[INFO] <?xml version="1.0" encoding="UTF-8"?>
[INFO] <configuration>
[INFO]   <someconfig>somevalue</someconfig>
[INFO]   <skip>false</skip>
[INFO]   <bnd>X-ParentProjectProperty: it failed
[INFO] X-ParentProjectProperty2: it worked
[INFO] Bundle-SymbolicName: biz.aQute.bnd-test.test-in-build-pluginManagement-inheritance</bnd>
[INFO] </configuration>

You will see that maven has merged the pluginManagement configuration info in with your somevalue. So this demonstrates that maven is merging the pluginManagement configuration info into the execution configuration info when both are declared in the same project.

See also the Bundle-SymbolicName value which is inherited by the child pom.

Here is the execution configuration from the child project:

[INFO] [INFO] >>> execution config for MavenProject: biz.aQute.bnd-test:test-inheriting-api-bundle:0.0.1 @ /Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/test-inheriting-api-bundle/pom.xml
[INFO] <?xml version="1.0" encoding="UTF-8"?>
[INFO] <configuration>
[INFO]   <someconfig>somevalue</someconfig>
[INFO]   <skip>false</skip>
[INFO]   <bnd>X-ParentProjectProperty: it failed
[INFO] X-ParentProjectProperty2: it worked
[INFO] Bundle-SymbolicName: biz.aQute.bnd-test.test-inheriting-api-bundle</bnd>
[INFO] </configuration>

But this <bnd> information is not used by the child project since it has a bnd.bnd file. So the child project inherits the Bundle-SymbolicName from the parent since it is not specified in the child project's bnd.bnd file. This is why the test fails.

[INFO] Running post-build script: /Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/verify.groovy
[INFO]           test/pom.xml ..................................... FAILED (4.0 s)
[INFO]   The post-build script did not succeed. 
assert in_build_pluginManagement_api_manifest.getValue('Bundle-SymbolicName') == 'biz.aQute.bnd-test.test-inheriting-api-bundle'
       |                                      |                               |
       |                                      |                               false
       |                                      biz.aQute.bnd-test.test-in-build-pluginManagement-inheritance
       [Bundle-SymbolicName:biz.aQute.bnd-test.test-in-build-pluginManagement-inheritance, Archiver-Version:Plexus Archiver, Built-By:hargrave, Bnd-LastModified:1517003832198, Bundle-ManifestVersion:2, Import-Package:org.example.types;version="[1.0,2)", Require-Capability:osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))", X-ParentProjectProperty2:it worked, Tool:Bnd-4.0.0.201801262111-SNAPSHOT, Manifest-Version:1.0, Export-Package:org.example.api;version="2.0.0";uses:="org.example.types",org.example.types;version="1.0.0", Bundle-Name:Test PluginManagement inheritance API Bundle, Bundle-Version:0.0.1, X-ParentProjectProperty:overridden, Created-By:1.8.0_162 (Oracle Corporation), Build-Jdk:1.8.0_162]

[kwin]

Ok, thanks for checking. Then I guess the inheritance works as expected and just needs to be documented properly.

[kwin]

Actually after looking again at master...kwin:inheritance-fails-when-mixing-pluginMgmt-and-plugins I still think something is broken with the inheritance here. In my modified test I did not touch the bnd instructions at all. I only added a non-leveraged configuration parameter to the parent's plugin config. That should not change the outcome of the generated bundle symbolic name. As I didn't modify the verified BSN I would expect that this would still be the one from before, namely "biz.aQute.bnd-test.test-inheriting-api-bundle", as this would be the outcome with default Maven inheritance means! The properties should always be evaluated in the context of the current project's pom.xml!

[bjhargrave]

The inheritance is working as expected. Your change added a configuration to the parent project:

[INFO] [DEBUG] execution configuration for MavenProject: biz.aQute.bnd-test:test-in-build-pluginManagement-inheritance:0.0.1 @ /Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/pom.xml
[INFO] <?xml version="1.0" encoding="UTF-8"?>
[INFO] <configuration>
[INFO]   <someconfig>somevalue</someconfig>
[INFO]   <skip>false</skip>
[INFO]   <bnd>X-ParentProjectProperty: it failed
[INFO] X-ParentProjectProperty2: it worked
[INFO] Bundle-SymbolicName: biz.aQute.bnd-test.test-in-build-pluginManagement-inheritance</bnd>
[INFO] </configuration>
[INFO] [DEBUG] loading bnd properties from bnd element in pom: MavenProject: biz.aQute.bnd-test:test-in-build-pluginManagement-inheritance:0.0.1 @ /Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/pom.xml
[INFO] [DEBUG] current builder properties: {Bundle-SymbolicName=biz.aQute.bnd-test.test-in-build-pluginManagement-inheritance, X-ParentProjectProperty2=it worked, X-ParentProjectProperty=it failed, project.dir=/Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/test-inheriting-api-bundle}

Since you added the configuration to the parent project, the value of the Bundle-Symbolic captures the project's artifactId which is not what the test expects. Even though the child project has a pom configuration with a Bundle-SymbolicName value expected by the test:

[INFO] [DEBUG] execution configuration for MavenProject: biz.aQute.bnd-test:test-inheriting-api-bundle:0.0.1 @ /Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/test-inheriting-api-bundle/pom.xml
[INFO] <?xml version="1.0" encoding="UTF-8"?>
[INFO] <configuration>
[INFO]   <someconfig>somevalue</someconfig>
[INFO]   <skip>false</skip>
[INFO]   <bnd>X-ParentProjectProperty: it failed
[INFO] X-ParentProjectProperty2: it worked
[INFO] Bundle-SymbolicName: biz.aQute.bnd-test.test-inheriting-api-bundle</bnd>
[INFO] </configuration>
[INFO] [DEBUG] loading bnd properties from file: /Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/test-inheriting-api-bundle/bnd.bnd
[INFO] [DEBUG] current builder properties: {Bundle-SymbolicName=biz.aQute.bnd-test.test-in-build-pluginManagement-inheritance, X-ParentProjectProperty2=it worked, -exportcontents=${packages;VERSIONED}, X-ParentProjectProperty=overridden, project.dir=/Users/hargrave/git/bnd/maven/bnd-maven-plugin/target/integration-test/projects/test/test-in-build-pluginManagement-inheritance/test-inheriting-api-bundle}

Since the child project has a bnd.bnd file, that is used rather than the child project's pom configuration. (You can see <someconfig> in the output which I added to show the inheritance of pom configuration was working as expected.)

When the parent project does not have a configuration, the child project's configuration evaluates the pluginManagement contributed configuration in the context of the child project as the parent project's contribution to the configuration and thus has the artifactId expected by the test. The test specifically checks for this behavior. That is, the pluginManagement contribution is done in the context of the project which first defines an execution configuration rather than in the context of the project which defines the pluginManagement contribution.