-
-
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
Apply the -runbundles+ decorator on the computed RunBundles when resolving #6407
Apply the -runbundles+ decorator on the computed RunBundles when resolving #6407
Conversation
…to the runbundles resolver output as well. Signed-off-by: Arnoud Glimmerveen <arnoud@glimmerveen.org>
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.
Thanks for the fix.
As said in a comment, I'd really appreciate if you move the tests to the RunResolutionTest to be closer to the original change.
@@ -208,7 +210,16 @@ public boolean updateBundles(BndEditModel model) { | |||
newer = older; | |||
} | |||
|
|||
model.setRunBundles(newer); | |||
// Apply the -runbundles decorator on the computed RunBundles |
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.
You have to change the check if the runbundles are identical before on line 197. The decoration might change the runbundles.
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 have moved the application of decoration above the older/newer check.
@@ -0,0 +1,7 @@ | |||
invoker.goals=--no-transfer-progress package |
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.
Hmm. I am not too happy this testing takes place in the maven sub project. The functional change is in bnd so it should be tested in the normal bnd testing. The biz.aQute.resolve.RunResolutionTest in biz.aQute.resolve contains examples.
I know it is more comfortable to stay in your comfort zone but bnd is a shared project. If you change a function in bnd the test must take place as close as possible to the change.
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.
That makes sense, I will look into adding test case(s) in RunResolutionTest. Do you want to retain these maven sub project test as well for this particular case, or just the tests within biz.aQute.resolve?
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 it is better to test it once in bnd. I consider ANY error that would be detected in a 'driver' (maven, gradle, eclipse, intellij) an error of the highest severity. We rarely have them fortunately. And in this case, the functionality is clearly in bnd, not the driver.
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.
Ok, I have removed the integration tests from the maven sub module.
* Moved decoration between older/newer check to ensure that check also incorporates the decoration. * Added JUnit test. Signed-off-by: Arnoud Glimmerveen <arnoud@glimmerveen.org>
I have pushed a commit that should address the feedback. The only question I have is whether the tests in de maven sub module should be retained. |
…tionally covered by RunResolutionTest Signed-off-by: Arnoud Glimmerveen <arnoud@glimmerveen.org>
thanks! |
This PR should address issue #6364 . I applied the same mechanism used within Project.getBundles in the RunResolution's updateBundles function, just before the model is updated with the new RunBundles.