[MDEP-808] restrict dependency analysis by group #218

chadwick00 · 2022-05-20T15:43:10Z

Full write up of the use case on MDEP-808. Short version is that I've added a new {{includeDependencies}} parameter to allow the scope of the warnings from the analyze goal to be limited to the include dependencies.

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

slawekjaranowski

Pleas add assertions for IT test

slawekjaranowski · 2022-08-27T10:19:05Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

@@ -215,7 +216,7 @@
     * @since 2.10
     */
    @Parameter
-    private String[] ignoredUsedUndeclaredDependencies = new String[0];
+    private final String[] ignoredUsedUndeclaredDependencies = new String[0];


Please don't use final for Mojo parameters. It can be inline by compiler ...

Although looking back through the code again, I probably did this to remind myself that these aren't mutated by the code. Presumably only a mad-man would write code that would change the contents of a Mojo parameter, even if the compiler allowed them to?

elharo

What happens if the same artifact matches both includes and excludes? This needs to be clearly documented.

Also, this PR should include the necessary changes to the project docs, not just the code.

elharo · 2023-09-20T11:36:41Z

src/it/projects/analyze-include-dependency/pom.xml

+  <dependencies>
+    <dependency>
+      <groupId>org.apache.maven</groupId>
+      <artifactId>maven-project</artifactId>


why so old?

I think it was just that I wanted to valid project and didn't particularly care about the versions within the project. I can search for a substitution variable to keep in current if having this always lag behind is an eye-sore come review time.

I've tried to update the version of the all the dependencies @mavenVersion@, but at least one of the dependencies doesn't like it. I can try harder by choosing alternative dependencies, but I suppose the benefit of keeping these at an old version is that the referenced classes within dependency won't deprecated, as has been done elsewhere.

elharo · 2023-09-20T11:38:02Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

+     * where each pattern segment is optional and supports full and partial <code>*</code> wildcards. An empty pattern
+     * segment is treated as an implicit wildcard. *
+     * <p>
+     * For example, <code>org.apache.*</code> will match all artifacts whose group id starts with


will match --> matches (tech writing lives in the eternal present)

elharo · 2023-09-20T11:38:44Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

+     * segment is treated as an implicit wildcard. *
+     * <p>
+     * For example, <code>org.apache.*</code> will match all artifacts whose group id starts with
+     * <code>org.apache.</code>, and <code>:maven-dependency-plugin</code> will result in the analysis being applied


This is confusing. Try using an example of an unrelated library instead like JodaTime.

Done by copying the example Javadoc above. Decided the purpose of this section of the Javadoc is to explain the Maven co-ordinate and wildcard combination rather than the specific behaviour of the parameter.

elharo · 2023-09-20T11:39:26Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

