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.

Sign up for GitHub

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

Remove first level transitive provided scope dependencies and their subtrees #172

Merged
merged 6 commits into from
Aug 12, 2020
Merged

Remove first level transitive provided scope dependencies and their subtrees #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8e891bb
Remove first level transitive, provided scope dependencies and their …
Jul 13, 2020
4288379
Removed accidentally included 'optional' test.
Jul 13, 2020
7372d8b
Fixed weird spacing modifications from before
Jul 13, 2020
a278b40
Fixed weird spacing issues with PR
Jul 14, 2020
3d3a2c5
Merge branch 'master' into provided-transitive
salehsquared Jul 16, 2020
d6c1c00
Fixing spacing issue
Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/it/mrm/parent-deps-1.1.pom
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>parent-deps</artifactId>
<version>1.1</version>
<packaging>pom</packaging>

<!-- for testing purpose, normally this would be dep management -->
<dependencies>
<dependency>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>dep</artifactId>
<version>1.1</version>
<scope>provided</scope>
</dependency>
</dependencies>
</project>
37 changes: 37 additions & 0 deletions src/it/projects/flatten-dependency-all-depmgnt-provided-transitive/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>flatten-dependency-all-depmgnt-provided-transitive</artifactId>
<version>0.0.1-SNAPSHOT</version>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>dep</artifactId>
<version>1.1</version>
<scope>provided</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<dependency>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>parent-deps</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. Why is this called parent-deps when its not the parent of this pom? Consider renaming some things.

Copy link
Contributor Author

@salehsquared salehsquared Jul 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe it was called parent-deps because it was the parent dependency of 'dep'. Currently there's a test artifact parent-deps:1.0 already within the 'mrm' folder used for testing.
In the PR, we add another test artifact parent-deps:1.1 that holds the same structure as the original parent-deps:1.0, but uses 'dep' with scope 'provided' instead of compile, and following the conventions that were present with the original parent-deps:1.0.

<version>1</version>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>flatten-maven-plugin</artifactId>
<configuration>
<flattenDependencyMode>all</flattenDependencyMode>
</configuration>
</plugin>
</plugins>
</build>
</project>
32 changes: 32 additions & 0 deletions src/it/projects/flatten-dependency-all-depmgnt-provided-transitive/verify.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
File originalPom = new File( basedir, 'pom.xml' )
assert originalPom.exists()

def originalProject = new XmlSlurper().parse( originalPom )
assert 1 == originalProject.dependencies.size()

File flattendPom = new File( basedir, '.flattened-pom.xml' )
assert flattendPom.exists()

def flattendProject = new XmlSlurper().parse( flattendPom )
assert 1 == flattendProject.dependencies.size()
assert 1 == flattendProject.dependencies.dependency.size()


28 changes: 28 additions & 0 deletions src/it/projects/flatten-dependency-all-provided-children/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>flatten-dependency-all-depmgnt-provided-transitive</artifactId>
<version>0.0.1-SNAPSHOT</version>

<dependencies>
<dependency>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>parent-deps</artifactId>
<version>1</version>
<scope>provided</scope>
</dependency>
</dependencies>

<build>
<defaultGoal>verify</defaultGoal>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>flatten-maven-plugin</artifactId>
<configuration>
<flattenDependencyMode>all</flattenDependencyMode>
</configuration>
</plugin>
</plugins>
</build>
</project>
30 changes: 30 additions & 0 deletions src/it/projects/flatten-dependency-all-provided-children/verify.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
File originalPom = new File( basedir, 'pom.xml' )
assert originalPom.exists()

def originalProject = new XmlSlurper().parse( originalPom )
assert 1 == originalProject.dependencies.size()

File flattendPom = new File( basedir, '.flattened-pom.xml' )
assert flattendPom.exists()

def flattendProject = new XmlSlurper().parse( flattendPom )
assert 1 == flattendProject.dependencies.size()
assert 1 == flattendProject.dependencies.dependency.size()
28 changes: 28 additions & 0 deletions src/it/projects/flatten-dependency-all-provided-direct/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>flatten-dependency-all-provided-direct</artifactId>
<version>0.0.1-SNAPSHOT</version>

<dependencies>
<dependency>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>dep</artifactId>
<version>1.1</version>
<scope>provided</scope>
</dependency>
</dependencies>

<build>
<defaultGoal>verify</defaultGoal>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>flatten-maven-plugin</artifactId>
<configuration>
<flattenDependencyMode>all</flattenDependencyMode>
</configuration>
</plugin>
</plugins>
</build>
</project>
33 changes: 33 additions & 0 deletions src/it/projects/flatten-dependency-all-provided-direct/verify.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
File originalPom = new File( basedir, 'pom.xml' )
assert originalPom.exists()

