fixes #160: when the direct dependency is optional, flattened its transitive dependencies and set them as optional #164

yangnuoyu · 2020-06-30T22:33:39Z

Code change in this pr:

add codes (line 1073 - 1076) to set the artifact of dependency node as optional when its parent is optional. Since we will visit the parent node first, it will finally set all the nodes in a subtree whose root node is optional as optional.
add two tests "flatten-dependency-all-optional" and "flatten-dependency-all-depmgnt-optional" to check the code change.
fix some style issue.
@stephaniewang526 @saturnism @elharo

… its transitive dependencies and set them as optional

stephaniewang526 · 2020-06-30T22:42:45Z

LGTM.

Out of curiosity, did you run some code formatter? Is that why the formatting changes came in?

yangnuoyu · 2020-06-30T22:48:50Z

LGTM.

Out of curiosity, did you run some code formatter? Is that why the formatting changes came in?

I just saw the format issue and change it by myself.

stephaniewang526 · 2020-06-30T23:00:34Z

@olamy PTAL fixes #160

elharo

I'm not clear on why this is desirable. is there an issue that explains this?

elharo · 2020-07-08T16:28:43Z

src/it/projects/flatten-dependency-all-depmgnt-optional/verify.groovy

+assert "core" ==  originalProject.dependencies.dependency.artifactId.text()
+assert "true" ==  originalProject.dependencies.dependency.optional.text()
+
+File flattendPom = new File( basedir, '.flattened-pom.xml' )


flattend --> flattenned

elharo · 2020-07-08T16:28:49Z

src/it/projects/flatten-dependency-all-depmgnt-optional/verify.groovy

+File flattendPom = new File( basedir, '.flattened-pom.xml' )
+assert flattendPom.exists()
+
+def flattendProject = new XmlSlurper().parse( flattendPom )


flattend --> flattenned

elharo · 2020-07-08T16:29:01Z

src/it/projects/flatten-dependency-all-optional/verify.groovy

+File flattendPom = new File( basedir, '.flattened-pom.xml' )
+assert flattendPom.exists()
+
+def flattendProject = new XmlSlurper().parse( flattendPom )


flattend --> flattenned

saturnism · 2020-07-08T17:03:23Z

i still feel that we should drop the entire subtree of an optional dependency, and let maven work it out itself.

elharo

I tend to agree with Ray. What is the reason for including dependencies of an optional dependency?

yangnuoyu · 2020-07-08T19:03:56Z

i still feel that we should drop the entire subtree of an optional dependency, and let maven work it out itself.

I tend to agree with Ray. What is the reason for including dependencies of an optional dependency?

The main reason is that the result of dependency:tree for dependency plugin 3.1.2 keeps the entire subtree of an direct optional dependency. The flatten-maven-plugin uses the same class in dependency plugin so I do not want to make a big change for it.

elharo

The goal should be to mirror what Maven does for the compile time and runtime classpaths, which drops this tree. mvn dependency:tree is an afterthought.

saturnism · 2020-07-08T22:55:13Z

another question is, what if we have a tree that looks like this?

A
+- B (optional)
|  +- C
+- C

If we set the C under B optional, what happened to the real C that we really wanted to use that's not optional?

This is a non-issue if we simply keep B as optional and drop the subtree.

So this becomes:

A
+- B (optional)
+- C

In another scenario, where if C is optional

A
+- B
|  +- C (optional)
|     +- D
+- C
   +- D

It would become:

A
+- B
+- C
+- D

yangnuoyu · 2020-07-09T06:47:00Z

another question is, what if we have a tree that looks like this?
A
+- B (optional)
|  +- C
+- C
If we set the C under B optional, what happened to the real C that we really wanted to use that's not optional?

This is a non-issue if we simply keep B as optional and drop the subtree.

So this becomes:
A
+- B (optional)
+- C
In another scenario, where if C is optional
A
+- B
|  +- C (optional)
|     +- D
+- C
   +- D
It would become:
A
+- B
+- C
+- D

I am thinking of removing the subtree but it still works in these two cases if we keep the subtree. In the first case, flatten-maven-plugin will choose the closer C and set the optional C as verbose, so the result will be the one you expected. In the second case, optional C is transitive so it will be dropped by flatten-maven-plugin before actually flatten the dependency tree.

saturnism · 2020-07-09T23:53:44Z

src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java

                    return true;
                }
