-
-
Notifications
You must be signed in to change notification settings - Fork 302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extract code to automatically derive data from maven-project #6362
base: master
Are you sure you want to change the base?
Extract code to automatically derive data from maven-project #6362
Conversation
f330ad1
to
f3cc8d4
Compare
The build failure looks like my change in the shared lib is not picked up by the maven build. |
@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. |
f3cc8d4
to
aa3c4e2
Compare
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 :-\ |
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. |
aa3c4e2
to
11e675e
Compare
biz.aQute.bnd.maven/src/aQute/bnd/maven/lib/configuration/BndConfiguration.java
Outdated
Show resolved
Hide resolved
11e675e
to
f081c06
Compare
Build seems running fine now! 👍 |
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. |
7f9d03b
to
a5c618e
Compare
a5c618e
to
e2e410b
Compare
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 <[email protected]>
e2e410b
to
bd8c51b
Compare
if (mavenProject == null) { | ||
return Optional.empty(); | ||
} | ||
return getConfiguration(mavenProject.getBuildPlugins()).or(() -> getConfiguration(mavenProject.getParent())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this here needs to also look in the parentage for properties. The failure stems from not finding the output time stamp in the parent pom of the project. So it is configuration(parentProject)+configuration(thisProject). Also need to allow the descendant projects to override the ancestor projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think you are right here, I was hoping this is already resolved by maven but it seems not be the case...
Currently if one loads the configuration with
aQute.bnd.maven.lib.configuration.BndConfiguration
then only theconfiguration 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 theBndConfiguration
class for the following benefits:AbstractBndMavenPlugin
maintain less code in the executemethod
BndConfiguration
to easily inherit commondefaults