Merge pull request #733 from D-Programming-Language/fix_issue

Fork: 0

johnpearcey / dub_jkp

Browse code Merge pull request #733 from D-Programming-Language/fix_issue_361-opt-dependencies Implement new optional dependency semantics. Fixes #361. master
commit c5935829500dac1b6a2e5aa0dd628a3a694ab75c 2 parents 2c13ad1 + ec8b3f1 Sönke Ludwig authored on 12 Feb 2016

Patch

Unified Split

Showing 16 changed files

                Ignore Space
               Show notes
              View
            
          
          75 ■■■■■
          source/dub/dependency.d
 		bool m_inclusiveB = true; // B comparison < (true) or <= (false)
		Version m_versB;
		Path m_path;
		bool m_optional = false;
		bool m_default = false;
	}
 
	// A Dependency, which matches every valid version.
	static @property any() { return Dependency(ANY_IDENT); }
		this(ANY_IDENT);
		m_path = path;
	}
 
	/// If set, overrides any version based dependency selection.
	@property void path(Path value) { m_path = value; }
	/// ditto
	@property Path path() const { return m_path; }
 
	/// Determines if the dependency is required or optional.
	@property bool optional() const { return m_optional; }
	/// ditto
	@property void optional(bool optional) { m_optional = optional; }
 
	/// Determines if an optional dependency should be chosen by default.
	@property bool default_() const { return m_default; }
	/// ditto
	@property void default_(bool value) { m_default = value; }
 
	/// Returns true $(I iff) the version range only matches a specific version.
	@property bool isExactVersion() const { return m_versA == m_versB; }
 
	/// Returns the exact version matched by the version range.
	@property Version version_() const {
		enforce(m_versA == m_versB, "Dependency "~versionString~" is no exact version.");
		return m_versA;
	}
 
	/// Returns a string representing the version range for the dependency.
	@property string versionString()
	const {
		string r;
 
			json = Json.emptyObject;
			json["version"] = this.versionString;
			if (!path.empty) json["path"] = path.toString();
			if (optional) json["optional"] = true;
			if (default_) json["default"] = true;
		}
		return json;
	}
 
				auto ver = verspec["version"].get!string;
				// Using the string to be able to specifiy a range of versions.
				dep = Dependency(ver);
			}
			if( auto po = "optional" in verspec ) {
				dep.optional = verspec.optional.get!bool;
			}
 
			if (auto po = "optional" in verspec) dep.optional = po.get!bool;
			if (auto po = "default" in verspec) dep.default_ = po.get!bool;
		} else {
			// canonical "package-id": "version"
			dep = Dependency(verspec.get!string);
		}
		Dependency parsed = fromJson(parseJsonString(`
		{
			"version": "2.0.0",
			"optional": true,
			"default": true,
			"path": "path/to/package"
		}
			`));
		Dependency d = Dependency.ANY; // supposed to ignore the version spec
		d.optional = true;
		d.default_ = true;
		d.path = Path("path/to/package");
		assert(d == parsed);
		// optional and path not checked by opEquals.
		assert(d.optional == parsed.optional);
		assert(d.default_ == parsed.default_);
		assert(d.path == parsed.path);
	}
 
	bool opEquals(in Dependency o)
	const {
		// TODO(mdondorff): Check if not comparing the path is correct for all clients.
		return o.m_inclusiveA == m_inclusiveA && o.m_inclusiveB == m_inclusiveB
			&& o.m_versA == m_versA && o.m_versB == m_versB
			&& o.m_optional == m_optional;
			&& o.m_optional == m_optional && o.m_default == m_default;
	}
 
	int opCmp(in Dependency o)
	const {
	bool valid() const {
		return m_versA <= m_versB && doCmp(m_inclusiveA && m_inclusiveB, m_versA, m_versB);
	}
 
	bool matchesAny() const {
		auto cmp = Dependency("*");
		cmp.optional = m_optional;
		cmp.default_ = m_default;
		return cmp == this;
	}
 
	bool matches(string vers) const { return matches(Version(vers)); }
	bool matches(const(Version) v) const { return matches(v); }
	bool matches(ref const(Version) v) const {
		if (this == ANY) return true;
		if (this.matchesAny) return true;
		//logDebug(" try match: %s with: %s", v, this);
		// Master only matches master
		if(m_versA.isBranch) {
			enforce(m_versA == m_versB);
 
	/// Merges to versions
	Dependency merge(ref const(Dependency) o)
	const {
		if (this == ANY) return o;
		if (o == ANY) return this;
		if (this.matchesAny) return o;
		if (o.matchesAny) return this;
		if (!this.valid || !o.valid) return INVALID;
		if (m_versA.isBranch != o.m_versA.isBranch) return INVALID;
		if (m_versB.isBranch != o.m_versB.isBranch) return INVALID;
		if (m_versA.isBranch) return m_versA == o.m_versA ? this : INVALID;
	a = Dependency.ANY;
	assert(!a.optional);
	assert(a.valid);
	assertThrown(a.version_);
	assert(a.matches(Version.MASTER));
	assert(a.matches(Version("1.0.0")));
	assert(a.matches(Version("0.0.1-pre")));
	b = Dependency(">=1.0.1");
	assert(b == a.merge(b));
	assert(b == b.merge(a));
 
	logDebug("Dependency Unittest sucess.");
	b = Dependency(Version.MASTER);
	assert(a.merge(b) == b);
	assert(b.merge(a) == b);
 
	a.optional = true;
	assert(a.matches(Version.MASTER));
	assert(a.matches(Version("1.0.0")));
	assert(a.matches(Version("0.0.1-pre")));
	b = Dependency(">=1.0.1");
	assert(b == a.merge(b));
	assert(b == b.merge(a));
	b = Dependency(Version.MASTER);
	assert(a.merge(b) == b);
	assert(b.merge(a) == b);
 
	logDebug("Dependency unittest sucess.");
}
 
unittest {
	assert(Dependency("~>1.0.4").versionString == "~>1.0.4");
 		bool m_inclusiveB = true; // B comparison < (true) or <= (false)
 		Version m_versB;
 		Path m_path;
 		bool m_optional = false;
+		bool m_default = false;
 	}
 	// A Dependency, which matches every valid version.
 	static @property any() { return Dependency(ANY_IDENT); }
 		this(ANY_IDENT);
 		m_path = path;
 	}
+	/// If set, overrides any version based dependency selection.
 	@property void path(Path value) { m_path = value; }
+	/// ditto
 	@property Path path() const { return m_path; }
+	/// Determines if the dependency is required or optional.
 	@property bool optional() const { return m_optional; }
+	/// ditto
 	@property void optional(bool optional) { m_optional = optional; }
+	/// Determines if an optional dependency should be chosen by default.
+	@property bool default_() const { return m_default; }
+	/// ditto
+	@property void default_(bool value) { m_default = value; }
+	/// Returns true $(I iff) the version range only matches a specific version.
 	@property bool isExactVersion() const { return m_versA == m_versB; }
+	/// Returns the exact version matched by the version range.
 	@property Version version_() const {
 		enforce(m_versA == m_versB, "Dependency "~versionString~" is no exact version.");
 		return m_versA;
 	}
+	/// Returns a string representing the version range for the dependency.
 	@property string versionString()
 	const {
 		string r;
 			json = Json.emptyObject;
 			json["version"] = this.versionString;
 			if (!path.empty) json["path"] = path.toString();
 			if (optional) json["optional"] = true;
+			if (default_) json["default"] = true;
 		}
 		return json;
 	}
 				auto ver = verspec["version"].get!string;
 				// Using the string to be able to specifiy a range of versions.
 				dep = Dependency(ver);
 			}
