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

Not to exclude a test dependency if the dependency is used in another dependency #185

Closed
suztomo opened this issue Nov 30, 2020 · 5 comments · Fixed by #307
Closed

Not to exclude a test dependency if the dependency is used in another dependency #185

suztomo opened this issue Nov 30, 2020 · 5 comments · Fixed by #307
Labels

Comments

@suztomo
Copy link
Contributor

suztomo commented Nov 30, 2020

Suppose we have the following 3 artifacts:

  • Artifact A has compile-scope (default) dependency to Artifact B.
  • Artifact A has test-scope dependency to Artifact C.
  • Artifact B has compile-scope (default) dependency to Artifact C.

Diagram:

tri-dependency

Artifact A's pom.xml is flattened before publishing to Maven Central.

Expected Result

Artifact A's flattened pom.xml does not exclude Artifact C when declaring ArtifactB's dependency. Artifact C may be used only in test in Artifact A, but it's needed by Artifact B.

Actual Result

With current implementation, Artifact A's flattened pom.xml excludes Artifact C from Artifact B's dependency. Artifact A's pom.xml would look like something like this:

  <groupId>g</groupId>
  <artifactId>Artifact A<artifactId>
...
  <dependency>
     <groupId>g</groupId>
     <artifactId>Artifact B</artifactId>
     <exclusions>
...
       <exclusion>
         <groupId>g</groupId>
         <artifactId>Artifact C</artifactId>
       </exclusion>

Version

flatten-maven-plugin version 1.2.5

Plugin Parameters

        <plugin>
          <groupId>org.codehaus.mojo</groupId>
          <artifactId>flatten-maven-plugin</artifactId>
          <version>1.2.5</version>
          <executions>
            <!-- enable flattening -->
            <execution>
              <id>flatten</id>
              <phase>process-resources</phase>
              <goals>
                <goal>flatten</goal>
              </goals>
            </execution>
            <!-- ensure proper cleanup -->
            <execution>
              <id>flatten.clean</id>
              <phase>clean</phase>
              <goals>
                <goal>clean</goal>
              </goals>
            </execution>
          </executions>
          <configuration>
            <flattenMode>oss</flattenMode>
            <flattenDependencyMode>all</flattenDependencyMode>
            <pomElements>
              <build>remove</build>
            </pomElements>
          </configuration>
        </plugin>

from com.google.cloud:google-cloud-shared-config:0.9.4

Real Example

In the example we encountered,

  • Artifact A: com.google.cloud:google-cloud-spanner-jdbc
  • Artifact B: com.google.cloud:google-cloud-spanner
  • Artifact C: com.google.api.grpc:grpc-google-cloud-spanner-admin-database-v1

The published pom.xml of com.google.cloud:google-cloud-spanner-jdbc (link) had the unexpected exclusion element:

   <dependency>
      <groupId>com.google.cloud</groupId>
      <artifactId>google-cloud-spanner</artifactId>
      <version>2.0.2</version>
      <scope>compile</scope>
      <exclusions>
...
        <exclusion>
          <artifactId>grpc-google-cloud-spanner-admin-database-v1</artifactId>
          <groupId>com.google.api.grpc</groupId>
        </exclusion>

(CC: @stephaniewang526 , thank you for helping me on the troubleshooting last week)

@stephaniewang526
Copy link
Contributor

I think this proposal makes sense.

@saturnism @olamy @rfscholte -- could you share your input too?

@saturnism
Copy link
Contributor

took a look in more detail and it does make sense. the exclusion is expected, but the dep should be added back in.

@hohwille hohwille added the bug label Jan 11, 2021
@hohwille
Copy link
Member

I fully agree that this is a bug. I have not yet analyzed this. It may be some kind of bug in the maven core classes that are reused (or partially even missused) here or if actually the flatten MOJO itself is the root problem. However, what we do in flatten-maven-plugin is use maven core classes to compute the effective POM and then we strip test dependencies. So what happens in your example project that you created to reproduce the error if you disable flatten-maven-plugin and do mvn help:effective-pom? Will the dependency C appear with test scope or compile scope? If still test you actually found a bug in maven core and this MOJO can not do anything about the problem.

@suztomo
Copy link
Contributor Author

suztomo commented Jan 15, 2021

@hohwille Thank you for comment. Here is the effective pom of the project (without flatten-maven-plugin). https://gist.github.com/suztomo/8bebb45c890a3bbcd4e845fbfc45d559

The dependency C (grpc-google-cloud-spanner-admin-database-v1) appears as test scope:

  <dependencyManagement>
    <dependencies>
...
      <dependency>
        <groupId>com.google.api.grpc</groupId>  <!-- com.google.cloud:google-cloud-spanner-bom:2.0.2, line 101 -->
        <artifactId>grpc-google-cloud-spanner-admin-database-v1</artifactId>  <!-- com.google.cloud:google-cloud-spanner-bom:2.0.2, line 102 -->
        <version>2.0.2</version>  <!-- com.google.cloud:google-cloud-spanner-bom:2.0.2, line 103 -->
      </dependency>
...
  <dependencies>
...
    <dependency>
      <groupId>com.google.cloud</groupId>  <!-- com.google.cloud:google-cloud-spanner-jdbc:1.17.3, line 105 -->
      <artifactId>google-cloud-spanner</artifactId>  <!-- com.google.cloud:google-cloud-spanner-jdbc:1.17.3, line 106 -->
      <version>2.0.2</version>  <!-- com.google.cloud:google-cloud-spanner-bom:2.0.2, line 87 -->
      <scope>compile</scope>
    </dependency>
...
    <dependency>
      <groupId>com.google.api.grpc</groupId>  <!-- com.google.cloud:google-cloud-spanner-jdbc:1.17.3, line 171 -->
      <artifactId>grpc-google-cloud-spanner-admin-database-v1</artifactId>  <!-- com.google.cloud:google-cloud-spanner-jdbc:1.17.3, line 172 -->
      <version>2.0.2</version>  <!-- com.google.cloud:google-cloud-spanner-bom:2.0.2, line 103 -->
      <scope>test</scope>  <!-- com.google.cloud:google-cloud-spanner-jdbc:1.17.3, line 173 -->
    </dependency>

If still test you actually found a bug in maven core

Would you elaborate more about this? The project's pom.xml declares the dependency (grpc-google-cloud-spanner-admin-database-v1) as "test" and I feel it's expected for Maven to show it as test scope in its effective pom.

@suztomo
Copy link
Contributor Author

suztomo commented Aug 9, 2022

I'm starting to work on this issue.

Our config

          <configuration>
            <flattenMode>oss</flattenMode>
            <flattenDependencyMode>all</flattenDependencyMode>

Step 1: Create a test that fails in master branch

src/it/mrm/repository/core-5.3.8.pom already has a dependency with test scope. But we don't use it.

Project A: I create pom.xml
B: core
C: dep

Step 2: Identify which code is responsible removing the test scope dependencies

image

"dependencies" property has "flatten" handling, which picks cleanPom among the argument.

createFlattenedDependency returns null for test scope dependency. This is reasonable unless we have the same artifact in transitive dependency.

We have dependencyNode variable. Can we use this? (Unfortunately the node is marked as duplicates)

image

        final DependencyNode dependencyNode = this.dependencyTreeBuilder.buildDependencyTree(this.project,
                this.localRepository, null);

However, DependencyNode with "omitted for duplicate" are not added to dependencyNodeLinkedList.

                if (node.getState() != DependencyNode.INCLUDED)
                {
                    return false;
                }

Step 3: Fix the problem

#307

Step 4: Format.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants
@suztomo @hohwille @saturnism @stephaniewang526 and others