On Nov 21, 2007 11:21 AM, Gilles Scokart <[EMAIL PROTECTED]> wrote: > 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.
Good catch! > > > 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. This sounds ok, in some cases tests check that the ivy.xml file is not downloaded when we can be sure eviction is happening before the module being downloaded. But it shouldn't be the consequence of side effects between multiple root configurations, so in this case the test was probably too brilliant. > > 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. Nice, and it makes the code simpler. > > > I think those changes are quiete sensible. Indeed, good job! > 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. They may, we will see. But your change in conflict management is a good one, so if other bugs come up, we will fix them. BTW, some comments on the change follow inline in the diff. > > 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; > > - } > I see a problem with the removal of this test: if a problem occurs during loading or just after loading, we used to fall into this if statement, and we don't any more. I think we should preserver this even with your (nice) change adding logging to the loading operation. > > > 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) { > Mmm, I think this was introduced for someone needing to discard a configuration in a conflict manager. I agree that it suffers from several problem: no javadoc and no unit test, but I think it's better to keep it. The problem is that if we keep it we won't get information about people using it. So maybe what we could do is to add a log when this method is called. Something like: if (!"true".equals(IvyContext.getContext().getSettings().getVariable(" ivy.disable.log.on.discard.conf.I.have.reported.my.usage.to.Ivy.team"))) { Message.info("WARNING! WARNING! WARNING! WARNING!\nThis method is about to be removed by Ivy team if you don't report your use to the Ivy team.\nPlease add a comment to IVY-XXX to report how you use this method and for what reason.\nOnce reported, you can set ivy.disable.log.on.discard.conf.I.have.reported.my.usage.to.Ivy.team=true to disable this log"); } Then we open an issue to remove the method, due to be fixed in one year or so. If we have no comment on the issue, we can remove the method, otherwise we can add good comment to the method and a unit test depending on real world usage report, and mark the issue as won't fix. > > > 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. > Yes, it must be redone. Indeed, we try to resolve conflicts before loading metadata, just in case the conflict resolution has enough information to evict the node and avoid its loading (which may include costly operation such as searching over the repository the latest version, ...). But maybe the conflict manager didn't know before actually resolving the node, so we have to resolve conflict after loading data. I agree, this would deserve some comments in the code :-) That's all for me, Xavier -- Xavier Hanin - Independent Java Consultant http://xhab.blogspot.com/ http://ant.apache.org/ivy/ http://www.xoocode.org/
