diff --git a/core/src/main/java/hudson/EnvVars.java b/core/src/main/java/hudson/EnvVars.java index f315242da8..b8600a5ddf 100644 --- a/core/src/main/java/hudson/EnvVars.java +++ b/core/src/main/java/hudson/EnvVars.java @@ -26,6 +26,8 @@ package hudson; import hudson.remoting.Callable; import hudson.remoting.VirtualChannel; import hudson.util.CaseInsensitiveComparator; +import hudson.util.CyclicGraphDetector; +import hudson.util.CyclicGraphDetector.CycleDetectedException; import hudson.util.VariableResolver; import java.io.File; @@ -40,6 +42,7 @@ import java.util.TreeMap; import java.util.Arrays; import java.util.TreeSet; import java.util.UUID; +import java.util.logging.Logger; /** * Environment variables. @@ -69,6 +72,7 @@ import java.util.UUID; * @author Kohsuke Kawaguchi */ public class EnvVars extends TreeMap { + private static Logger LOGGER = Logger.getLogger(EnvVars.class.getName()); /** * If this {@link EnvVars} object represents the whole environment variable set, * not just a partial list used for overriding later, then we need to know @@ -158,21 +162,8 @@ public class EnvVars extends TreeMap { /** * Calculates the order to override variables. * - * We should override variables in a following order: - *
    - *
  1. variables that does not contain variable expressions.
    2. - *
  2. variables that refers variables overridden in 1.
    3. - *
  3. variables that refers variables overridden in 2.
    4. - *
  4. ...
    5. - *
  5. (last) variables contains '+' (as PATH+MAVEN)
    6. - *
+ * Sort variables with topological sort with their reference graph. * - * This class orders variables in a following way: - *
    - *
  1. scan each overriding variables and list all referred variables (includes indirect references).
    2. - *
  2. sort variables with a number of referring variables (ascending order).
    3. - *
- * * This is package accessible for testing purpose. */ static class OverrideOrderCalculator { @@ -198,143 +189,133 @@ public class EnvVars extends TreeMap { } } + private static class VariableReferenceSorter extends CyclicGraphDetector { + // map from a variable to a set of variables that variable refers. + private final Map> refereeSetMap; + + public VariableReferenceSorter(Map> refereeSetMap) { + this.refereeSetMap = refereeSetMap; + } + + @Override + protected Iterable getEdges(String n) { + // return variables referred from the variable. + if (!refereeSetMap.containsKey(n)) { + // there is a case a non-existing variable is referred... + return Collections.emptySet(); + } + return refereeSetMap.get(n); + } + }; + private final Comparator comparator; + private final EnvVars target; private final Map overrides; - /** - * set of variables that a variable is REFERRING. - * When A=${B}, refereeSetMap.get("A").contains("B"). - * Also contains indirect references, when A=${B}, B=${C}, - * refereeSetMap.get("A").contains("C"). - */ - private Map> refereeSetMap; - /** - * set of variables that a variable is REFERRED BY. - * When A=${B}, referrerSetMap.get("B").contains("A"). - * Also contains indirect references, when A=${B}, B=${C}, - * referrerSetMap.get("C").contains("A"). - */ - private Map> referrerSetMap; + private Map> refereeSetMap; + private List orderedVariableNames; public OverrideOrderCalculator(EnvVars target, Map overrides) { comparator = target.comparator(); + this.target = target; this.overrides = overrides; - refereeSetMap = new TreeMap>(comparator); - referrerSetMap = new TreeMap>(comparator); scan(); } - private int getRefereeNum(String variable) { - if (refereeSetMap.containsKey(variable)) { - return refereeSetMap.get(variable).size(); - } - return 0; + public List getOrderedVariableNames() { + return orderedVariableNames; } - public List getOrderedVariableNames() { - List nameList = new ArrayList(overrides.keySet()); - Collections.sort(nameList, new Comparator() { - @Override - public int compare(String o1, String o2) { - if (o1.indexOf('+') > -1) { - if (o2.indexOf('+') > -1) { - // ABC+FOO == XYZ+BAR - return 0; - } - // ABC+FOO > BAR - return 1; - } - - if (o2.indexOf('+') > -1) { - // FOO < ABC+BAR - return -1; - } - - // depends on the number of variables each variable refers. - return getRefereeNum(o1) - getRefereeNum(o2); + // Cut the reference to the variable in a cycle. + private void cutCycleAt(String referee, List cycle) { + // cycle contains variables in referrer-to-referee order. + // This should not be negative, for the first and last one is same. + int refererIndex = cycle.lastIndexOf(referee) - 1; + + assert(refererIndex >= 0); + String referrer = cycle.get(refererIndex); + boolean removed = refereeSetMap.get(referrer).remove(referee); + assert(removed); + LOGGER.warning(String.format("Cyclic reference detected: %s", Util.join(cycle," -> "))); + LOGGER.warning(String.format("Cut the reference %s -> %s", referrer, referee)); + } + + // Cut the variable reference in a cycle. + private void cutCycle(List cycle) { + // if an existing variable is contained in that cycle, + // cut the cycle with that variable: + // existing: + // PATH=/usr/bin + // overriding: + // PATH1=/usr/local/bin:${PATH} + // PATH=/opt/something/bin:${PATH1} + // then consider reference PATH1 -> PATH can be ignored. + for (String referee: cycle) { + if (target.containsKey(referee)) { + cutCycleAt(referee, cycle); + return; } - }); - return nameList; + } + + // if not, cut the reference to the first one. + cutCycleAt(cycle.get(0), cycle); } /** * Scan all variables and list all referring variables. */ public void scan() { + refereeSetMap = new TreeMap>(comparator); + List extendingVariableNames = new ArrayList(); + TraceResolver resolver = new TraceResolver(comparator); - for (String currentVar: overrides.keySet()) { - if (currentVar.indexOf('+') > 0) { + for (Map.Entry entry: overrides.entrySet()) { + if (entry.getKey().indexOf('+') > 0) { // XYZ+AAA variables should be always processed in last. + extendingVariableNames.add(entry.getKey()); continue; } resolver.clear(); - Util.replaceMacro(overrides.get(currentVar), resolver); + Util.replaceMacro(entry.getValue(), resolver); // Variables directly referred from the current scanning variable. Set refereeSet = resolver.referredVariables; - - if (refereeSet.isEmpty()) { - // nothing to do if this variables does not refer other variables. + // Ignore self reference. + refereeSet.remove(entry.getKey()); + refereeSetMap.put(entry.getKey(), refereeSet); + } + + VariableReferenceSorter sorter; + while(true) { + sorter = new VariableReferenceSorter(refereeSetMap); + try { + sorter.run(refereeSetMap.keySet()); + } catch(CycleDetectedException e) { + // cyclic reference found. + // cut the cycle and retry. + @SuppressWarnings("unchecked") + List cycle = e.cycle; + cutCycle(cycle); continue; } - - // Find indirect referred variables: - // A=${B} - // CurrentVar=${A} - // -> CurrentVar refers B. - Set indirectRefereeSet = new TreeSet(comparator); - for (String referee: refereeSet) { - if (refereeSetMap.containsKey(referee)) { - indirectRefereeSet.addAll(refereeSetMap.get(referee)); - } - } - - // now contains variables referred both directly and indirectly. - refereeSet.addAll(indirectRefereeSet); - - // Variables refers the current scanning variable. - // this contains both direct and indirect reference. - Set referrerSet = referrerSetMap.get(currentVar); - - // what I have to do: - // 1. Create a link between the current scanning variable and referred variables. - // 1-a. register the current variable as a referrer of referred variables. - // 1-b. register referred variables as a referee of the current variable. - // 2. Create links between referring variables and referred variables. - // 2-a. register referring variables as referrers of referred variables. - // 2-b. register referred variables as referees of referring variables. - // - // Links between referring variables and the current scanning variable - // is already created from referring variables. - for (String referee: refereeSet) { - if (!referrerSetMap.containsKey(referee)) { - referrerSetMap.put(referee, new TreeSet(comparator)); - } - // 1-a. register the current variable as a referrer of referred variables. - referrerSetMap.get(referee).add(currentVar); - // 2-b. register referred variables as referees of referring variables. - if (referrerSet != null) { - referrerSetMap.get(referee).addAll(referrerSet); - } - } - - if (!refereeSetMap.containsKey(currentVar)) { - refereeSetMap.put(currentVar, new TreeSet(comparator)); - } - // 1-b. register referred variables as a referee of the current variable. - refereeSetMap.get(currentVar).addAll(refereeSet); - - if (referrerSet != null) { - for (String referer: referrerSet) { - // 2-b. register referred variables as referees of referring variables. - // For referrer refers the current scanning variable, - // refereeSetMap.get(referer) always exists. - refereeSetMap.get(referer).addAll(refereeSet); - } + break; + } + + // When A refers B, the last appearance of B always comes after + // the last appearance of A. + List reversedDuplicatedOrder = new ArrayList(sorter.getSorted()); + Collections.reverse(reversedDuplicatedOrder); + + orderedVariableNames = new ArrayList(overrides.size()); + for(String key: reversedDuplicatedOrder) { + if(overrides.containsKey(key) && !orderedVariableNames.contains(key)) { + orderedVariableNames.add(key); } } + Collections.reverse(orderedVariableNames); + orderedVariableNames.addAll(extendingVariableNames); } } diff --git a/core/src/test/java/hudson/EnvVarsTest.java b/core/src/test/java/hudson/EnvVarsTest.java index 5ef414bd60..1da7b51686 100644 --- a/core/src/test/java/hudson/EnvVarsTest.java +++ b/core/src/test/java/hudson/EnvVarsTest.java @@ -118,13 +118,18 @@ public class EnvVarsTest extends TestCase { public void testOverrideOrderCalculatorCyclic() { EnvVars env = new EnvVars(); + env.put("C", "Existing"); EnvVars overrides = new EnvVars(); overrides.put("A", "${B}"); - overrides.put("B", "${C}"); + overrides.put("B", "${C}"); // This will be ignored. overrides.put("C", "${A}"); + overrides.put("D", "${C}${E}"); + overrides.put("E", "${C}${D}"); + OverrideOrderCalculator calc = new OverrideOrderCalculator(env, overrides); List order = calc.getOrderedVariableNames(); - assertEquals(Sets.newHashSet("A", "B", "C"), new HashSet(order)); + assertEquals(Arrays.asList("B", "A", "C"), order.subList(0, 3)); + assertEquals(Sets.newHashSet("E", "D"), new HashSet(order.subList(3, order.size()))); } }