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

[MPH-183] Effective POM path to source #37

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jjkester
Copy link

@jjkester jjkester commented Nov 1, 2021

The goal of MPH-183 is to print via which BOM(s) a dependency got in the effective POM.

Related MNG-7344 PR that introduces the necessary changes in the model: apache/maven#603

Method for retrieving the hierarchical data from the InputSource is loaded with reflection. This way there is no behavior change for Maven versions below 4.0.0-alpha-1 with the new version of the plugin.

  • Handle integration test gracefully when running on a version of Maven that does not support the introduced functionality

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 [MPH-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MPH-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.

@jjkester jjkester changed the title Mph 183 effective pom path to source [MPH-183] Effective POM path to source Nov 1, 2021
@jjkester jjkester force-pushed the MPH-183_effective-pom-path-to-source branch from 2333232 to 4391032 Compare November 1, 2021 20:07
@jjkester jjkester force-pushed the MPH-183_effective-pom-path-to-source branch from 4391032 to 307fd9a Compare November 2, 2021 17:17
Copy link

@mthmulders mthmulders left a comment

Choose a reason for hiding this comment

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

Looks good to me. Happy to see the unhappy paths (e.g. running older Maven versions) are handled gracefully.

Let's wait for a build of Maven Core that adds this feature (apache/maven#603) so we can test-drive this feature once more before merging.

@mthmulders mthmulders force-pushed the MPH-183_effective-pom-path-to-source branch from 307fd9a to e467844 Compare July 5, 2022 13:51
@juulhobert juulhobert force-pushed the MPH-183_effective-pom-path-to-source branch 7 times, most recently from 5771163 to 3073624 Compare March 15, 2024 15:22
import org.codehaus.plexus.util.StringUtils;

/**
* Maven 3.x-based implementation of {@link InputLocation.StringFormatter}.
Copy link
Member

Choose a reason for hiding this comment

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

it's not Maven 3, it"s "without import tracking", as I hope we'll have import tracking for Maven 3 too

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we should probably change the name of the class and edit the JDocs. We would like to backport it to Maven 3, but initially want to make it work for Maven 4 first without breaking other versions.

try {
InputLocation result = (InputLocation) getImportedFromMethod.invoke(location);

if (result == null && project != null) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't get why we do all this: if the current location is not the result of an import, why are we trying to do something?

Copy link
Contributor

Choose a reason for hiding this comment

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

When there is an importedFrom on the current dependency we can simply return the result, if there is not an importedFrom on the current dependency we try to find a match in the dependencies of dependencyManagement

InputLocation result = (InputLocation) getImportedFromMethod.invoke(location);

if (result == null && project != null) {
for (Dependency dependency : project.getDependencyManagement().getDependencies()) {
Copy link
Member

Choose a reason for hiding this comment

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

MNG-7344 is not supposed to be limited to dependencyManagement import, but be able to work with imports done for example by Tiles Maven Plugin. Then iterating only on dependencyManagement is suspect

notice that this opens again the question of what is the purpose of trying to do something if there war no importFrom in location

Copy link
Contributor

Choose a reason for hiding this comment

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

I've no experience with the maven tiles plugin. If this results in the same model (which I assume it does) it probably would still work, we would have to test this though.

* Implementation of {@link InputLocation.StringFormatter}. Enhances the default implementation with support for
* following "references" (caused by e.g. dependency management imports).
*/
public class ImportedFromLocationFormatter extends InputLocation.StringFormatter {
Copy link
Member

Choose a reason for hiding this comment

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

if this class was only working on import tracking, and be injected in DefaultLocationFormatter if proper InputLocation API was found, this would IMHO clarify the purpose of this addition, that keeps using the initial location tracking

Copy link
Contributor

Choose a reason for hiding this comment

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

We thought it would make sense to separate the actual display logic from the logic on whether or not to use the correct formatter. We could change that but it would bundle more logic in to 1 bigger class?


while (importedFrom != null
&& !source.toString().equals(importedFrom.getSource().toString())) {
p.append(" from ").append(importedFrom.getSource().getModelId());
Copy link
Member

Choose a reason for hiding this comment

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

we also need the line, to know where the import has been done: there may be multiple ones in a source

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand your comment correctly, we print the line in the return statement (line 67).
If we have a project like:

bom1 -> dependency x (line 12)
  ^
  |
bom2 -> import bom1
  ^
  |
project -> import bom2

The output of the effective pom of this project's dependency shows bom1, line 12 from bom2

@jjkester jjkester marked this pull request as ready for review June 7, 2024 11:55
@jjkester
Copy link
Author

jjkester commented Jun 7, 2024

Marked ready for review on behalf of @juulhobert.

@Giovds Giovds force-pushed the MPH-183_effective-pom-path-to-source branch from 3f989f6 to 26ba8ea Compare September 20, 2024 07:08
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.

5 participants