Skip to content
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

acceptPomPackage does not have effect on the parent project when aggregate-add-third-party #271

Closed
nfalco79 opened this issue Feb 20, 2019 · 8 comments
Milestone

Comments

@nfalco79
Copy link
Contributor

nfalco79 commented Feb 20, 2019

Hi, we have a maven project (packaging pom) that aggregate only artifacts to be delivered to a client.
To do this usually we define dependencies on a project of with pom packaging and run the assembly plugin (or depedency-copy-dependency) to gathers all of them.
Now our department would realize a report to double check all licenses to give to a client.
The acceptPomPackage documents "To execute or not this mojo if project packaging is pom." but it is not executed if the project is that where we run the mvn command.

Looking the code the aggregate-add-third-party cycles on each project in the reactor but skip itself. This mean that only child module of kind pom are considered. From the documentation I expect it run a forked add-third-party also on the parent pom (the project from which I run mvn command).

@ppalaga
Copy link
Contributor

ppalaga commented Feb 20, 2019

Would running add-third-party in the assembly pom project be a valid workaround?

@nfalco79
Copy link
Contributor Author

nfalco79 commented Feb 20, 2019

yes with the above property set to true. My question is what I describe is wanted behaviour or it's a bug? Or at least the documentation is misleading for aggregate-add-third-party but valid for add-third-party goal.

@ppalaga
Copy link
Contributor

ppalaga commented Feb 20, 2019

wanted behaviour or it's a bug?

I have no opinion on that. You may try your luck to ask the original author(s).

@nfalco79
Copy link
Contributor Author

I have no opinion on that. You may try your luck to ask the original author(s).

I should ask to @tchemit but since seems you are the mantainer and the most (second) contributor for this plugin. What do you think? Is reasonable think to have the same behaviour for both goals?

@ppalaga ppalaga added this to the Backlog milestone Feb 20, 2019
@ppalaga
Copy link
Contributor

ppalaga commented Feb 22, 2019

Taken very strictly, the proposed fix changes the meaning of a param which accounts to breaking the backwards compatibility. At the same time, I admit the proposed behavior might be both useful and what the most users expect based on what the JavaDoc says.

I am ready to accept the fix under the following conditions:

  1. You @nfalco79 promise to be ready (anytime in the future) to add an additional param that would bring back the current behavior, if somebody comes complaining about the backwards compatibility breakage.

  2. @nfalco79 adds a piece of JavaDoc to acceptPomPackaging that will explain the difference since 1.18 vs. before 1.18. (Ideally @nfalco79 asks somebody to check the English text even before amending Fix #271 aggregate-add-third-party evaluate acceptPomPackage also for current project #281 so that I do not need to loose my time with language corrections.)

Is 1. and 2. acceptable for you, @nfalco79 ?

@nfalco79
Copy link
Contributor Author

nfalco79 commented Feb 22, 2019

I know about my English, but here seems there was not volunteers to describe parameters in a way that everyone can understand (javadoc was written in my free time at 3 a.m., not "sponsorized" by our society)

About backwards compatibility, actually the goal has not effect on root project that means

  • if you have a single pom project I expect no one use this goal simply because does no have effect
  • if you have a multi module project than the dependencies in the root project are however analyzed in child module. This mean that if you have for example jettison defined in root project you have to provide a missing file in all chidren modules (or just one in the parent). So also this case is covered.
  • the only case where new behavior fails is when in a multi module project all children modules are excluded (using profiles or whatever) then you have an unexpected failure on root project where there was not before

If you think so, we can already add a parameter (excludeRootProject) to keep the old behavior. Any suggestions on the name?

@ppalaga
Copy link
Contributor

ppalaga commented Feb 22, 2019

we can already add a parameter (excludeRootProject)

No, I vote for doing it only if somebody comes with a complaint in the future (which very much hope will not happen ;-)

@nfalco79
Copy link
Contributor Author

Ok.
I will provide a javadoc on acceptPomPackaging to describe the new behavior respect 1.17

@ppalaga ppalaga modified the milestones: Backlog, 1.18 Feb 23, 2019
ppalaga added a commit that referenced this issue Feb 23, 2019
Fix #271 aggregate-add-third-party evaluate acceptPomPackage also for…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants