You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "hgschmie (via GitHub)" <gi...@apache.org> on 2023/01/22 04:07:10 UTC
[GitHub] [maven-invoker-plugin] hgschmie opened a new pull request, #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
hgschmie opened a new pull request, #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169
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:
- [X] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MINVOKER) 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.
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [X] 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.
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [X] 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](https://www.apache.org/licenses/icla.pdf) 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](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083550618
##########
src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java:
##########
@@ -83,7 +83,7 @@
@Mojo(
name = "install",
defaultPhase = LifecyclePhase.PRE_INTEGRATION_TEST,
- requiresDependencyResolution = ResolutionScope.RUNTIME,
+ requiresDependencyResolution = ResolutionScope.TEST,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1399917729
Project `git@github.com:jdbi/jdbi3-guava-cache` not exists or is private
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] michael-o commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1403245683
> > 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 as well. Longer than me and Slawek.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski closed pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski closed pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
URL: https://github.com/apache/maven-invoker-plugin/pull/169
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083560158
##########
src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java:
##########
@@ -83,7 +83,7 @@
@Mojo(
name = "install",
defaultPhase = LifecyclePhase.PRE_INTEGRATION_TEST,
- requiresDependencyResolution = ResolutionScope.RUNTIME,
+ requiresDependencyResolution = ResolutionScope.TEST,
Review Comment:
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. 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). This is what it is used for. 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1399640367
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.git`and 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1400071461
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083419471
##########
src/it/fail-test-scope/src/it/settings.xml:
##########
@@ -0,0 +1,60 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<settings>
+ <profiles>
+ <profile>
+ <id>it-repo</id>
+ <activation>
+ <activeByDefault>true</activeByDefault>
+ </activation>
+ <!--
+ NOTE: We don't use @localRepository@ here which refers to the isolated local repository of the ITs and is quite
+ empty. In contrast, @invoker.repo.local@ has been defined to refer to the original local repository which is a
+ far better source for artifacts.
+ -->
Review Comment:
`invoker.repo.local` is used to set local repository for test process.
Remote repositories from settings point to local repository of parent outbound process and should be defined as fallback.
##########
src/it/fail-test-scope/verify.bsh:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.*;
+import java.util.*;
+import java.util.regex.*;
+
+try
+{
+ // see if both the commons-lang jar and the commons-lang-tests jar were installed
+
+ File jarFile = new File( basedir, "target/local-repo/org/apache/commons/commons-lang3/3.12.0/commons-lang3-3.12.0.jar" );
+ System.out.println( "Checking for jar file: " + jarFile );
+ if ( !jarFile.exists() )
Review Comment:
What is purpose of checking for existing library?
When we use `addTestClassPath` we will have everything from project test scope on classpath in script.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083548095
##########
src/it/fail-test-scope/src/it/settings.xml:
##########
@@ -0,0 +1,60 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<settings>
+ <profiles>
+ <profile>
+ <id>it-repo</id>
+ <activation>
+ <activeByDefault>true</activeByDefault>
+ </activation>
+ <!--
+ NOTE: We don't use @localRepository@ here which refers to the isolated local repository of the ITs and is quite
+ empty. In contrast, @invoker.repo.local@ has been defined to refer to the original local repository which is a
+ far better source for artifacts.
+ -->
Review Comment:
hi slawek. Thank you for taking time to look at the PR. This is the same setup as the the invocation-multiple, invocation-offline or invocation-project integration tests (this is where this file was copied from).
The way this works is that the integration test uses the invoke:install goal within the test to copy dependencies from the local repository into an integration test specific local repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1399632103
@slawekjaranowski have you actually tried the examples that I gave you / can you reproduce the problems?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083420364
##########
src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java:
##########
@@ -83,7 +83,7 @@
@Mojo(
name = "install",
defaultPhase = LifecyclePhase.PRE_INTEGRATION_TEST,
- requiresDependencyResolution = ResolutionScope.RUNTIME,
+ requiresDependencyResolution = ResolutionScope.TEST,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083562092
##########
src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java:
##########
@@ -83,7 +83,7 @@
@Mojo(
name = "install",
defaultPhase = LifecyclePhase.PRE_INTEGRATION_TEST,
- requiresDependencyResolution = ResolutionScope.RUNTIME,
+ requiresDependencyResolution = ResolutionScope.TEST,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1399638431
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083560158
##########
src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java:
##########
@@ -83,7 +83,7 @@
@Mojo(
name = "install",
defaultPhase = LifecyclePhase.PRE_INTEGRATION_TEST,
- requiresDependencyResolution = ResolutionScope.RUNTIME,
+ requiresDependencyResolution = ResolutionScope.TEST,
Review Comment:
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1399977392
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]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1410724565
@hgschmie please look at #174
I added new parameter for selecting scope of installed artifacts
I close it as suppressed by #174
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1403132500
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1403129672
> git@github.com:jdbi/jdbi3-guava-cache
Yes, it is git@github.com:jdbi/jdbi-guava-cache Apologies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#issuecomment-1399441826
👎 from me, unless you convince me be more real example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] hgschmie commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "hgschmie (via GitHub)" <gi...@apache.org>.
hgschmie commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083548291
##########
src/it/fail-test-scope/verify.bsh:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.*;
+import java.util.*;
+import java.util.regex.*;
+
+try
+{
+ // see if both the commons-lang jar and the commons-lang-tests jar were installed
+
+ File jarFile = new File( basedir, "target/local-repo/org/apache/commons/commons-lang3/3.12.0/commons-lang3-3.12.0.jar" );
+ System.out.println( "Checking for jar file: " + jarFile );
+ if ( !jarFile.exists() )
Review Comment:
I am not sure that I understand what you say or that you looked at what this tests. This is *not* testing the existence of a file in the `target/local-repo` folder of the project but in a test-specific `target/it/fail-test-scope/target/local-repo` which does not contain any files before executing the `invoker:install` goal in the integration test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-invoker-plugin] slawekjaranowski commented on a diff in pull request #169: [MINVOKER-318] - invoker:install must resolve deps in 'test' scope
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #169:
URL: https://github.com/apache/maven-invoker-plugin/pull/169#discussion_r1083557797
##########
src/main/java/org/apache/maven/plugins/invoker/InstallMojo.java:
##########
@@ -83,7 +83,7 @@
@Mojo(
name = "install",
defaultPhase = LifecyclePhase.PRE_INTEGRATION_TEST,
- requiresDependencyResolution = ResolutionScope.RUNTIME,
+ requiresDependencyResolution = ResolutionScope.TEST,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org