Skip to content

Extract code to automatically derive data from maven-project - #6362

Merged
chrisrueger merged 1 commit into
bndtools:masterfrom
laeubi:extract_common_code_to_maven_artifact
Feb 4, 2025
Merged

Extract code to automatically derive data from maven-project#6362
chrisrueger merged 1 commit into
bndtools:masterfrom
laeubi:extract_common_code_to_maven_artifact

Conversation

@laeubi

@laeubi laeubi commented Nov 9, 2024

Copy link
Copy Markdown
Contributor

Currently if one loads the configuration with
aQute.bnd.maven.lib.configuration.BndConfiguration then only the
configuration itself is loaded, but actually some implicit configuration later apply (e.g. from the maven project configuration)
that is currently only performed by the AbstractBndMavenPlugin.

This extracts this logic from the AbstractBndMavenPlugin into the
BndConfiguration class for the following benefits:

  • making the AbstractBndMavenPlugin maintain less code in the execute
    method
  • allow other users of BndConfiguration to easily inherit common
    defaults
  • separation of concerns, easier to debug and understand

@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch 2 times, most recently from f330ad1 to f3cc8d4 Compare November 9, 2024 14:16
@laeubi

laeubi commented Nov 9, 2024

Copy link
Copy Markdown
Contributor Author

The build failure looks like my change in the shared lib is not picked up by the maven build.

@laeubi

laeubi commented Nov 9, 2024

Copy link
Copy Markdown
Contributor Author

@bjhargrave any hint / idea what could be wrong?

@bjhargrave

Copy link
Copy Markdown
Member

@bjhargrave any hint / idea what could be wrong?

I don't know.

But this PR seems to be poorly designed from an object oriented perspective. There are 2 things unrelated: reading the configuration from the pom/bnd file and setting some defaults, for bundle building, if they have not been set. I don't see why BndConfiguration should have an instance method for setting defaults since default settings never needed the configuration. That seems a poor place to put default settings for the bnd maven plugin and conflates two things.

@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch from f3cc8d4 to aa3c4e2 Compare November 10, 2024 08:51
@laeubi

laeubi commented Nov 10, 2024

Copy link
Copy Markdown
Contributor Author

There are 2 things unrelated: reading the configuration from the pom/bnd file and setting some defaults, for bundle building, if they have not been set

I think my naming was not really good here, I have now adjusted it to better reflect the meaning. It is actually not any defaults but apply some implicit configuration (and just converted to bnd instructions). Still of course it could also be extracted in a separate class, the main point is that it should not be mangled with the Mojo so it can be used in other context as well.

Sadly this would not resolve the issue that for some reason the invoker tests seem not to "see" the bnd-build jar file with the shared code but some outdated variant :-\

@pkriens

pkriens commented Nov 11, 2024

Copy link
Copy Markdown
Member

The problem seems to be that the bnd-baseline-maven-plugin has no dependency to biz.aQute.bnd.maven in its pom.xml. Therefore, the latest biz.aQute.bnd.maven it is not copied to the testing repo. With the wonderful maven logic, it will then likely get it from some random repository, which happens to be the previous version ...

Other bnd plugins might have the same problem although the bnd-maven-plugin seems ok.

However, I'd like to see @bjhargrave 's concerns addressed.

@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch from aa3c4e2 to 11e675e Compare November 11, 2024 15:22
Comment thread biz.aQute.bnd.maven/src/aQute/bnd/maven/lib/configuration/BndConfiguration.java Outdated
Comment thread maven-plugins/bnd-baseline-maven-plugin/pom.xml
@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch from 11e675e to f081c06 Compare November 15, 2024 13:16
@laeubi

laeubi commented Nov 15, 2024

Copy link
Copy Markdown
Contributor Author

Build seems running fine now! 👍

@laeubi

laeubi commented Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

I have extracted now the adjustments to the poms in a separate PR here:

once it is merged I will rebase/adjust this change so we only have the changes related to the topic itself.

@pkriens
pkriens marked this pull request as draft December 6, 2024 16:56
@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch 2 times, most recently from 7f9d03b to a5c618e Compare December 11, 2024 08:24
@laeubi
laeubi requested a review from bjhargrave December 11, 2024 08:37
@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch from a5c618e to e2e410b Compare December 11, 2024 08:52
@laeubi
laeubi marked this pull request as ready for review December 11, 2024 08:56
@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch from e2e410b to bd8c51b Compare December 11, 2024 09:09
@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch from bd8c51b to c9db60b Compare February 4, 2025 09:09
Currently if one loads the configuration with
aQute.bnd.maven.lib.configuration.BndConfiguration then only the
configuration itself is loaded, but actually some implicit configuration
later apply (e.g. from the maven project configuration)
that is currently only performed by the AbstractBndMavenPlugin.

This extracts this logic from the AbstractBndMavenPlugin into the
BndConfiguration class for the following benefits:

- making the AbstractBndMavenPlugin maintain less code in the execute
method
- allow other users of BndConfiguration to easily inherit common
defaults
- separation of concerns, easier to debug and understand

Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi
laeubi force-pushed the extract_common_code_to_maven_artifact branch from c9db60b to f439ce8 Compare February 4, 2025 09:56
@laeubi

laeubi commented Feb 4, 2025

Copy link
Copy Markdown
Contributor Author

Build is green now!

@laeubi
laeubi requested a review from bjhargrave February 4, 2025 11:50

@bjhargrave bjhargrave left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chrisrueger

Copy link
Copy Markdown
Contributor

Can I merge this?

@bjhargrave @pkriens

@bjhargrave

Copy link
Copy Markdown
Member

Can I merge this?

I have no objection.

@chrisrueger
chrisrueger merged commit d76cce2 into bndtools:master Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants