[MINVOKER-318] - invoker:install must resolve deps in 'test' scope #169

hgschmie · 2023-01-22T04:07:08Z

When integration tests require dependencies in "test" scope, that overlap with dependencies in "runtime" scope (e.g. the normal jar in runtime scope and a "tests" jar in test scope), the install goal will not resolve and install the test dependency because it requests only runtime scope resolution.

This PR contains an integration tests to demonstrate the problem. The change is trivial: make the "install" goal resolve its dependencies in test, not in runtime scope.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MINVOKER-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MINVOKER-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

When integration tests require dependencies in "test" scope, that overlap with dependencies in "runtime" scope (e.g. the normal jar in runtime scope and a "tests" jar in test scope), the install goal will not resolve and install the test dependency because it requests only runtime scope resolution. This PR contains an integration tests to demonstrate the problem. The change is trivial: make the "install" goal resolve its dependencies in test, not in runtime scope.

src/it/fail-test-scope/src/it/settings.xml

src/it/fail-test-scope/verify.bsh

slawekjaranowski · 2023-01-22T09:42:03Z

src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java

-        requiresDependencyResolution = ResolutionScope.RUNTIME,
+        requiresDependencyResolution = ResolutionScope.TEST,


Project can have many items in test scope, some can be a big,

With it we will always copy items which will be never used in the more of case.

the problem is that the current setup makes it impossible to run actual unit tests that have test specific dependencies which overlap with runtime dependencies.

Here is what I would suggest you should do: run the following commands:

git clone git@github.com:apache/maven-invoker-plugin gh pr checkout 169 mvn clean install -Prun-its -Dinvoker.test=fail-test-scope

this will show you the integration test pass. Now change the resolution scope in src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java back to runtime. then run mvn clean install -Prun-its -Dinvoker.test=fail-test-scope again

now you will see the integration test fail. It fails because the install plugin fails to install the commons-lang3-3.12.0-tests.jar dependency in the local repository. Even though the dependency is required by the integration test pom.

what does not work

force the dependency with extraArtifacts. Because the dependency is not present in the resolution scope, the plugin can not install it in the local repository.

move the dependency from "test" to "runtime" scope

This is a bug in the invoker:install goal. Here is a real world example:

git clone git@github.com:jdbi/jdbi3-guava-cache cd jdbi3-guava-cache ./mvnw -Dbasepom.it.skip=false clean install invoker:install invoker:integration-test invoker:verify

This fails. Install the patched maven invoker plugin (with the fix applied), then run

./mvnw -Ddep.plugin.invoker.version=3.4.1-SNAPSHOT -Dbasepom.it.skip=false clean install invoker:install invoker:integration-test invoker:verify

The integration test now succeeds. This project is directly affected by the bug and its fix.

it looks like you try to execute unit test by invoker,
but invoker is designed for executing integration test for Maven plugins not for unit tests ...

I don't think that it is a bug here.

During we testing plugins we don't need artifacts from test scope.

if you feel that this plugin should only be used to run maven plugin integration tests, then it should probably be marked as an internal plugin that should not be used for any other use cases. Otherwise, the headline of the project reads The Invoker Plugin is used to run a set of Maven projects. The plugin can determine whether each project execution is successful, and optionally can verify the output generated from a given project execution.. That is what I use it for (and others do as well). It executes maven projects (in my cases projects that run unit tests) and checks the outcome.

I gave you a bug report, an integration test and a fix. I am pretty sure that MINVOKER-314 and MINVOKER-317 are the same bug (In fact I know that 317 is the same bug).

314 and 317 are something else ...

My proposition for resolving this PR is providing scope for install goal as new parameter with default value to runtime
So when we need in some case another scope you can change configuration for your project.

I hope that will be ok for you.

slawekjaranowski · 2023-01-22T09:47:08Z

👎 from me, unless you convince me be more real example.

hgschmie · 2023-01-22T22:57:18Z

@slawekjaranowski have you actually tried the examples that I gave you / can you reproduce the problems?

slawekjaranowski · 2023-01-22T23:20:42Z

@slawekjaranowski have you actually tried the examples that I gave you / can you reproduce the problems?

@hgschmie - I have only looked at your example, I suspect that can fail as you wrote

So we can introduce new parameter to have possibility to change scope for instal goal

hgschmie · 2023-01-22T23:28:42Z

I don't think there is a point for me to discuss this any further until you tried the bug report examples that I gave you. I am glad that you accommodate the possibility that things "can fail as I wrote".

As an amendment to -317, if check out git@github.com:hgschmie/invoker-bug.gitand follow the instructions in the README and then run mvn -Ddep.plugin.invoker.version=3.4.1-SNAPSHOT -pl :test clean install, you will see it succeeds while mvn -Ddep.plugin.invoker.version=3.4.0 -pl :test clean install fails. So this bug fix also fixes the problem present in -317.

The dependency resolution scope is declared by a plugin and then given to the core. I am not sure how one can add a parameter to change this.

slawekjaranowski · 2023-01-23T07:35:29Z

Project git@github.com:jdbi/jdbi3-guava-cache not exists or is private

slawekjaranowski · 2023-01-23T08:41:22Z

This PR doesn't resolve MINVOKER-317

in hgschmie/invoker-bug.git you have mixed two cases

one with dependency with test scope
and second with dependencies from project which is not part of current reactor build

You use install goal, so artifacts from previous build are present in local repository - it can have impact on next build

I tryed:

rm -rf ~/.m2/repository/invoker-bug/

mvn -Ddep.plugin.invoker.version=3.4.1-SNAPSHOT -pl :test clean install

and we will have:

[INFO] Scanning for projects...
[INFO] 
[INFO] --------------------------< invoker-bug:test >--------------------------
[INFO] Building test 0.1-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[WARNING] The POM for invoker-bug:dep:jar:0.1-SNAPSHOT is missing, no dependency information available
[WARNING] The POM for invoker-bug:dep:jar:tests:0.1-SNAPSHOT is missing, no dependency information available
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  0.749 s
[INFO] Finished at: 2023-01-23T09:38:14+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal on project test: Could not resolve dependencies for project invoker-bug:test:jar:0.1-SNAPSHOT: The following artifacts could not be resolved: invoker-bug:dep:jar:0.1-SNAPSHOT, invoker-bug:dep:jar:tests:0.1-SNAPSHOT: Could not find artifact invoker-bug:dep:jar:0.1-SNAPSHOT -> [Help 1]

slawekjaranowski · 2023-01-23T09:51:47Z

We have discovered another bug here.

When we have dependency which is part of reactor build, all attached artifacts from reactor project are copied, should be copied only listed as dependency.

I think that this PR will change current behavior, which is correct in most of cases.

We should resolve root cause not only one special case.

hgschmie · 2023-01-25T05:52:45Z

git@github.com:jdbi/jdbi3-guava-cache

Yes, it is git@github.com:jdbi/jdbi-guava-cache Apologies.

hgschmie · 2023-01-25T05:58:05Z

We have discovered another bug here.

When we have dependency which is part of reactor build, all attached artifacts from reactor project are copied, should be copied only listed as dependency.

I think that this PR will change current behavior, which is correct in most of cases.

We should resolve root cause not only one special case.

Yes, you should. Except that the average bug on the MINVOKER JIRA is about three years open (there are bugs from 2014 open).

You are failing your users.

michael-o · 2023-01-25T08:21:32Z

We have discovered another bug here.
When we have dependency which is part of reactor build, all attached artifacts from reactor project are copied, should be copied only listed as dependency.
I think that this PR will change current behavior, which is correct in most of cases.
We should resolve root cause not only one special case.

Yes, you should. Except that the average bug on the MINVOKER JIRA is about three years open (there are bugs from 2014 open).

You are failing your users.

You as well since you are a committer too. Longer than me and Slawek.

slawekjaranowski · 2023-01-31T16:47:28Z

@hgschmie please look at #174

I added new parameter for selecting scope of installed artifacts

I close it as suppressed by #174

hgschmie force-pushed the invoker-install-scope branch from 3704630 to 51291a5 Compare January 22, 2023 04:18

slawekjaranowski requested changes Jan 22, 2023

View reviewed changes

src/it/fail-test-scope/src/it/settings.xml Show resolved Hide resolved

src/it/fail-test-scope/verify.bsh Show resolved Hide resolved

slawekjaranowski reviewed Jan 22, 2023

View reviewed changes

slawekjaranowski closed this Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MINVOKER-318] - invoker:install must resolve deps in 'test' scope #169

[MINVOKER-318] - invoker:install must resolve deps in 'test' scope #169

hgschmie commented Jan 22, 2023

slawekjaranowski Jan 22, 2023

hgschmie Jan 22, 2023

slawekjaranowski Jan 22, 2023

hgschmie Jan 22, 2023 •

edited

Loading

slawekjaranowski Jan 22, 2023

slawekjaranowski commented Jan 22, 2023

hgschmie commented Jan 22, 2023

slawekjaranowski commented Jan 22, 2023

hgschmie commented Jan 22, 2023

slawekjaranowski commented Jan 23, 2023

slawekjaranowski commented Jan 23, 2023

slawekjaranowski commented Jan 23, 2023

hgschmie commented Jan 25, 2023

hgschmie commented Jan 25, 2023

michael-o commented Jan 25, 2023 •

edited

Loading

slawekjaranowski commented Jan 31, 2023

		requiresDependencyResolution = ResolutionScope.RUNTIME,
		requiresDependencyResolution = ResolutionScope.TEST,

[MINVOKER-318] - invoker:install must resolve deps in 'test' scope #169

[MINVOKER-318] - invoker:install must resolve deps in 'test' scope #169

Conversation

hgschmie commented Jan 22, 2023

slawekjaranowski Jan 22, 2023

Choose a reason for hiding this comment

hgschmie Jan 22, 2023

Choose a reason for hiding this comment

slawekjaranowski Jan 22, 2023

Choose a reason for hiding this comment

hgschmie Jan 22, 2023 • edited Loading

Choose a reason for hiding this comment

slawekjaranowski Jan 22, 2023

Choose a reason for hiding this comment

slawekjaranowski commented Jan 22, 2023

hgschmie commented Jan 22, 2023

slawekjaranowski commented Jan 22, 2023

hgschmie commented Jan 22, 2023

slawekjaranowski commented Jan 23, 2023

slawekjaranowski commented Jan 23, 2023

slawekjaranowski commented Jan 23, 2023

hgschmie commented Jan 25, 2023

hgschmie commented Jan 25, 2023

michael-o commented Jan 25, 2023 • edited Loading

slawekjaranowski commented Jan 31, 2023

hgschmie Jan 22, 2023 •

edited

Loading

michael-o commented Jan 25, 2023 •

edited

Loading