Remove first level transitive provided scope dependencies and their subtrees #172
Conversation
I think there are a couple of reasons: (1) In the current state, provided transitive dependencies are only kept if managed to provided or changed to provided by their parent. This change would keep the behavior consistent (always dropping provided transitive dependencies, instead of only sometimes). (2) Originally, Maven says that provided dependencies should not be transitive [1]. From the consumer perspective of the flatten plugin, they'd end up being second level transitives. If one of these dependencies is needed at compile time, it might make more sense to include it immediately within the consumer POM. |
|
|
||
| if ("provided".equals(node.getArtifact().getScope())) { | ||
| DependencyNode parent = node.getParent(); | ||
| if(!parent.getArtifact().getGroupId().equals(projectArtifact.getGroupId()) || !parent.getArtifact().getArtifactId().equals(projectArtifact.getArtifactId())) |
There was a problem hiding this comment.
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
Bwill not be in the transitive. So it doesn't need to be in the runtime scope. Initial thinking is - we can simply dropBand its subtree altogether, just like theoptionalcase.- or, we can keep the information of
providedscope dependencies in the flattened pom, so that in case a user sees a runtime classpath issue, user can know their environment is missing theprovidedartifacts.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (....)
{
...
}
There was a problem hiding this comment.
All formatting has been fixed. Thank you for the notification!
| <dependencies> | ||
| <dependency> | ||
| <groupId>org.codehaus.mojo.flatten.its</groupId> | ||
| <artifactId>parent-deps</artifactId> |
There was a problem hiding this comment.
I'm confused. Why is this called parent-deps when its not the parent of this pom? Consider renaming some things.
There was a problem hiding this comment.
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.
|
@olamy PTAL |
|
@lasselindqvist @olamy could we get this merged and released please? |
…ubtrees (#172) * Remove first level transitive, provided scope dependencies and their subtrees * Removed accidentally included 'optional' test. * Fixed weird spacing modifications from before * Fixed weird spacing issues with PR * Fixing spacing issue Co-authored-by: Saleh Mostafa <[email protected]>
This also contains associated tests to display the behaviors (and maintained behaviors).
(1) Adds 5 lines of code to FlattenMojo.createFlattenedDependenciesAll(). Checks for 'provided' dependencies, then for transitivity (i.e. its parent’s groupID/artifactID don’t match the project’s groupID/artifactID). If it is both provided and transitive, the dependency is removed.
(2) Added a file (parent-deps-1.1.pom) containing a provided dependency to the 'mrm' directory.
(3) Added four associated tests:
----------(a) flatten-dependency-all-provided-children - contains a direct provided scope dependency - this passes in the original version and the PR and is to test that correct behavior is maintained.
----------(b) flatten-dependency-all-provided-transitive - contains a transitive pre-managed provided scope dependency - this passes in the original version and the PR and is to test that correct behavior is maintained.
----------(c) flatten-dependency-all-depmgnt-provided-transitive - contains a transitive compile scope dependency managed to 'provided' - this fails in the original version (the dependency is kept) and the PR functions to drop this dependency.
----------(d) flatten-dependency-all-provided-children - contains a direct provided dependency with a compile scope child - this fails in the original version (the dependency is kept) and the PR functions to drop this dependency.
Expected Behavior of this PR:
If we are flattening A such that A -> B -> C (provided) -> D, our flattened tree will become A -> B (regardless of whether we have D).
If we are flattening E such that E -> F (provided) -> G, our flattened tree will become E -> F (provided)
Direct provided dependencies are kept. Any transitive dependency marked as provided will be removed, as will its children.
@stephaniewang526 @saturnism @elharo