-
-
Notifications
You must be signed in to change notification settings - Fork 305
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
this is the embedded dependency in maven plugins #1803
Conversation
Signed-off-by: Christian Schneider <chris@die-schneider.net>
@@ -124,11 +124,6 @@ | |||
<version>${project.version}</version> | |||
</dependency> | |||
<dependency> | |||
<groupId>biz.aQute.bnd</groupId> |
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.
How is this delete related to adding the embedded repo to the plugns?
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.
it's just cleanup from a previous change! Is that not ok?
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, for some reason I thought #1794 removed the reference, but it was just in the project pom. So this is fine.
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-dependency-plugin</artifactId> | ||
<version>2.10</version> | ||
<configuration> |
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.
It is not clear to me that this configuration should be in the parent pom. It is shared by 2 of the plugins but if we need to use the dependency plugin in another plugin, that plugin may not need the embedded repo. This config should be in each project's pom.
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..
<version>${project.version}</version> | ||
<type>jar</type> | ||
<overWrite>false</overWrite> | ||
<outputDirectory>${project.build.directory}/classes</outputDirectory> |
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.
<outputDirectory>${project.build.outputDirectory}</outputDirectory>
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.
Sorry, do you mean I should replace the entire string including /classes
?
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. Using classes means failure if user uses a diff output folder.
<version>${project.version}</version> | ||
<type>jar</type> | ||
<overWrite>false</overWrite> | ||
<outputDirectory>${project.build.directory}/classes</outputDirectory> |
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.
<outputDirectory>${project.build.outputDirectory}</outputDirectory>
…s not present The same issue exists with `bnd-testing-maven-plugin` Signed-off-by: Raymond Auge <raymond.auge@liferay.com>
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.
LGTM
No description provided.