-
Notifications
You must be signed in to change notification settings - Fork 115
IVY-1653: makepom: write overrides to Maven dependencyManagement section
#114
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
base: master
Are you sure you want to change the base?
Conversation
|
I'm wondering what happens if you specify the organisation in the override, like in the example in the JIRA. |
|
This test case does just that:
|
|
Sure, but how does Maven handle the '*' artifactId? Does that work? |
|
I'd have to try that with maven. Is there a concern that lots of top-level overrides are in use? Could this get taken as-is and if we find a compatibility problem, work it as a new issue? |
|
If maven gives errors on these wildcards, perhaps we could only add it to the dependencymanagement if the Ivy-override is fully specified (read: both groupId and artifactId have a non-wildcard value) ? We should try to avoid generating pom.xml files that cannot be handled by Maven. |
|
|
I fixed the tests to write proper |
| ModuleRules<DependencyDescriptorMediator> mr = md.getAllDependencyDescriptorMediators(); | ||
| if (mr.getAllRules().isEmpty()) { | ||
| return; | ||
| } |
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.
Perhaps also return here if none of the rules are an instance of non-wildcard OverrideDependencyDescriptorMediator.
This way, we avoid an empty <dependencyManagement>/<dependencies>-construct if container was true.
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.
Sure -- I tried to keep it simple instead of building up a filtered collection.
| }; | ||
|
|
||
| assertEquals(String.join(System.lineSeparator(), expect), readFileToString(pomFile, "UTF-8")); | ||
| } |
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.
Could you also add a test case for updating a template pom.xml that already has a dependencyManagement section, and we have an ivy.xml with an override element. (just to verify that an existing dependencyManagent gets updated correctly).
| public class IvyMakePomTest { | ||
|
|
||
| private Project project; | ||
| private Project project = TestHelper.newProject(); |
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.
This isn't the same as creating a new project in a @before method.
Now, the same project is reused for every IvyMakePom task in the test methods.
Before, every task had a new project.
I'm not saying this is a problem per se, but why change it? Did it cause issues?
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.
In JUnit 4, a new instance of the test class is created for each test case. You are describing the JUnit 3 behavior.
| org.apache.ivy.plugins.pack;version="2.6.0", | ||
| org.apache.ivy.plugins.parser;version="2.0.0", | ||
| org.apache.ivy.plugins.parser.m2;version="2.0.0", | ||
| org.apache.ivy.plugins.parser.m2;version="2.6.0", |
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.
Just out of curiousity, is this Eclipse that's doing this for you?
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 am doing this. If you look at the 2.5.3 manifest, all the versions are stuck at 2.0.0 which is not really proper for OSGi.
| dependenciesPrinted = true; | ||
| } | ||
| if (line.contains("</dependencyManagement>")) { | ||
| dependencyManagement = false; |
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 you should also check here that any overrides are printed.
This to tackle the case where the template contains an empty <dependencyManagent> element (without <dependencies> subelement)
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 suppose that's possible. Was just handling likely scenarios first.
|
I've added some comments, could you have a look? |
|
I will get to your feedback on this one. Now that |
|
Could you review my PRs from oldest to newest? |
Inspect the
DependencyDescriptorMediatorobjects ofModuleDescriptorto represent overrides in Maven'sdependencyManagementelement.https://issues.apache.org/jira/browse/IVY-1653