Just a few words to help you to review this change. I started by making a minor refactoring to add some traces in IvyNode.
Then, to pass the unit test, I have fixed ResolveEngine.computeConflicts. In one of the case, it was not taking into account the rootConfiguration. However, that triggered some regression in other unit tests. One of the unit test (testTransitiveEviction) was actually too brilliant. It was checking that the ivy.xml was not downloaded. But the change that I made removed an incorrect eviction. So, now the ivy file is downloaded and only evicted after. Some other tests related to transitive eviction where also failing. I have added to notion of transitively evicted in the isEvicted method. I guess that the test was working before because of cross-configuration eviction, rather than transitive eviction. Finally, I found that my last regression was coming from the fact that we have two method markEvicted and one of them didn't made enough thing. So I merged them. I think those changes are quiete sensible. Some other bugs might have been catched. But the problem that I had with the regression also showed that this fix revealed some bugs that were hidden by the fact that at one place the conflict detection was made across all root configuration. So, new bugs may appear now. Gilles > -----Original Message----- > From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] > Sent: mercredi 21 novembre 2007 10:05 > To: [EMAIL PROTECTED] > Subject: svn commit: r596998 - in /incubator/ivy/core/trunk: > src/java/org/apache/ivy/core/resolve/ > test/java/org/apache/ivy/core/resolve/ > > Author: gscokart > Date: Wed Nov 21 01:04:46 2007 > New Revision: 596998 > > URL: http://svn.apache.org/viewvc?rev=596998&view=rev > Log: > IVY-644 fix NPE in case of eviction by 2 modules on 2 different confs mixed > with extends > > Modified: > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNode.java > > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNodeEviction.java > > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/ResolveEngine.java > > incubator/ivy/core/trunk/test/java/org/apache/ivy/core/resolve/ResolveTest.java > > Modified: > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNode.java > URL: > http://svn.apache.org/viewvc/incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNode. > java?rev=596998&r1=596997&r2=596998&view=diff > ============================================================================== > --- > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNode.java > (original) > +++ > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNode.java > Wed Nov 21 01:04:46 > 2007 > @@ -98,6 +98,10 @@ > //CheckStyle:MagicNumber| OFF > return hash; > } > + > + public String toString() { > + return "NodeConf(" + conf + ")"; > + } > } > > // //////// CONTEXT > @@ -182,14 +186,21 @@ > */ > public boolean loadData(String rootModuleConf, IvyNode parent, String > parentConf, String conf, > boolean shouldBePublic) { > + Message.debug("loadData of " + this.toString() + " of rootConf=" + > rootModuleConf); > if (!isRoot() && (data.getReport() != null)) { > data.getReport().addDependency(this); > } > > boolean loaded = false; > - if (!isEvicted(rootModuleConf) > - && (hasConfigurationsToLoad() || > !isRootModuleConfLoaded(rootModuleConf)) > - && !hasProblem()) { > + if (hasProblem()) { > + Message.debug("Node has problem. Skip loading"); > + handleConfiguration(loaded, rootModuleConf, parent, parentConf, > conf, shouldBePublic); > + return false; > + } else if (isEvicted(rootModuleConf)) { > + Message.debug(rootModuleConf + " is evicted. Skip loading"); > + } else if (!hasConfigurationsToLoad() && > isRootModuleConfLoaded(rootModuleConf)) { > + Message.debug(rootModuleConf + " is loaded and no conf to load. > Skip loading"); > + } else { > markRootModuleConfLoaded(rootModuleConf); > if (md == null) { > DependencyResolver resolver = > data.getSettings().getResolver(getModuleId()); > @@ -309,11 +320,6 @@ > loaded = true; > } > } > - if (hasProblem()) { > - return handleConfiguration(loaded, rootModuleConf, parent, > parentConf, conf, > - shouldBePublic) > - && loaded; > - } > if (!handleConfiguration(loaded, rootModuleConf, parent, parentConf, > conf, > shouldBePublic)) { > return false; > } > @@ -347,8 +353,7 @@ > "impossible to get dependencies when data has not been > loaded"); > } > DependencyDescriptor[] dds = md.getDependencies(); > - Collection dependencies = new LinkedHashSet(); // it's important to > respect dependencies > - // order > + Collection dependencies = new LinkedHashSet(); // it's important to > respect order > for (int i = 0; i < dds.length; i++) { > DependencyDescriptor dd = dds[i]; > String[] dependencyConfigurations = > dd.getDependencyConfigurations(conf, > requestedConf); > @@ -547,6 +552,7 @@ > return (String[]) depConfs.toArray(new String[depConfs.size()]); > } > > + //This is never called. Could we remove it? > public void discardConf(String rootModuleConf, String conf) { > Set depConfs = (Set) rootModuleConfs.get(rootModuleConf); > if (depConfs == null) { > @@ -750,7 +756,13 @@ > // no configuration required => no artifact required > return new Artifact[0]; > } > + if (md == null) { > + throw new IllegalStateException( > + "impossible to get artefacts when data has not been > loaded. IvyNode = " > + + this.toString()); > + } > > + > Set artifacts = new HashSet(); // the set we fill before returning > > // we check if we have dependencyArtifacts includes description for > this rootModuleConf > @@ -909,7 +921,8 @@ > public ConflictManager getConflictManager(ModuleId mid) { > if (md == null) { > throw new IllegalStateException( > - "impossible to get conflict manager when data has not > been loaded"); > + "impossible to get conflict manager when data has not > been loaded. IvyNode = " > + + this.toString()); > } > ConflictManager cm = md.getConflictManager(mid); > return cm == null ? settings.getConflictManager(mid) : cm; > @@ -1097,7 +1110,9 @@ > > public void markEvicted(String rootModuleConf, IvyNode node, > ConflictManager conflictManager, > Collection resolved) { > - eviction.markEvicted(rootModuleConf, node, conflictManager, > resolved); > + EvictionData evictionData = new EvictionData(rootModuleConf, node, > conflictManager, > + resolved); > + markEvicted(evictionData); > } > > public void setEvictedNodes(ModuleId moduleId, String rootModuleConf, > Collection evicted) { > > Modified: > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNodeEviction.java > URL: > http://svn.apache.org/viewvc/incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNodeE > viction.java?rev=596998&r1=596997&r2=596998&view=diff > ============================================================================== > --- > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNodeEviction.java > (original) > +++ > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/IvyNodeEviction.java > Wed Nov 21 > 01:04:46 2007 > @@ -91,6 +91,10 @@ > public String getRootModuleConf() { > return rootModuleConf; > } > + > + public boolean isTransitivelyEvicted() { > + return parent == null; > + } > } > > private static final class ModuleIdConf { > @@ -235,9 +239,13 @@ > public boolean isEvicted(String rootModuleConf) { > cleanEvicted(); > IvyNode root = node.getRoot(); > - return root != node > - && !root.getResolvedRevisions(node.getId().getModuleId(), > rootModuleConf).contains( > - node.getResolvedId()) && getEvictedData(rootModuleConf) > != null; > + ModuleId moduleId = node.getId().getModuleId(); > + Collection resolvedRevisions = root.getResolvedRevisions(moduleId, > rootModuleConf); > + EvictionData evictedData = getEvictedData(rootModuleConf); > + return root != node && evictedData != null > + && (!resolvedRevisions.contains(node.getResolvedId()) > + || evictedData.isTransitivelyEvicted() > + ); > } > > public boolean isCompletelyEvicted() { > @@ -271,13 +279,6 @@ > } > } > } > - } > - > - public void markEvicted(String rootModuleConf, IvyNode node, > ConflictManager conflictManager, > - Collection resolved) { > - EvictionData evictionData = new EvictionData(rootModuleConf, node, > conflictManager, > - resolved); > - markEvicted(evictionData); > } > > public void markEvicted(EvictionData evictionData) { > > Modified: > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/ResolveEngine.java > URL: > http://svn.apache.org/viewvc/incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/ResolveE > ngine.java?rev=596998&r1=596997&r2=596998&view=diff > ============================================================================== > --- > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/ResolveEngine.java > (original) > +++ > incubator/ivy/core/trunk/src/java/org/apache/ivy/core/resolve/ResolveEngine.java > Wed Nov 21 > 01:04:46 2007 > @@ -548,10 +548,11 @@ > } else { > Message.verbose("== resolving dependencies for " + node.getId() > + " [" + conf + "]"); > } > - resolveConflict(node); > + resolveConflict(node, conf); > > if (node.loadData(conf, shouldBePublic)) { > - resolveConflict(node); > + resolveConflict(node, conf); //We just did it. Should it be > redone? > + //NB:removing it break a unit test. > if (!node.isEvicted()) { > String[] confs = node.getRealConfs(conf); > for (int i = 0; i < confs.length; i++) { > @@ -672,8 +673,8 @@ > return false; > } > > - private void resolveConflict(VisitNode node) { > - resolveConflict(node, node.getParent(), Collections.EMPTY_SET); > + private void resolveConflict(VisitNode node, String conf) { > + resolveConflict(node, node.getParent(), conf,Collections.EMPTY_SET); > } > > /** > @@ -689,7 +690,8 @@ > * descendants of ancestor) > * @return true if conflict resolution has been done, false it can't be > done yet > */ > - private boolean resolveConflict(VisitNode node, VisitNode ancestor, > Collection toevict) { > + private boolean resolveConflict(VisitNode node, VisitNode ancestor, > String conf, > + Collection toevict) { > if (ancestor == null || node == ancestor) { > return true; > } > @@ -702,7 +704,7 @@ > // job is done and node is selected, nothing to do for this > ancestor, but we still > have > // to check higher levels, for which conflict resolution might > have been impossible > // before > - if (resolveConflict(node, ancestor.getParent(), toevict)) { > + if (resolveConflict(node, ancestor.getParent(), conf, toevict)) { > // now that conflict resolution is ok in ancestors > // we just have to check if the node wasn't previously > evicted in root ancestor > EvictionData evictionData = > node.getEvictionDataInRoot(node.getRootModuleConf(), > @@ -730,7 +732,7 @@ > node.getModuleId(), node.getRootModuleConf())); > > resolvedNodes.addAll(ancestor.getNode().getPendingConflicts(node.getRootModuleConf(), > node.getModuleId())); > - Collection conflicts = computeConflicts(node, ancestor, toevict, > resolvedNodes); > + Collection conflicts = computeConflicts(node, ancestor, conf, > toevict, resolvedNodes); > > if (settings.debugConflictResolution()) { > Message.debug("found conflicting revisions for " + node + " in " > + ancestor + ": " > @@ -788,7 +790,7 @@ > ancestor.getNode().setPendingConflicts(node.getModuleId(), > node.getRootModuleConf(), > Collections.EMPTY_SET); > > - return resolveConflict(node, ancestor.getParent(), toevict); > + return resolveConflict(node, ancestor.getParent(), conf, > toevict); > } else { > // node has been evicted for the current parent > if (resolved.isEmpty()) { > @@ -827,7 +829,7 @@ > IvyNode sel = (IvyNode) iter.next(); > if (!prevResolved.contains(sel)) { > solved &= resolveConflict(node.gotoNode(sel), > ancestor.getParent(), > - toevict); > + conf, toevict); > } > } > } > @@ -835,8 +837,8 @@ > } > } > > - private Collection computeConflicts(VisitNode node, VisitNode ancestor, > Collection toevict, > - Collection resolvedNodes) { > + private Collection computeConflicts(VisitNode node, VisitNode ancestor, > String conf, > + Collection toevict, Collection resolvedNodes) { > Collection conflicts = new HashSet(); > conflicts.add(node.getNode()); > if (resolvedNodes.removeAll(toevict)) { > @@ -853,14 +855,13 @@ > .addAll(dep.getResolvedNodes(node.getModuleId(), > node.getRootModuleConf())); > } > } else if (resolvedNodes.isEmpty() && node.getParent() != ancestor) { > - DependencyDescriptor[] dds = > ancestor.getDescriptor().getDependencies(); > - for (int i = 0; i < dds.length; i++) { > - if (dds[i].getDependencyId().equals(node.getModuleId())) { > - IvyNode n = > node.getNode().findNode(dds[i].getDependencyRevisionId()); > - if (n != null) { > - conflicts.add(n); > - break; > - } > + //Conflict must only be computed per root configuration at this > step. > + Collection ancestorDepIvyNodes = node.getParent().getNode() > + .getDependencies(node.getRootModuleConf(), new > String[] {conf}); > + for (Iterator it = ancestorDepIvyNodes.iterator(); > it.hasNext();) { > + IvyNode ancestorDep = (IvyNode) it.next(); > + if (ancestorDep.getModuleId().equals(node.getModuleId())) { > + conflicts.add(ancestorDep); > } > } > } else { > > Modified: > incubator/ivy/core/trunk/test/java/org/apache/ivy/core/resolve/ResolveTest.java > URL: > http://svn.apache.org/viewvc/incubator/ivy/core/trunk/test/java/org/apache/ivy/core/resolve/Resolve > Test.java?rev=596998&r1=596997&r2=596998&view=diff > ============================================================================== > --- > incubator/ivy/core/trunk/test/java/org/apache/ivy/core/resolve/ResolveTest.java > (original) > +++ > incubator/ivy/core/trunk/test/java/org/apache/ivy/core/resolve/ResolveTest.java > Wed Nov 21 > 01:04:46 2007 > @@ -18,9 +18,6 @@ > package org.apache.ivy.core.resolve; > > import java.io.File; > -import java.io.IOException; > -import java.net.MalformedURLException; > -import java.text.ParseException; > import java.util.ArrayList; > import java.util.Arrays; > import java.util.Collection; > @@ -35,7 +32,6 @@ > > import org.apache.ivy.Ivy; > import org.apache.ivy.TestHelper; > -import org.apache.ivy.core.IvyContext; > import org.apache.ivy.core.cache.ArtifactOrigin; > import org.apache.ivy.core.cache.CacheManager; > import org.apache.ivy.core.module.descriptor.Artifact; > @@ -58,9 +54,7 @@ > import org.apache.ivy.plugins.resolver.DualResolver; > import org.apache.ivy.plugins.resolver.FileSystemResolver; > import org.apache.ivy.util.CacheCleaner; > -import org.apache.ivy.util.DefaultMessageLogger; > import org.apache.ivy.util.FileUtil; > -import org.apache.ivy.util.Message; > import org.apache.tools.ant.Project; > import org.apache.tools.ant.taskdefs.Delete; > import org.xml.sax.SAXException; > @@ -675,8 +669,6 @@ > ModuleRevisionId.newInstance("org1", "mod1.2", "2.1")).exists()); > assertTrue(getArchiveFileInCache("org1", "mod1.2", "2.1", "mod1.2", > "jar", > "jar").exists()); > > - assertFalse(cacheManager.getIvyFileInCache( > - ModuleRevisionId.newInstance("org1", "mod1.2", "2.0")).exists()); > assertFalse(getArchiveFileInCache("org1", "mod1.2", "2.0", "mod1.2", > "jar", > "jar").exists()); > } > > @@ -1474,7 +1466,7 @@ > public void testTransitiveEviction() throws Exception { > // mod7.3 depends on mod7.2 v1.0 and on mod7.1 v2.0 > // mod7.2 v1.0 depends on mod7.1 v1.0 (which then should be evicted) > - // mod7.1 v1.0 depends on mod 1.2 v1.0 (which should be evicted by > transitivity) > + // mod7.1 v1.0 depends on mod 1.2 v2.0 (which should be evicted by > transitivity) > > ResolveReport report = ivy.resolve(new > File("test/repositories/2/mod7.3/ivy-1.0.xml") > .toURL(), getResolveOptions(new String[] {"*"})); > @@ -1495,9 +1487,9 @@ > ModuleRevisionId.newInstance("org7", "mod7.1", "2.0")).exists()); > assertTrue(getArchiveFileInCache("org7", "mod7.1", "2.0", "mod7.1", > "jar", > "jar").exists()); > > - assertTrue(!getArchiveFileInCache("org7", "mod7.1", "1.0", "mod7.1", > "jar", > "jar").exists()); > + assertFalse(getArchiveFileInCache("org7", "mod7.1", "1.0", "mod7.1", > "jar", > "jar").exists()); > > - assertTrue(!getArchiveFileInCache("org1", "mod1.2", "2.0", "mod1.2", > "jar", > "jar").exists()); > + assertFalse(getArchiveFileInCache("org1", "mod1.2", "2.0", "mod1.2", > "jar", > "jar").exists()); > } > > public void testTransitiveEviction2() throws Exception { > @@ -1753,15 +1745,15 @@ > .newInstance("org5", "mod5.1", "4.2")).length); > } > > - /* > + > public void testMultipleEviction() throws Exception { > > ResolveReport report = ivy.resolve( > new > File("test/repositories/1/IVY-644/M1/ivys/ivy-1.0.xml").toURL(), > - getResolveOptions(new String[] {"*"})); > + getResolveOptions(new String[] {"test" , "runtime" })); //NB the > order impact the bug > assertFalse(report.hasError()); > } > - */ > + > > public void testResolveForce() throws Exception { > // mod4.1 v 4.2 depends on