-                if (node.getState() != DependencyNode.INCLUDED) return true;
+                if (node.getState() != DependencyNode.INCLUDED) {
+                    return true;


i know this is in the previous code, but should this actually return false, as we discussed in a separate call in the past?

I think that returning true will omit the current depNode but still visit its children. I will check whether it will make any difference if we return false.

quick ping ot see if we've checked this?

@saturnism all tests passed if it returns false.

saturnism · 2020-07-09T23:54:50Z

src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java

+                }
+                if (node.getParent().getArtifact().isOptional())
+                {
+                    node.getArtifact().setOptional(true);


my other concern is that we are mutating a node, but prob can't be helped.

I can try to use HashSet to record the artifact we want to set as "optional" so we do not need to change the node.

yangnuoyu · 2020-07-10T00:45:05Z

The goal should be to mirror what Maven does for the compile time and runtime classpaths, which drops this tree. mvn dependency:tree is an afterthought.

I run the command "mvn org.apache.maven.plugins:maven-dependency-plugin:3.1.2:list -f pom.xml -DincludeScope=runtime" to make sure I check the compile time and runtime dependencies. The result still includes the direct optional dependency and its transitive dependencies which is marked as "optional"

For example: there is a pom file with only the dependency "junit:junit:4.13" which is optional (and hamcrest-core is a no-optional compile transitive dependency of junit):

  <dependencies>
    <dependency>
      <groupId>junit</groupId>
      <artifactId>junit</artifactId>
      <version>4.13</version>
      <optional>true</optional>
    </dependency>
  </dependencies>

When I run the command "mvn org.apache.maven.plugins:maven-dependency-plugin:3.1.2:list -f pom.xml -DincludeScope=runtime", the result is as follows:

[INFO] --- maven-dependency-plugin:3.1.2:list (default-cli) @ test ---
[INFO] 
[INFO] The following files have been resolved:
[INFO]    junit:junit:jar:4.13:compile (optional)  -- module junit [auto]
[INFO]    org.hamcrest:hamcrest-core:jar:1.3:compile (optional)  -- module hamcrest.core (auto)

The result shows that Maven still see the hamcrest-core in the compile time and runtime.

yangnuoyu · 2020-07-15T15:27:35Z

@stephaniewang526 @saturnism @elharo @olamy Updated, PTAL

…nsitive dependencies and set them as optional (#164) * fixes #160: when the direct dependency is optional, flattened its transitive dependencies and set them as optional * remove optional dependency subtree * return false in visitor when node is not included

fixes mojohaus#160: when the direct dependency is optional, flattened…

4375656

… its transitive dependencies and set them as optional

yangnuoyu marked this pull request as ready for review June 30, 2020 22:54

elharo reviewed Jul 8, 2020

View reviewed changes

saturnism reviewed Jul 9, 2020

View reviewed changes

yangnuoyu added 2 commits July 13, 2020 09:52

remove optional dependency subtree

3d405b4

return false in visitor when node is not included

ec676df

olamy merged commit 45eb5df into mojohaus:master Jul 16, 2020

olamy mentioned this pull request May 3, 2022

use shared gh action asf #273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #160: when the direct dependency is optional, flattened its transitive dependencies and set them as optional #164

fixes #160: when the direct dependency is optional, flattened its transitive dependencies and set them as optional #164

yangnuoyu commented Jun 30, 2020 •

edited

Loading

stephaniewang526 commented Jun 30, 2020

yangnuoyu commented Jun 30, 2020

stephaniewang526 commented Jun 30, 2020 •

edited

Loading

elharo left a comment

elharo Jul 8, 2020

yangnuoyu Jul 13, 2020

elharo Jul 8, 2020

yangnuoyu Jul 13, 2020

elharo Jul 8, 2020

yangnuoyu Jul 13, 2020

saturnism commented Jul 8, 2020

elharo left a comment

yangnuoyu commented Jul 8, 2020

elharo left a comment

saturnism commented Jul 8, 2020 •

edited

Loading

yangnuoyu commented Jul 9, 2020

saturnism Jul 9, 2020

yangnuoyu Jul 10, 2020

saturnism Jul 14, 2020

yangnuoyu Jul 14, 2020

saturnism Jul 9, 2020 •

edited

Loading

yangnuoyu Jul 10, 2020

yangnuoyu commented Jul 10, 2020

yangnuoyu commented Jul 15, 2020

fixes #160: when the direct dependency is optional, flattened its transitive dependencies and set them as optional #164

fixes #160: when the direct dependency is optional, flattened its transitive dependencies and set them as optional #164

Conversation

yangnuoyu commented Jun 30, 2020 • edited Loading

stephaniewang526 commented Jun 30, 2020

yangnuoyu commented Jun 30, 2020

stephaniewang526 commented Jun 30, 2020 • edited Loading

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

saturnism commented Jul 8, 2020

elharo left a comment

Choose a reason for hiding this comment

yangnuoyu commented Jul 8, 2020

elharo left a comment

Choose a reason for hiding this comment

saturnism commented Jul 8, 2020 • edited Loading

yangnuoyu commented Jul 9, 2020

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

saturnism Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangnuoyu commented Jul 10, 2020

yangnuoyu commented Jul 15, 2020

yangnuoyu commented Jun 30, 2020 •

edited

Loading

stephaniewang526 commented Jun 30, 2020 •

edited

Loading

saturnism commented Jul 8, 2020 •

edited

Loading

saturnism Jul 9, 2020 •

edited

Loading