+
+
+    /**
+     * List of dependencies to be included. The result of this list is then applied to the ignore parameters.


This isn't clear that everything is included by default when this isn't set.

Yes, I've been musing on this point. The default behaviour prior to this change is that everything is included. That can't change without requiring a pom changes downstream. So this is strictly:

"an override to the ignore"

That's a bit mind-bending if you don't have the context. I think the best we can do is to provide a recipe in the Javadoc describing an example use case, such as the one we're going to use it for. Broadly, everything ignored, but this one Maven co-ordinate in.

It doesn't feel like if we were designing this anew we'd start from here, but without a brain-wave on how to prevent a backward incompatible change, this will be the approach.

Thinking through some of the use-cases further and wanting more fine-grained control, we're essentially building sub-graphs in the dependency graph. I think that is expressible using Maven co-ordinates, but would be good to back these thoughts with some tests.

Decided that building sub-graphs was way too complicated, so sticking with the original approach and updating the Javadoc as requested, which is consistent with the standard Maven filters.... include then exclude.

elharo · 2023-09-20T11:40:26Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

@@ -438,6 +478,40 @@ private boolean checkDependencies()
            warning = true;
        }

+        // log dependencies that weren't included
+        if ( verbose && !notIncludedUsedDeclared.isEmpty() )


why are these logged? I thought the point of this PR was to avoid reporting on these.

These are logged in the verbose section only. Mirroring the existing verbose section for the explicitly ignored dependencies, I think you'd also want to understand what was ignored implicitly by not being covered within the includes.

elharo · 2023-09-20T11:40:59Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

+    /**
+     * Filter for artifacts that don't match the <code>exclude</code> criteria.
+     *
+     * @param artifacts filtered by elements that don't match the <code>excludes</code> argument


exclude or excludes? You have both.

elharo · 2023-09-20T11:41:54Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

+    {
+        List<Artifact> result = new ArrayList<>();
+
+        if ( includes.length > 0 )


Not a Collection. Happy to call Arrays.asList (includes) earlier if you prefer.

elharo · 2023-09-20T11:43:24Z

src/main/java/org/apache/maven/plugins/dependency/analyze/AbstractAnalyzeMojo.java

+                Artifact artifact = it.next();
+                if ( !filter.include( artifact ) )
+                {
+                    it.remove();


why the remove call? Doesn't seem needed.

The original filterDependencies method both mutates the artifacts argument and indicates what is has removed in the result. I didn't want to change the stateful nature of this methods because that would be a pretty serious overhaul to the code. So the remove call is needed to remove artifacts that don't match the include argument.

…sis-by-group-id

…id' of https://github.com/chadwick00/maven-dependency-plugin into feature/MDEP-808-Restrict-dependency-analysis-by-group-id

…on of parameters

elharo

I'm still skeptical of this feature. What is the problem solved by not warning on all unused dependencies? Unused dependencies don't break the build. If someone can't fix them all right now, so what?

elharo · 2023-10-04T11:36:21Z

src/site/apt/examples/control-dependencies-under-dependency-analysis.apt.vm

+
+  Another case is when mature projects have accumulated superfluous dependencies or
+  relied upon transitivity for their dependencies. Where fixing all the dependency 
+  problems up-front is not viable the plugin can be configured to only raise


comma after viable

elharo · 2023-10-04T11:37:14Z

src/site/apt/examples/control-dependencies-under-dependency-analysis.apt.vm

+
+              <!-- limit warnings to dependencies for group com.google.code.findbugs -->
+              <includeDependencies>
+                <includeDependency>com.google.code.findbugs</includeDependency>


includeDependency --> groupId

Instead of a pattern here,= separate groupId and artifactId elements are simpler.

Maybe includeDependencies --> analyze or check. I'm not sure.

elharo · 2023-10-04T11:38:28Z

src/site/apt/examples/control-dependencies-under-dependency-analysis.apt.vm

+  Another case is when mature projects have accumulated superfluous dependencies or
+  relied upon transitivity for their dependencies. Where fixing all the dependency 
+  problems up-front is not viable the plugin can be configured to only raise
+  warnings for dependencies meeting "include" criteria.


This needs to be expanded. Based solely on this text — that is, without reading the code or the JIRA — I do not know which dependencies are and are not checked.

chadwick00 added 2 commits May 20, 2022 16:32

[MDEP-808] - List dependencies for inclusion

f757e35

[MDEP-808] - List dependencies for inclusion integration test

fe669e0

slawekjaranowski requested changes Aug 27, 2022

View reviewed changes

elharo changed the title ~~Feature/mdep 808 restrict dependency analysis by group~~ [MDEP-808] restrict dependency analysis by group Nov 27, 2022

elharo self-requested a review September 20, 2023 11:36

elharo requested changes Sep 20, 2023

View reviewed changes

chadwick00 and others added 2 commits October 2, 2023 23:52

[MDEP-808] - List dependencies for inclusion - review changes

95f55d7

Merge branch 'master' into feature/MDEP-808-Restrict-dependency-analy…

2a56e85

…sis-by-group-id

chadwick00 requested review from elharo and slawekjaranowski October 3, 2023 09:11

chadwick00 added 3 commits October 3, 2023 22:21

[MDEP-808] - List dependencies for inclusion - project documentation

3f80197

Merge branch 'feature/MDEP-808-Restrict-dependency-analysis-by-group-…

ccb94f6

…id' of https://github.com/chadwick00/maven-dependency-plugin into feature/MDEP-808-Restrict-dependency-analysis-by-group-id

[MDEP-808] - List dependencies for inclusion - revert auto finalizati…

94844fd

…on of parameters

elharo reviewed Oct 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDEP-808] restrict dependency analysis by group #218

[MDEP-808] restrict dependency analysis by group #218

chadwick00 commented May 20, 2022

slawekjaranowski left a comment

slawekjaranowski Aug 27, 2022

chadwick00 Sep 20, 2023

chadwick00 Sep 20, 2023 •

edited

elharo left a comment

elharo Sep 20, 2023

chadwick00 Sep 20, 2023

chadwick00 Oct 2, 2023

elharo Sep 20, 2023

elharo Sep 20, 2023

chadwick00 Oct 2, 2023

elharo Sep 20, 2023

chadwick00 Sep 20, 2023

chadwick00 Sep 20, 2023

chadwick00 Oct 2, 2023 •

edited

elharo Sep 20, 2023

chadwick00 Oct 2, 2023 •

edited

elharo Sep 20, 2023

elharo Sep 20, 2023

chadwick00 Oct 2, 2023 •

edited

elharo Sep 20, 2023

chadwick00 Oct 2, 2023

elharo left a comment

elharo Oct 4, 2023

elharo Oct 4, 2023 •

edited

elharo Oct 4, 2023



		/**
		* List of dependencies to be included. The result of this list is then applied to the ignore parameters.

[MDEP-808] restrict dependency analysis by group #218

Are you sure you want to change the base?

[MDEP-808] restrict dependency analysis by group #218

Conversation

chadwick00 commented May 20, 2022

slawekjaranowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwick00 Sep 20, 2023 • edited

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwick00 Oct 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwick00 Oct 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwick00 Oct 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo Oct 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwick00 Sep 20, 2023 •

edited

chadwick00 Oct 2, 2023 •

edited

chadwick00 Oct 2, 2023 •

edited

chadwick00 Oct 2, 2023 •

edited

elharo Oct 4, 2023 •

edited