You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/11/27 19:15:15 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request #137: [MRESOLVER-228] Proposed changes
cstamas opened a new pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137
Proposed changes for https://github.com/apache/maven-resolver/pull/136
@caiwei-ebay please take a look
Changes:
* cleanup
* do not create reconciler when not used (moved tiny logic)
* clean up flow in two cases (reconciler used and reconciler not used)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #137: [MRESOLVER-228] Proposed changes
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137#issuecomment-982433000
@caiwei-ebay Please integrate this into your PR first, then we will continue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] caiwei-ebay commented on pull request #137: [MRESOLVER-228] Proposed changes
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137#issuecomment-982401111
@michael-o @cstamas
The code changes looks good to me and it's much cleaner. Thanks for updating the PR.
Is there anything I can help further before you folks accept the PR?
Thanks,
caiwe-ebay
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a change in pull request #137: [MRESOLVER-228] Proposed changes
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137#discussion_r757813151
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/ConflictWinnerFinder.java
##########
@@ -100,9 +93,9 @@ public ConflictWinnerFinder( boolean verbose )
{
if ( i < index )
{
- if ( verbose )
+ if ( LOGGER.isDebugEnabled() )
Review comment:
This is redundant. SLF4J will do the appropriate magic.
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java
##########
@@ -169,8 +161,13 @@ public CollectResult collectDependencies( RepositorySystemSession session, Colle
requireNonNull( request, "request cannot be null" );
session = optimizeSession( session );
- verbose = ConfigUtils.getBoolean( session, false, CONFIG_PROP_RESOLVER_VERBOSE_MODE );
- skipMode = ConfigUtils.getBoolean( session, true, CONFIG_PROP_RESOLVER_MODE );
+ boolean useSkipReconcile = ConfigUtils.getBoolean(
+ session, CONFIG_PROP_USE_SKIP_RECONCILE_DEFAULT, CONFIG_PROP_USE_SKIP_RECONCILE
+ );
+ if ( useSkipReconcile )
+ {
+ LOGGER.debug( "Collector skip & reconcile enabled." );
Review comment:
I think such a message is redundant as well because the user explicitly wanted to skip this.
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/ConflictWinnerFinder.java
##########
@@ -150,20 +144,20 @@ public ConflictWinnerFinder( boolean verbose )
}
catch ( RepositoryException e )
{
- res.addException( e );
+ result.addException( e );
}
- if ( !res.getExceptions().isEmpty() )
+ if ( !result.getExceptions().isEmpty() )
{
- throw new DependencyCollectionException( res );
+ throw new DependencyCollectionException( result );
}
DependencyConflictWinnersVisitor conflicts = new DependencyConflictWinnersVisitor();
root.accept( conflicts );
- if ( verbose )
+ if ( LOGGER.isDebugEnabled() )
Review comment:
Same here
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java
##########
@@ -536,47 +538,70 @@ private void doRecurse( Args args, Results results, List<RemoteRepository> repos
VersionFilter childFilter = verFilter != null ? verFilter.deriveChildFilter( context ) : null;
final List<RemoteRepository> childRepos =
- args.ignoreRepos
- ? repositories
- : remoteRepositoryManager.aggregateRepositories( args.session, repositories,
- descriptorResult.getRepositories(), true );
+ args.ignoreRepos
+ ? repositories
+ : remoteRepositoryManager.aggregateRepositories( args.session, repositories,
+ descriptorResult.getRepositories(), true );
- Object key =
- args.pool.toKey( d.getArtifact(), childRepos, childSelector, childManager, childTraverser, childFilter );
+ Object key = args.pool.toKey(
+ d.getArtifact(), childRepos, childSelector, childManager, childTraverser, childFilter
+ );
- List<DependencyNode> parents = args.nodes.getParentNodes();
- int depth = parents.size() + 1; //the depth if pushed the child to stack
- List<DependencyNode> cachedChildren = args.pool.getChildren( key );
- DependencyResolveReconciler.CacheResult result = args.reconciler.findCache( child, cachedChildren, depth );
- if ( result != null )
+ if ( args.reconciler == null )
{
- if ( result.candidateWithSameKey )
+ List<DependencyNode> children = args.pool.getChildren( key );
+ if ( children == null )
{
- child.setChildren( result.dependencyNodes );
+ args.pool.putChildren( key, child.getChildren() );
+
+ args.nodes.push( child );
+
+ process( args, results, descriptorResult.getDependencies(), childRepos, childSelector, childManager,
+ childTraverser, childFilter );
+
+ args.nodes.pop();
}
- else if ( result.candidateWithLowerDepth )
+ else
{
- //No need to set the children as the result can be ignored (won't be picked up)
- args.reconciler.addSkip( child, key, descriptorResult.getDependencies(), parents,
- result.parentPathsOfCandidateLowerDepth );
- if ( verbose )
- {
- LOGGER.info( "Skipped resolving artifact {} of depth {}", child.getArtifact(), depth );
- }
+ child.setChildren( children );
}
}
else
{
- args.nodes.push( child );
- if ( verbose )
+ List<DependencyNode> parents = args.nodes.getParentNodes();
+ int depth = parents.size() + 1; //the depth if pushed the child to stack
+ List<DependencyNode> cachedChildren = args.pool.getChildren( key );
+ DependencyResolveReconciler.CacheResult result = args.reconciler.findCache( child, cachedChildren, depth );
+ if ( result != null )
{
- LOGGER.info( "Resolving artifact {} of depth {}", child.getArtifact(), depth );
+ if ( result.candidateWithSameKey )
+ {
+ child.setChildren( result.dependencyNodes );
+ }
+ else if ( result.candidateWithLowerDepth )
+ {
+ //No need to set the children as the result can be ignored (won't be picked up)
+ args.reconciler.addSkip( child, key, descriptorResult.getDependencies(), parents,
+ result.parentPathsOfCandidateLowerDepth );
+ if ( LOGGER.isDebugEnabled() )
Review comment:
Same here
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java
##########
@@ -536,47 +538,70 @@ private void doRecurse( Args args, Results results, List<RemoteRepository> repos
VersionFilter childFilter = verFilter != null ? verFilter.deriveChildFilter( context ) : null;
final List<RemoteRepository> childRepos =
- args.ignoreRepos
- ? repositories
- : remoteRepositoryManager.aggregateRepositories( args.session, repositories,
- descriptorResult.getRepositories(), true );
+ args.ignoreRepos
+ ? repositories
+ : remoteRepositoryManager.aggregateRepositories( args.session, repositories,
+ descriptorResult.getRepositories(), true );
- Object key =
- args.pool.toKey( d.getArtifact(), childRepos, childSelector, childManager, childTraverser, childFilter );
+ Object key = args.pool.toKey(
+ d.getArtifact(), childRepos, childSelector, childManager, childTraverser, childFilter
+ );
- List<DependencyNode> parents = args.nodes.getParentNodes();
- int depth = parents.size() + 1; //the depth if pushed the child to stack
- List<DependencyNode> cachedChildren = args.pool.getChildren( key );
- DependencyResolveReconciler.CacheResult result = args.reconciler.findCache( child, cachedChildren, depth );
- if ( result != null )
+ if ( args.reconciler == null )
{
- if ( result.candidateWithSameKey )
+ List<DependencyNode> children = args.pool.getChildren( key );
+ if ( children == null )
{
- child.setChildren( result.dependencyNodes );
+ args.pool.putChildren( key, child.getChildren() );
+
+ args.nodes.push( child );
+
+ process( args, results, descriptorResult.getDependencies(), childRepos, childSelector, childManager,
+ childTraverser, childFilter );
+
+ args.nodes.pop();
}
- else if ( result.candidateWithLowerDepth )
+ else
{
- //No need to set the children as the result can be ignored (won't be picked up)
- args.reconciler.addSkip( child, key, descriptorResult.getDependencies(), parents,
- result.parentPathsOfCandidateLowerDepth );
- if ( verbose )
- {
- LOGGER.info( "Skipped resolving artifact {} of depth {}", child.getArtifact(), depth );
- }
+ child.setChildren( children );
}
}
else
{
- args.nodes.push( child );
- if ( verbose )
+ List<DependencyNode> parents = args.nodes.getParentNodes();
+ int depth = parents.size() + 1; //the depth if pushed the child to stack
+ List<DependencyNode> cachedChildren = args.pool.getChildren( key );
+ DependencyResolveReconciler.CacheResult result = args.reconciler.findCache( child, cachedChildren, depth );
+ if ( result != null )
{
- LOGGER.info( "Resolving artifact {} of depth {}", child.getArtifact(), depth );
+ if ( result.candidateWithSameKey )
+ {
+ child.setChildren( result.dependencyNodes );
+ }
+ else if ( result.candidateWithLowerDepth )
+ {
+ //No need to set the children as the result can be ignored (won't be picked up)
+ args.reconciler.addSkip( child, key, descriptorResult.getDependencies(), parents,
+ result.parentPathsOfCandidateLowerDepth );
+ if ( LOGGER.isDebugEnabled() )
+ {
+ LOGGER.debug( "Skipped resolving artifact {} of depth {}", child.getArtifact(), depth );
+ }
+ }
+ }
+ else
+ {
+ args.nodes.push( child );
+ if ( LOGGER.isDebugEnabled() )
Review comment:
Same here
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyResolveReconciler.java
##########
@@ -171,9 +143,9 @@ public void addSkip( DependencyNode current, Object key, List<Dependency> depend
return Collections.emptyList();
}
- if ( verbose )
+ if ( LOGGER.isDebugEnabled() )
Review comment:
Same here
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyResolveReconciler.java
##########
@@ -211,31 +183,22 @@ public void addSkip( DependencyNode current, Object key, List<Dependency> depend
}
//group nodes by artifact
- Multimap<Artifact, DependencyResolveSkip> reconcileNodes = LinkedHashMultimap.create();
+ HashMap<Artifact, ArrayList<DependencyResolveSkip>> reconcileNodes = new HashMap<>();
for ( DependencyResolveSkip skip : skips )
{
- reconcileNodes.put( skip.node.getArtifact(), skip );
+ reconcileNodes.computeIfAbsent( skip.node.getArtifact(), k -> new ArrayList<>() ).add( skip );
}
//only reconcile the node with lowest depth
LinkedHashSet<DependencyResolveSkip> filteredSkips = new LinkedHashSet<>();
for ( Artifact key : reconcileNodes.keySet() )
{
Collection<DependencyResolveSkip> col = reconcileNodes.get( key );
- List<DependencyResolveSkip> list = new ArrayList<>();
- list.addAll( col );
- Collections.sort( list, new Comparator<DependencyResolveSkip>()
- {
- @Override
- public int compare( DependencyResolveSkip o1, DependencyResolveSkip o2 )
- {
- return o1.depth - o2.depth;
- }
- } );
-
- if ( verbose )
+ List<DependencyResolveSkip> list = new ArrayList<>( col );
+ list.sort( Comparator.comparingInt( o -> o.depth ) );
+ if ( LOGGER.isDebugEnabled() )
Review comment:
Same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] caiwei-ebay commented on a change in pull request #137: [MRESOLVER-228] Proposed changes
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on a change in pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137#discussion_r758174653
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java
##########
@@ -169,8 +161,13 @@ public CollectResult collectDependencies( RepositorySystemSession session, Colle
requireNonNull( request, "request cannot be null" );
session = optimizeSession( session );
- verbose = ConfigUtils.getBoolean( session, false, CONFIG_PROP_RESOLVER_VERBOSE_MODE );
- skipMode = ConfigUtils.getBoolean( session, true, CONFIG_PROP_RESOLVER_MODE );
+ boolean useSkipReconcile = ConfigUtils.getBoolean(
+ session, CONFIG_PROP_USE_SKIP_RECONCILE_DEFAULT, CONFIG_PROP_USE_SKIP_RECONCILE
+ );
+ if ( useSkipReconcile )
+ {
+ LOGGER.debug( "Collector skip & reconcile enabled." );
Review comment:
@michael-o @cstamas
Thanks for reviewing and updating the PR.
I provide a property just for compatibility consideration. As I'm confident with the "skip & reconcile" approach as we've dryrun 2000+ applications in our company, so I would raise both hands in favour of that we don't provide a property to disable this behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a change in pull request #137: [MRESOLVER-228] Proposed changes
Posted by GitBox <gi...@apache.org>.
cstamas commented on a change in pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137#discussion_r757953664
##########
File path: maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java
##########
@@ -169,8 +161,13 @@ public CollectResult collectDependencies( RepositorySystemSession session, Colle
requireNonNull( request, "request cannot be null" );
session = optimizeSession( session );
- verbose = ConfigUtils.getBoolean( session, false, CONFIG_PROP_RESOLVER_VERBOSE_MODE );
- skipMode = ConfigUtils.getBoolean( session, true, CONFIG_PROP_RESOLVER_MODE );
+ boolean useSkipReconcile = ConfigUtils.getBoolean(
+ session, CONFIG_PROP_USE_SKIP_RECONCILE_DEFAULT, CONFIG_PROP_USE_SKIP_RECONCILE
+ );
+ if ( useSkipReconcile )
+ {
+ LOGGER.debug( "Collector skip & reconcile enabled." );
Review comment:
It was present in original PR by @caiwei-ebay so let's wait for his response here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #137: [MRESOLVER-228] Proposed changes
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137#issuecomment-984966381
Integrated into upstream branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o closed pull request #137: [MRESOLVER-228] Proposed changes
Posted by GitBox <gi...@apache.org>.
michael-o closed pull request #137:
URL: https://github.com/apache/maven-resolver/pull/137
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org