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

[discuss] Enables to resolve direct pom dependencies (not transitive ones) location as project.dependencies. reference - eases javaagent usage for ex #1281

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rmannibucau
Copy link
Contributor

Sample proposal to try to ease mockito/mockito#3037.

Main issue of this PR is to not support transitive deps but think it is not a bad compromise (using javaagent "scope" would be as bad + totally wrong/broken in terms of design - would loose the scope to be clear, and adding an attribute sounds overkill until we get something like gradle configuration).
Main advantage is it makes it easy to work for everybody and stays aligned on the JVM (explicit toggling with ordering) for javaagent cases.

So overall think it can be a good compromise. If validated we have to document it properly - probably in maven site and surefire one?

@bmarwell if you want to try/take this over, but thought a PR would be easier than slack ;)

@bmarwell
Copy link
Contributor

So the idea is that a given group and artifact ID gets resolved to the following property:

${project.dependencies.com.bar.dummy-agent}

Only disadvantage of your solution: I don't see where the groupId ends and the artifact ID begins. But that's probably not going to clash.

This is very helpful for agents.

Other ideas:

Change the default lifecycle XML to include the property goal of the dependency plugin. Way smaller change.

@bmarwell
Copy link
Contributor

I forgot this file doesn't exist anymore.

https://github.com/apache/maven/blob/maven-3.3.8/maven-core/src/main/resources/META-INF/plexus/default-bindings.xml

But I think you get what I'm referring to.

@rmannibucau
Copy link
Contributor Author

If it clashes we have other troubles and using another sep looks wrong in placeholders to me.
Im also very hesitant to add another plugin in any lifecycle cause:

  1. It is specific to default lifecycles
  2. Adds another prerequisite and i tend to think we already have too much
  3. It will be too late in the interpolation and need other passes

@bmarwell
Copy link
Contributor

Thank you for the explanation, that makes excellent documentation! 😃

@gnodet
Copy link
Contributor

gnodet commented Oct 18, 2023

This really looks like a temporary hack for agents to being supported by Surefire...
It only saves the following declaration:

 <plugin>
    <artifactId>maven-dependency-plugin</artifactId>
    <executions>
      <execution>
        <goals>
          <goal>properties</goal>
        </goals>
      </execution>
    </executions>
  </plugin>

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

I've now read the full mockito thread. I get a better understand. While I'd be ok with the goal provided by the PR (ease or configuration for agents ), I think the current PR is wrong as it is implemented when the model is built, before dependencies are resolved. That means the version will be empty in most cases (when using managed dependencies).
So I'm not sure the current PR is worth it.

@rmannibucau
Copy link
Contributor Author

Well any later phase is wrong cause then it depends if the interpolation will be reprocessed or not so think it is the least bad compromise to work immediately but agree on your point, the limitation is to explicit define the dependency in the project (potentially using a property but without anything implicit).
I'm fine dropping this PR if it does not help while we don't add dependency plugin in any default lifecycle.

@gnodet
Copy link
Contributor

gnodet commented Oct 18, 2023

Well any later phase is wrong cause then it depends if the interpolation will be reprocessed or not so think it is the least bad compromise to work immediately but agree on your point, the limitation is to explicit define the dependency in the project (potentially using a property but without anything implicit). I'm fine dropping this PR if it does not help while we don't add dependency plugin in any default lifecycle.

@rmannibucau Wouldn't it be better to enhance the PluginParameterExpressionEvaluatorV4 class instead ?

@rmannibucau
Copy link
Contributor Author

@gnodet I'm mixed about it since it assumes everything is wired in plugins conf and plugins/extensions don't read any other property so not sure it is really better today, maybe let's wait to see what mockito picks?

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

Successfully merging this pull request may close these issues.

3 participants