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

Fix #185 Removing test-scope dependencies before building dependency tree #307

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

suztomo
Copy link
Contributor

@suztomo suztomo commented Aug 11, 2022

Problem

Fixes #185 , in which test-scope dependencies are unexpectedly excluded when

  • a library itself only uses the dependency as test
  • but another dependency actually uses the dependency (non-test)

(The issue has a diagram to illustrate the mechanism)

The root cause is that, when building the dependency tree of the project, test-scope dependencies hides the actually-used dependencies with "omitted for duplicate" mark.

Solution

With this pull request, when building a dependency tree, the plugin removes the direct, test-scope dependencies. This avoids marking the actually-used dependencies as "omitted for duplicate".

This modification is safe because the test-scope dependencies are not visible to library users in any way.

@suztomo
Copy link
Contributor Author

suztomo commented Aug 12, 2022

@slawekjaranowski Would you help me by reviewing this change?

You might not have the context of #185 (2 years ago) and it's a bit complex problem. Unfortunately this issue came up in my organization recently; we released a library that excluded actually-used dependencies (googleapis/java-pubsub#1239). I want to contribute to fix it.

@slawekjaranowski
Copy link
Member

I have merged #305 please resolve conflicts ... and check if new version of m-dependency-tree has the same issue.

@suztomo
Copy link
Contributor Author

suztomo commented Aug 14, 2022

Yes, I will!

@suztomo
Copy link
Contributor Author

suztomo commented Aug 15, 2022

In 2ced44c, I merged master branch and commented out my modification. I confirmed that the new version of maven-dependency-tree has the same problem: the test case for #185 it/flatten-dependency-all-both-test-and-transitive fails:

Running post-build script: /Users/suztomo/flatten-maven-plugin/target/it/flatten-dependency-all-both-test-and-transitive/verify.groovy
Assertion failed: 

assert 2 ==  flattenedProject.dependencies.dependency.size()
         |   |                |            |          |
         |   |                |            |          1
         |   |                |            org.codehaus.mojo.flatten.itscore3.2.1compileorg.codehaus.mojo.flatten.itsdepfalse
         |   |                org.codehaus.mojo.flatten.itscore3.2.1compileorg.codehaus.mojo.flatten.itsdepfalse
         |   4.0.0org.codehaus.mojo.flatten.itsflatten-dependency-all-both-test-and-transitive0.0.1-SNAPSHOTorg.codehaus.mojo.flatten.itscore3.2.1compileorg.codehaus.mojo.flatten.itsdepfalse
         false

	at org.codehaus.groovy.runtime.InvokerHelper.assertFailed(InvokerHelper.java:436)
	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.assertFailed(ScriptBytecodeAdapter.java:670)
	at Script1.run(Script1.groovy:35)

It fails because the flattened project does not have a actually-used dependency that was interfered by the direct test dependency.

Now, uncommenting my modification...

@suztomo
Copy link
Contributor Author

suztomo commented Aug 15, 2022

@slawekjaranowski All tests passed. Would you review this pull request?

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok, finally please squash to one commit

@suztomo
Copy link
Contributor Author

suztomo commented Aug 15, 2022

I just squashed the commits into one and rebased origin/master. Waiting for the test results.

@slawekjaranowski slawekjaranowski merged commit e6ba68b into mojohaus:master Aug 15, 2022
@suztomo
Copy link
Contributor Author

suztomo commented Aug 15, 2022

Thank you!

@suztomo suztomo deleted the i185_2 branch August 15, 2022 13:53
@slawekjaranowski slawekjaranowski changed the title Removing test-scope dependencies before building dependency tree Fix: #185 Removing test-scope dependencies before building dependency tree Aug 16, 2022
@slawekjaranowski slawekjaranowski changed the title Fix: #185 Removing test-scope dependencies before building dependency tree Fix #185 Removing test-scope dependencies before building dependency tree Aug 16, 2022
@meltsufin
Copy link

@slawekjaranowski When can we expect a new release with this change included?

@suztomo
Copy link
Contributor Author

suztomo commented Oct 21, 2022

It's been done as https://github.com/mojohaus/flatten-maven-plugin/releases/tag/flatten-maven-plugin-1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not to exclude a test dependency if the dependency is used in another dependency
3 participants