def originalProject = new XmlSlurper().parse( originalPom )
assert 1 == originalProject.dependencies.size()

File flattendPom = new File( basedir, '.flattened-pom.xml' )
assert flattendPom.exists()

def flattenedProject = new XmlSlurper().parse( flattendPom )
assert 1 == flattenedProject.dependencies.size()
assert 1 == flattenedProject.dependencies.dependency.size()

String scope = flattenedProject.dependencies.dependency.scope
assert "provided" == scope
27 changes: 27 additions & 0 deletions src/it/projects/flatten-dependency-all-provided-transitive/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>flatten-dependency-all-provided-transitive</artifactId>
<version>0.0.1-SNAPSHOT</version>

<dependencies>
<dependency>
<groupId>org.codehaus.mojo.flatten.its</groupId>
<artifactId>parent-deps</artifactId>
<version>1.1</version>
</dependency>
</dependencies>

<build>
<defaultGoal>verify</defaultGoal>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>flatten-maven-plugin</artifactId>
<configuration>
<flattenDependencyMode>all</flattenDependencyMode>
</configuration>
</plugin>
</plugins>
</build>
</project>
30 changes: 30 additions & 0 deletions src/it/projects/flatten-dependency-all-provided-transitive/verify.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
File originalPom = new File( basedir, 'pom.xml' )
assert originalPom.exists()

def originalProject = new XmlSlurper().parse( originalPom )
assert 1 == originalProject.dependencies.size()

File flattendPom = new File( basedir, '.flattened-pom.xml' )
assert flattendPom.exists()

def flattenedProject = new XmlSlurper().parse( flattendPom )
assert 1 == flattenedProject.dependencies.size()
assert 1 == flattenedProject.dependencies.dependency.size()
14 changes: 12 additions & 2 deletions src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,10 +1067,20 @@ private void createFlattenedDependenciesAll( List<Dependency> projectDependencie
{
return true;
}
if (node.getState() != DependencyNode.INCLUDED) {
if ("provided".equals(node.getArtifact().getScope()))
{
DependencyNode parent = node.getParent();
if(!parent.getArtifact().getGroupId().equals(projectArtifact.getGroupId()) || !parent.getArtifact().getArtifactId().equals(projectArtifact.getArtifactId()))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after if

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more general case where:

If we are flattening A where A -> B (provided) -> C
And we have consumer X where X -> A -> B (provided) -> C

  • B will not be in the transitive. So it doesn't need to be in the runtime scope. Initial thinking is - we can simply drop B and its subtree altogether, just like the optional case.
  • or, we can keep the information of provided scope dependencies in the flattened pom, so that in case a user sees a runtime classpath issue, user can know their environment is missing the provided artifacts.

In which case, do we keep only the first level one? or can the code be generic, and simply walk the tree and simply add the provided dependency, and return false to drop the children?

If we take that approach, what happens in the case of: A -> B -> C (provided) -> D
In this case, C is transitive. C doesn't show up in the compile path, but will we walk to C in this case? If we did, the flattened pom will look like A -> B, C (provided), is that the right behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given A flattened -> B (provided) -> C,
It may be better to maintain B but drop its subtree for information purposes. Namely if there's an issue with provided dependencies (e.g. it's not actually provided at runtime), the user will benefit from having additional information regarding the provided artifact. This is particularly useful in the case of differing dependency trees for the flattened POM and the original POM.

In the case of the code that is currently in the PR (where A is flattened),
A -> B (provided) -> C <==> Output: A -> B (provided)

I believe this should be the desired output as well, since we maintain our direct dependency on B from the flatten perspective. From the consumer perspective, it also gives a clear guideline for output (all second-level transitive provided dependencies are dropped, while first-level transitive provided dependencies are kept when compiling with the flattened POM). A good example of this would be the proto-javabigquerystorage / grpc-javabigquerystorage libraries.

It's also good for informational purposes, as mentioned with potential classpath issues, in which the consumer would receive more information on where their usage of provided dependencies might be going wrong with their provided dependencies.

In most cases we could just walk the tree, add the associated provided dependency to our linked list, then return false to not include its subtree. This would work for all cases except when a transitive node is managed to the provided scope (with a compile scope parent) ==> A -> B (compile) -> C (managed to 'provided')

In this case, we would need to know whether the node was transitive or not, meaning we would either have to look up from the node itself or look down from its parent.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the investigation and the clarification. I feel this is sound. The only last nits are the code format to be consistent w/ the rest of the file, i.e.,

if (....)
{
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All formatting has been fixed. Thank you for the notification!

{
return false;
}
}
if (node.getState() != DependencyNode.INCLUDED)
{
return false;
}
if (node.getArtifact().isOptional()) {
if (node.getArtifact().isOptional())
{
return false;
}
dependencyNodeLinkedList.add(node);
Expand Down