+			if( auto po = "optional" in verspec ) {
+				dep.optional = verspec.optional.get!bool;
+			}
+			if (auto po = "optional" in verspec) dep.optional = po.get!bool;
+			if (auto po = "default" in verspec) dep.default_ = po.get!bool;
 		} else {
 			// canonical "package-id": "version"
 			dep = Dependency(verspec.get!string);
 		}
 		Dependency parsed = fromJson(parseJsonString(`
 		{
 			"version": "2.0.0",
 			"optional": true,
+			"default": true,
 			"path": "path/to/package"
 		}
 			`));
 		Dependency d = Dependency.ANY; // supposed to ignore the version spec
 		d.optional = true;
+		d.default_ = true;
 		d.path = Path("path/to/package");
 		assert(d == parsed);
 		// optional and path not checked by opEquals.
 		assert(d.optional == parsed.optional);
+		assert(d.default_ == parsed.default_);
 		assert(d.path == parsed.path);
 	}
 	bool opEquals(in Dependency o)
 	const {
 		// TODO(mdondorff): Check if not comparing the path is correct for all clients.
 		return o.m_inclusiveA == m_inclusiveA && o.m_inclusiveB == m_inclusiveB
 			&& o.m_versA == m_versA && o.m_versB == m_versB
+			&& o.m_optional == m_optional;
+			&& o.m_optional == m_optional && o.m_default == m_default;
 	}
 	int opCmp(in Dependency o)
 	const {
 	bool valid() const {
 		return m_versA <= m_versB && doCmp(m_inclusiveA && m_inclusiveB, m_versA, m_versB);
 	}
+	bool matchesAny() const {
+		auto cmp = Dependency("*");
+		cmp.optional = m_optional;
+		cmp.default_ = m_default;
+		return cmp == this;
+	}
 	bool matches(string vers) const { return matches(Version(vers)); }
 	bool matches(const(Version) v) const { return matches(v); }
 	bool matches(ref const(Version) v) const {
+		if (this == ANY) return true;
+		if (this.matchesAny) return true;
 		//logDebug(" try match: %s with: %s", v, this);
 		// Master only matches master
 		if(m_versA.isBranch) {
 			enforce(m_versA == m_versB);
 	/// Merges to versions
 	Dependency merge(ref const(Dependency) o)
 	const {
+		if (this == ANY) return o;
+		if (o == ANY) return this;
+		if (this.matchesAny) return o;
+		if (o.matchesAny) return this;
 		if (!this.valid || !o.valid) return INVALID;
 		if (m_versA.isBranch != o.m_versA.isBranch) return INVALID;
 		if (m_versB.isBranch != o.m_versB.isBranch) return INVALID;
 		if (m_versA.isBranch) return m_versA == o.m_versA ? this : INVALID;
 	a = Dependency.ANY;
 	assert(!a.optional);
 	assert(a.valid);
 	assertThrown(a.version_);
+	assert(a.matches(Version.MASTER));
+	assert(a.matches(Version("1.0.0")));
+	assert(a.matches(Version("0.0.1-pre")));
 	b = Dependency(">=1.0.1");
 	assert(b == a.merge(b));
 	assert(b == b.merge(a));
+	logDebug("Dependency Unittest sucess.");
+	b = Dependency(Version.MASTER);
+	assert(a.merge(b) == b);
+	assert(b.merge(a) == b);
+	a.optional = true;
+	assert(a.matches(Version.MASTER));
+	assert(a.matches(Version("1.0.0")));
+	assert(a.matches(Version("0.0.1-pre")));
+	b = Dependency(">=1.0.1");
+	assert(b == a.merge(b));
+	assert(b == b.merge(a));
+	b = Dependency(Version.MASTER);
+	assert(a.merge(b) == b);
+	assert(b.merge(a) == b);
+	logDebug("Dependency unittest sucess.");
 }
 unittest {
 	assert(Dependency("~>1.0.4").versionString == "~>1.0.4");

                Ignore Space
               Show notes
              View
            
          
          137 ■■■■■
          source/dub/dependencyresolver.d
 class DependencyResolver(CONFIGS, CONFIG) {
	static struct TreeNodes {
		string pack;
		CONFIGS configs;
		DependencyType depType = DependencyType.required;
 
		hash_t toHash() const nothrow @trusted {
			size_t ret = typeid(string).getHash(&pack);
			ret ^= typeid(CONFIGS).getHash(&configs);
	}
 
	CONFIG[string] resolve(TreeNode root, bool throw_on_failure = true)
	{
		static string rootPackage(string p) {
			auto idx = indexOf(p, ":");
			if (idx < 0) return p;
			return p[0 .. idx];
		}
 
		auto root_base_pack = rootPackage(root.pack);
 
		// find all possible configurations of each possible dependency
		size_t[string] package_indices;
		string[size_t] package_names;
		CONFIG[][] all_configs;
		bool[string] required_deps;
		bool[TreeNode] visited;
		void findConfigsRec(TreeNode parent, bool parent_unique)
		{
			if (parent in visited) return;
 
			foreach (ch; getChildren(parent)) {
				auto basepack = rootPackage(ch.pack);
				auto pidx = all_configs.length;
 
				if (ch.depType == DependencyType.required) required_deps[ch.pack] = true;
 
				CONFIG[] configs;
				if (auto pi = basepack in package_indices) {
					pidx = *pi;
					configs = all_configs[*pi];
					if (basepack == root_base_pack) configs = [root.config];
					else configs = getAllConfigs(basepack);
					all_configs ~= configs;
					package_indices[basepack] = pidx;
					package_names[pidx] = basepack;
				}
 
				configs = getSpecificConfigs(ch) ~ configs;
				configs = getSpecificConfigs(basepack, ch) ~ configs;
 
				// eliminate configurations from which we know that they can't satisfy
				// the uniquely defined root dependencies (==version or ~branch style dependencies)
				if (parent_unique) configs = configs.filter!(c => matches(ch.configs, c)).array;
			}
		}
		findConfigsRec(root, true);
 
		// prepend an invalid configuration to denote an unchosen dependency
		// append an invalid configuration to denote an unchosen dependency
		// this is used to properly support optional dependencies (when
		// getChildren() returns no configurations for an optional dependency,
		// but getAllConfigs() has already provided an existing list of configs)
		foreach (ref cfgs; all_configs) cfgs = CONFIG.invalid ~ cfgs;
		foreach (i, ref cfgs; all_configs)
			if (cfgs.length == 0 || package_names[i] !in required_deps)
				cfgs = cfgs ~ CONFIG.invalid;
 
		logDebug("Configurations used for dependency resolution:");
		foreach (n, i; package_indices) logDebug("  %s (%s): %s", n, i, all_configs[i]);
		foreach (n, i; package_indices) logDebug("  %s (%s%s): %s", n, i, n in required_deps ? "" : ", optional", all_configs[i]);
 
		auto config_indices = new size_t[all_configs.length];
		config_indices[] = 0;
 
 
				// get the current config/version of the current dependency
				sizediff_t childidx = package_indices[basepack];
				if (all_configs[childidx] == [CONFIG.invalid]) {
					// ignore invalid optional dependencies
					if (ch.depType != DependencyType.required)
						continue;
 
					enforce(parentbase != root_base_pack, format("Root package %s contains reference to invalid package %s %s", parent.pack, ch.pack, ch.configs));
					// choose another parent config to avoid the invalid child
					if (parentidx > maxcpi) {
						error = format("Package %s contains invalid dependency %s", parent.pack, ch.pack);
					auto config = all_configs[childidx][config_indices[childidx]];
					auto chnode = TreeNode(ch.pack, config);
 
					if (config == CONFIG.invalid || !matches(ch.configs, config)) {
						// ignore missing optional dependencies
						if (config == CONFIG.invalid && ch.depType != DependencyType.required)
							continue;
 
						// if we are at the root level, we can safely skip the maxcpi computation and instead choose another childidx config
						if (parentbase == root_base_pack) {
							error = format("No match for dependency %s %s of %s", ch.pack, ch.configs, parent.pack);
							return childidx;
					if (all_configs[i].length) {
						auto cfg = all_configs[i][config_indices[i]];
						if (cfg != CONFIG.invalid) ret[p] = cfg;
					}
				purgeOptionalDependencies(root, ret);
				return ret;
			}
 
			// find the next combination of configurations
		}
	}
 
	protected abstract CONFIG[] getAllConfigs(string pack);
	protected abstract CONFIG[] getSpecificConfigs(TreeNodes nodes);
	protected abstract CONFIG[] getSpecificConfigs(string pack, TreeNodes nodes);
	protected abstract TreeNodes[] getChildren(TreeNode node);
	protected abstract bool matches(CONFIGS configs, CONFIG config);
 
	private void purgeOptionalDependencies(TreeNode root, ref CONFIG[string] configs)
	{
		bool[string] required;
 
		void markRecursively(TreeNode node)
		{
			if (node.pack in required) return;
			required[node.pack] = true;
			foreach (dep; getChildren(node).filter!(dep => dep.depType != DependencyType.optional))
				if (auto dp = rootPackage(dep.pack) in configs)
					markRecursively(TreeNode(dep.pack, *dp));
		}
 
		// recursively mark all required dependencies of the concrete dependency tree
		markRecursively(root);
 
		// remove all un-marked configurations
		foreach (p; configs.keys.dup)
			if (p !in required)
				configs.remove(p);
	}
}
 
enum DependencyType {
	required,
	optionalDefault,
	optional
}
 
private string rootPackage(string p)
{
	auto idx = indexOf(p, ":");
	if (idx < 0) return p;
	return p[0 .. idx];
}
 
 
unittest {
		alias value this;
		enum invalid = IntConfig(-1);
	}
	static IntConfig ic(int v) { return IntConfig(v); }
 
	static class TestResolver : DependencyResolver!(IntConfig[], IntConfig) {
	static struct IntConfigs {
		IntConfig[] configs;
		alias configs this;
	}
	static IntConfigs ics(IntConfig[] cfgs) { return IntConfigs(cfgs); }
 
	static class TestResolver : DependencyResolver!(IntConfigs, IntConfig) {
		private TreeNodes[][string] m_children;
		this(TreeNodes[][string] children) { m_children = children; }
		protected override IntConfig[] getAllConfigs(string pack) {
			auto ret = appender!(IntConfig[]);
			}
			ret.data.sort!"a>b"();
			return ret.data;
		}
		protected override IntConfig[] getSpecificConfigs(TreeNodes nodes) { return null; }
		protected override IntConfig[] getSpecificConfigs(string pack, TreeNodes nodes) { return null; }
		protected override TreeNodes[] getChildren(TreeNode node) { return m_children.get(node.pack ~ ":" ~ node.config.to!string(), null); }
		protected override bool matches(IntConfig[] configs, IntConfig config) { return configs.canFind(config); }
		protected override bool matches(IntConfigs configs, IntConfig config) { return configs.canFind(config); }
	}
 
	// properly back up if conflicts are detected along the way (d:2 vs d:1)
	with (TestResolver) {
		auto res = new TestResolver([
			"a:0": [TreeNodes("b", [ic(2), ic(1)]), TreeNodes("d", [ic(1)]), TreeNodes("e", [ic(2), ic(1)])],
			"b:1": [TreeNodes("c", [ic(2), ic(1)]), TreeNodes("d", [ic(1)])],
			"b:2": [TreeNodes("c", [ic(3), ic(2)]), TreeNodes("d", [ic(2), ic(1)])],
			"a:0": [TreeNodes("b", ics([ic(2), ic(1)])), TreeNodes("d", ics([ic(1)])), TreeNodes("e", ics([ic(2), ic(1)]))],
			"b:1": [TreeNodes("c", ics([ic(2), ic(1)])), TreeNodes("d", ics([ic(1)]))],
			"b:2": [TreeNodes("c", ics([ic(3), ic(2)])), TreeNodes("d", ics([ic(2), ic(1)]))],
			"c:1": [], "c:2": [], "c:3": [],
			"d:1": [], "d:2": [],
			"e:1": [], "e:2": [],
		]);
 
	// handle cyclic dependencies gracefully
	with (TestResolver) {
		auto res = new TestResolver([
			"a:0": [TreeNodes("b", [ic(1)])],
			"b:1": [TreeNodes("b", [ic(1)])]
			"a:0": [TreeNodes("b", ics([ic(1)]))],
			"b:1": [TreeNodes("b", ics([ic(1)]))]
		]);
		assert(res.resolve(TreeNode("a", ic(0))) == ["b":ic(1)]);
	}
 
	// don't choose optional dependencies by default
	with (TestResolver) {
		auto res = new TestResolver([
			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optional)],
			"b:1": []
		]);
		assert(res.resolve(TreeNode("a", ic(0))).length == 0, to!string(res.resolve(TreeNode("a", ic(0)))));
	}
 
	// choose default optional dependencies by default
	with (TestResolver) {
		auto res = new TestResolver([
			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optionalDefault)],
			"b:1": []
		]);
		assert(res.resolve(TreeNode("a", ic(0))) == ["b":ic(1)], to!string(res.resolve(TreeNode("a", ic(0)))));
	}
 
	// choose optional dependency if non-optional within the dependency tree
	with (TestResolver) {
		auto res = new TestResolver([
			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optional), TreeNodes("c", ics([ic(1)]))],
			"b:1": [],
			"c:1": [TreeNodes("b", ics([ic(1)]))]
		]);
		assert(res.resolve(TreeNode("a", ic(0))) == ["b":ic(1), "c":ic(1)], to!string(res.resolve(TreeNode("a", ic(0)))));
	}
 
	// don't choose optional dependency if non-optional outside of final dependency tree
	with (TestResolver) {
		auto res = new TestResolver([
			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optional)],
			"b:1": [],
			"preset:0": [TreeNodes("b", ics([ic(1)]))]
		]);
		assert(res.resolve(TreeNode("a", ic(0))).length == 0, to!string(res.resolve(TreeNode("a", ic(0)))));
	}
}
 class DependencyResolver(CONFIGS, CONFIG) {
 	static struct TreeNodes {
 		string pack;
 		CONFIGS configs;
+		DependencyType depType = DependencyType.required;
 		hash_t toHash() const nothrow @trusted {
 			size_t ret = typeid(string).getHash(&pack);
 			ret ^= typeid(CONFIGS).getHash(&configs);
 	}
 	CONFIG[string] resolve(TreeNode root, bool throw_on_failure = true)
 	{
-		static string rootPackage(string p) {
-			auto idx = indexOf(p, ":");
-			if (idx < 0) return p;
-			return p[0 .. idx];
-		}
 		auto root_base_pack = rootPackage(root.pack);
 		// find all possible configurations of each possible dependency
 		size_t[string] package_indices;
+		string[size_t] package_names;
 		CONFIG[][] all_configs;
+		bool[string] required_deps;
 		bool[TreeNode] visited;
 		void findConfigsRec(TreeNode parent, bool parent_unique)
 		{
 			if (parent in visited) return;
 			foreach (ch; getChildren(parent)) {
 				auto basepack = rootPackage(ch.pack);
 				auto pidx = all_configs.length;
+				if (ch.depType == DependencyType.required) required_deps[ch.pack] = true;
 				CONFIG[] configs;
 				if (auto pi = basepack in package_indices) {
 					pidx = *pi;
 					configs = all_configs[*pi];
 					if (basepack == root_base_pack) configs = [root.config];
 					else configs = getAllConfigs(basepack);
 					all_configs ~= configs;
 					package_indices[basepack] = pidx;
+					package_names[pidx] = basepack;
 				}
+				configs = getSpecificConfigs(ch) ~ configs;
+				configs = getSpecificConfigs(basepack, ch) ~ configs;
 				// eliminate configurations from which we know that they can't satisfy
 				// the uniquely defined root dependencies (==version or ~branch style dependencies)
 				if (parent_unique) configs = configs.filter!(c => matches(ch.configs, c)).array;
 			}
 		}
 		findConfigsRec(root, true);
+		// prepend an invalid configuration to denote an unchosen dependency
+		// append an invalid configuration to denote an unchosen dependency
 		// this is used to properly support optional dependencies (when
 		// getChildren() returns no configurations for an optional dependency,
 		// but getAllConfigs() has already provided an existing list of configs)
+		foreach (ref cfgs; all_configs) cfgs = CONFIG.invalid ~ cfgs;
+		foreach (i, ref cfgs; all_configs)
+			if (cfgs.length == 0 || package_names[i] !in required_deps)
+				cfgs = cfgs ~ CONFIG.invalid;
 		logDebug("Configurations used for dependency resolution:");
+		foreach (n, i; package_indices) logDebug("  %s (%s): %s", n, i, all_configs[i]);
+		foreach (n, i; package_indices) logDebug("  %s (%s%s): %s", n, i, n in required_deps ? "" : ", optional", all_configs[i]);
 		auto config_indices = new size_t[all_configs.length];
 		config_indices[] = 0;
 				// get the current config/version of the current dependency
 				sizediff_t childidx = package_indices[basepack];
 				if (all_configs[childidx] == [CONFIG.invalid]) {
+					// ignore invalid optional dependencies
+					if (ch.depType != DependencyType.required)
+						continue;
 					enforce(parentbase != root_base_pack, format("Root package %s contains reference to invalid package %s %s", parent.pack, ch.pack, ch.configs));
 					// choose another parent config to avoid the invalid child
 					if (parentidx > maxcpi) {
 						error = format("Package %s contains invalid dependency %s", parent.pack, ch.pack);
 					auto config = all_configs[childidx][config_indices[childidx]];
 					auto chnode = TreeNode(ch.pack, config);
 					if (config == CONFIG.invalid || !matches(ch.configs, config)) {
+						// ignore missing optional dependencies
+						if (config == CONFIG.invalid && ch.depType != DependencyType.required)
+							continue;
 						// if we are at the root level, we can safely skip the maxcpi computation and instead choose another childidx config
 						if (parentbase == root_base_pack) {
 							error = format("No match for dependency %s %s of %s", ch.pack, ch.configs, parent.pack);
 							return childidx;
 					if (all_configs[i].length) {
 						auto cfg = all_configs[i][config_indices[i]];
 						if (cfg != CONFIG.invalid) ret[p] = cfg;
 					}
+				purgeOptionalDependencies(root, ret);
 				return ret;
 			}
 			// find the next combination of configurations
 		}
 	}
 	protected abstract CONFIG[] getAllConfigs(string pack);
+	protected abstract CONFIG[] getSpecificConfigs(TreeNodes nodes);
+	protected abstract CONFIG[] getSpecificConfigs(string pack, TreeNodes nodes);
 	protected abstract TreeNodes[] getChildren(TreeNode node);
 	protected abstract bool matches(CONFIGS configs, CONFIG config);
+	private void purgeOptionalDependencies(TreeNode root, ref CONFIG[string] configs)
+	{
+		bool[string] required;
+		void markRecursively(TreeNode node)
+		{
+			if (node.pack in required) return;
+			required[node.pack] = true;
+			foreach (dep; getChildren(node).filter!(dep => dep.depType != DependencyType.optional))
+				if (auto dp = rootPackage(dep.pack) in configs)
+					markRecursively(TreeNode(dep.pack, *dp));
+		}
+		// recursively mark all required dependencies of the concrete dependency tree
+		markRecursively(root);
+		// remove all un-marked configurations
+		foreach (p; configs.keys.dup)
+			if (p !in required)
+				configs.remove(p);
+	}
+}
+enum DependencyType {
+	required,
+	optionalDefault,
+	optional
+}
+private string rootPackage(string p)
+{
+	auto idx = indexOf(p, ":");
+	if (idx < 0) return p;
+	return p[0 .. idx];
 }
 unittest {
 		alias value this;
 		enum invalid = IntConfig(-1);
 	}
 	static IntConfig ic(int v) { return IntConfig(v); }
+	static class TestResolver : DependencyResolver!(IntConfig[], IntConfig) {
+	static struct IntConfigs {
+		IntConfig[] configs;
+		alias configs this;
+	}
+	static IntConfigs ics(IntConfig[] cfgs) { return IntConfigs(cfgs); }
+	static class TestResolver : DependencyResolver!(IntConfigs, IntConfig) {
 		private TreeNodes[][string] m_children;
 		this(TreeNodes[][string] children) { m_children = children; }
 		protected override IntConfig[] getAllConfigs(string pack) {
 			auto ret = appender!(IntConfig[]);
 			}
 			ret.data.sort!"a>b"();
 			return ret.data;
 		}
+		protected override IntConfig[] getSpecificConfigs(TreeNodes nodes) { return null; }
+		protected override IntConfig[] getSpecificConfigs(string pack, TreeNodes nodes) { return null; }
 		protected override TreeNodes[] getChildren(TreeNode node) { return m_children.get(node.pack ~ ":" ~ node.config.to!string(), null); }
+		protected override bool matches(IntConfig[] configs, IntConfig config) { return configs.canFind(config); }
+		protected override bool matches(IntConfigs configs, IntConfig config) { return configs.canFind(config); }
 	}
 	// properly back up if conflicts are detected along the way (d:2 vs d:1)
 	with (TestResolver) {
 		auto res = new TestResolver([
+			"a:0": [TreeNodes("b", [ic(2), ic(1)]), TreeNodes("d", [ic(1)]), TreeNodes("e", [ic(2), ic(1)])],
+			"b:1": [TreeNodes("c", [ic(2), ic(1)]), TreeNodes("d", [ic(1)])],
+			"b:2": [TreeNodes("c", [ic(3), ic(2)]), TreeNodes("d", [ic(2), ic(1)])],
+			"a:0": [TreeNodes("b", ics([ic(2), ic(1)])), TreeNodes("d", ics([ic(1)])), TreeNodes("e", ics([ic(2), ic(1)]))],
+			"b:1": [TreeNodes("c", ics([ic(2), ic(1)])), TreeNodes("d", ics([ic(1)]))],
+			"b:2": [TreeNodes("c", ics([ic(3), ic(2)])), TreeNodes("d", ics([ic(2), ic(1)]))],
 			"c:1": [], "c:2": [], "c:3": [],
 			"d:1": [], "d:2": [],
 			"e:1": [], "e:2": [],
 		]);
 	// handle cyclic dependencies gracefully
 	with (TestResolver) {
 		auto res = new TestResolver([
+			"a:0": [TreeNodes("b", [ic(1)])],
+			"b:1": [TreeNodes("b", [ic(1)])]
+			"a:0": [TreeNodes("b", ics([ic(1)]))],
+			"b:1": [TreeNodes("b", ics([ic(1)]))]
 		]);
 		assert(res.resolve(TreeNode("a", ic(0))) == ["b":ic(1)]);
 	}
+	// don't choose optional dependencies by default
+	with (TestResolver) {
+		auto res = new TestResolver([
+			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optional)],
+			"b:1": []
+		]);
+		assert(res.resolve(TreeNode("a", ic(0))).length == 0, to!string(res.resolve(TreeNode("a", ic(0)))));
+	}
+	// choose default optional dependencies by default
+	with (TestResolver) {
+		auto res = new TestResolver([
+			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optionalDefault)],
+			"b:1": []
+		]);
+		assert(res.resolve(TreeNode("a", ic(0))) == ["b":ic(1)], to!string(res.resolve(TreeNode("a", ic(0)))));
+	}
+	// choose optional dependency if non-optional within the dependency tree
+	with (TestResolver) {
+		auto res = new TestResolver([
+			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optional), TreeNodes("c", ics([ic(1)]))],
+			"b:1": [],
+			"c:1": [TreeNodes("b", ics([ic(1)]))]
+		]);
+		assert(res.resolve(TreeNode("a", ic(0))) == ["b":ic(1), "c":ic(1)], to!string(res.resolve(TreeNode("a", ic(0)))));
+	}
+	// don't choose optional dependency if non-optional outside of final dependency tree
+	with (TestResolver) {
+		auto res = new TestResolver([
+			"a:0": [TreeNodes("b", ics([ic(1)]), DependencyType.optional)],
+			"b:1": [],
+			"preset:0": [TreeNodes("b", ics([ic(1)]))]
+		]);
+		assert(res.resolve(TreeNode("a", ic(0))).length == 0, to!string(res.resolve(TreeNode("a", ic(0)))));
+	}
 }

Ignore Space Show notes View source/dub/dub.d

Ignore Space Show notes View source/dub/project.d

Ignore Space Show notes View source/dub/recipe/sdl.d

Ignore Space Show notes View test/issue361-optional-deps.sh 0 → 100755

Ignore Space Show notes View test/issue361-optional-deps/.no_build 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/a/dub.sdl 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/a/src/a.d 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/b/dub.sdl 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/b/src/b.d 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/main1/dub.sdl 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/main1/src/main1.d 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/main2/dub.sdl 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/main2/dub.selections.json 0 → 100644

Ignore Space Show notes View test/issue361-optional-deps/main2/src/main2.d 0 → 100644

Show line notes below