diff --git a/source/dub/dependency.d b/source/dub/dependency.d index 4fa71ef..0ddf681 100644 --- a/source/dub/dependency.d +++ b/source/dub/dependency.d @@ -39,6 +39,7 @@ Version m_versB; Path m_path; bool m_optional = false; + bool m_default = false; } // A Dependency, which matches every valid version. @@ -66,17 +67,31 @@ 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; @@ -201,6 +216,7 @@ json["version"] = this.versionString; if (!path.empty) json["path"] = path.toString(); if (optional) json["optional"] = true; + if (default_) json["default"] = true; } return json; } @@ -228,9 +244,9 @@ // 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); @@ -244,15 +260,18 @@ { "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); } @@ -261,7 +280,7 @@ // 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) @@ -286,10 +305,17 @@ 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) { @@ -308,8 +334,8 @@ /// 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; @@ -477,11 +503,28 @@ 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)); + b = Dependency(Version.MASTER); + assert(a.merge(b) == b); + assert(b.merge(a) == b); - logDebug("Dependency Unittest sucess."); + 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 { diff --git a/source/dub/dependencyresolver.d b/source/dub/dependencyresolver.d index 9038184..3635e58 100644 --- a/source/dub/dependencyresolver.d +++ b/source/dub/dependencyresolver.d @@ -21,6 +21,7 @@ static struct TreeNodes { string pack; CONFIGS configs; + DependencyType depType = DependencyType.required; hash_t toHash() const nothrow @trusted { size_t ret = typeid(string).getHash(&pack); @@ -54,17 +55,13 @@ 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) { @@ -74,6 +71,9 @@ 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; @@ -83,9 +83,10 @@ 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) @@ -99,14 +100,16 @@ } 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; @@ -133,6 +136,10 @@ // 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) { @@ -145,6 +152,10 @@ 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); @@ -196,6 +207,7 @@ auto cfg = all_configs[i][config_indices[i]]; if (cfg != CONFIG.invalid) ret[p] = cfg; } + purgeOptionalDependencies(root, ret); return ret; } @@ -213,9 +225,44 @@ } 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]; } @@ -226,8 +273,13 @@ enum invalid = IntConfig(-1); } static IntConfig ic(int v) { return IntConfig(v); } + static struct IntConfigs { + IntConfig[] configs; + alias configs this; + } + static IntConfigs ics(IntConfig[] cfgs) { return IntConfigs(cfgs); } - static class TestResolver : DependencyResolver!(IntConfig[], IntConfig) { + static class TestResolver : DependencyResolver!(IntConfigs, IntConfig) { private TreeNodes[][string] m_children; this(TreeNodes[][string] children) { m_children = children; } protected override IntConfig[] getAllConfigs(string pack) { @@ -241,17 +293,17 @@ 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": [], @@ -262,9 +314,47 @@ // 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))))); + } } diff --git a/source/dub/dub.d b/source/dub/dub.d index a18857d..afdbd14 100644 --- a/source/dub/dub.d +++ b/source/dub/dub.d @@ -1040,16 +1040,24 @@ if (!(m_options & UpgradeOptions.preRelease)) versions = versions.filter!(v => !v.isPreRelease).array ~ versions.filter!(v => v.isPreRelease).array; + // filter out invalid/unreachable dependency specs + versions = versions.filter!((v) { + bool valid = getPackage(pack, Dependency(v)) !is null; + if (!valid) logDiagnostic("Excluding invalid dependency specification %s %s from dependency resolution process.", pack, v); + return valid; + }).array; + if (!versions.length) logDiagnostic("Nothing found for %s", pack); + else logDiagnostic("Return for %s: %s", pack, versions); auto ret = versions.map!(v => Dependency(v)).array; m_packageVersions[pack] = ret; return ret; } - protected override Dependency[] getSpecificConfigs(TreeNodes nodes) + protected override Dependency[] getSpecificConfigs(string pack, TreeNodes nodes) { - if (!nodes.configs.path.empty) return [nodes.configs]; + if (!nodes.configs.path.empty && getPackage(pack, nodes.configs)) return [nodes.configs]; else return null; } @@ -1084,11 +1092,22 @@ continue; } - if (dspec.optional && !m_dub.packageManager.getFirstPackage(dname)) - continue; - if (m_options & UpgradeOptions.upgrade || !m_selectedVersions || !m_selectedVersions.hasSelectedVersion(dbasename)) - ret ~= TreeNodes(dname, dspec.mapToPath(pack.path)); - else ret ~= TreeNodes(dname, m_selectedVersions.getSelectedVersion(dbasename)); + DependencyType dt; + if (dspec.optional) { + if (dspec.default_) dt = DependencyType.optionalDefault; + else dt = DependencyType.optional; + } else dt = DependencyType.required; + + if (m_options & UpgradeOptions.upgrade || !m_selectedVersions || !m_selectedVersions.hasSelectedVersion(dbasename)) { + // keep deselected dependencies deselected by default + if (m_selectedVersions && !m_selectedVersions.bare && dt == DependencyType.optionalDefault) + dt = DependencyType.optional; + ret ~= TreeNodes(dname, dspec.mapToPath(pack.path), dt); + } else { + // keep already selected optional dependencies if possible + if (dt == DependencyType.optional) dt = DependencyType.optionalDefault; + ret ~= TreeNodes(dname, m_selectedVersions.getSelectedVersion(dbasename), dt); + } } return ret.data; } diff --git a/source/dub/project.d b/source/dub/project.d index dfb75e4..bc5bcce 100644 --- a/source/dub/project.d +++ b/source/dub/project.d @@ -1230,6 +1230,7 @@ enum FileVersion = 1; Selected[string] m_selections; bool m_dirty = false; // has changes since last save + bool m_bare = true; } enum defaultFile = "dub.selections.json"; @@ -1247,12 +1248,16 @@ auto json = jsonFromFile(path); deserialize(json); m_dirty = false; + m_bare = false; } @property string[] selectedPackages() const { return m_selections.keys; } @property bool dirty() const { return m_dirty; } + /// Determine if this set of selections is empty and has not been saved to disk. + @property bool bare() const { return m_bare && !m_dirty; } + void clear() { m_selections = null; @@ -1310,6 +1315,7 @@ file.writePrettyJsonString(json); file.put('\n'); m_dirty = false; + m_bare = false; } static Json dependencyToJson(Dependency d) diff --git a/source/dub/recipe/sdl.d b/source/dub/recipe/sdl.d index 18e94a2..a9d68ee 100644 --- a/source/dub/recipe/sdl.d +++ b/source/dub/recipe/sdl.d @@ -185,6 +185,9 @@ if ("optional" in attrs) dep.optional = attrs["optional"][0].value.get!bool; + if ("default" in attrs) + dep.default_ = attrs["default"][0].value.get!bool; + bs.dependencies[pkg] = dep; } diff --git a/test/issue361-optional-deps.sh b/test/issue361-optional-deps.sh new file mode 100755 index 0000000..69b7f0f --- /dev/null +++ b/test/issue361-optional-deps.sh @@ -0,0 +1,26 @@ +#!/bin/sh + +cd ${CURR_DIR}/issue361-optional-deps +rm -rf a/.dub +rm -rf a/b/.dub +rm -rf main1/.dub +rm -rf main2/.dub +rm -f main1/dub.selections.json + +${DUB} build --bare --compiler=${DC} main1 || exit 1 +echo "{" > cmp.tmp +echo " \"fileVersion\": 1," >> cmp.tmp +echo " \"versions\": {" >> cmp.tmp +echo " \"b\": \"~master\"" >> cmp.tmp +echo " }" >> cmp.tmp +echo "}" >> cmp.tmp +diff cmp.tmp main1/dub.selections.json || exit 1 + +${DUB} build --bare --compiler=${DC} main2 || exit 1 +echo "{" > cmp.tmp +echo " \"fileVersion\": 1," >> cmp.tmp +echo " \"versions\": {" >> cmp.tmp +echo " \"a\": \"~master\"" >> cmp.tmp +echo " }" >> cmp.tmp +echo "}" >> cmp.tmp +diff cmp.tmp main2/dub.selections.json || exit 1 diff --git a/test/issue361-optional-deps/.no_build b/test/issue361-optional-deps/.no_build new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/test/issue361-optional-deps/.no_build diff --git a/test/issue361-optional-deps/a/dub.sdl b/test/issue361-optional-deps/a/dub.sdl new file mode 100644 index 0000000..5730427 --- /dev/null +++ b/test/issue361-optional-deps/a/dub.sdl @@ -0,0 +1 @@ +name "a" \ No newline at end of file diff --git a/test/issue361-optional-deps/a/src/a.d b/test/issue361-optional-deps/a/src/a.d new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/test/issue361-optional-deps/a/src/a.d diff --git a/test/issue361-optional-deps/b/dub.sdl b/test/issue361-optional-deps/b/dub.sdl new file mode 100644 index 0000000..c37c6fc --- /dev/null +++ b/test/issue361-optional-deps/b/dub.sdl @@ -0,0 +1 @@ +name "b" \ No newline at end of file diff --git a/test/issue361-optional-deps/b/src/b.d b/test/issue361-optional-deps/b/src/b.d new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/test/issue361-optional-deps/b/src/b.d diff --git a/test/issue361-optional-deps/main1/dub.sdl b/test/issue361-optional-deps/main1/dub.sdl new file mode 100644 index 0000000..49c30d7 --- /dev/null +++ b/test/issue361-optional-deps/main1/dub.sdl @@ -0,0 +1,3 @@ +name "main1" +dependency "a" version="*" optional=true +dependency "b" version="*" optional=true default=true \ No newline at end of file diff --git a/test/issue361-optional-deps/main1/src/main1.d b/test/issue361-optional-deps/main1/src/main1.d new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/test/issue361-optional-deps/main1/src/main1.d diff --git a/test/issue361-optional-deps/main2/dub.sdl b/test/issue361-optional-deps/main2/dub.sdl new file mode 100644 index 0000000..d098466 --- /dev/null +++ b/test/issue361-optional-deps/main2/dub.sdl @@ -0,0 +1,3 @@ +name "main2" +dependency "a" version="*" optional=true +dependency "b" version="*" optional=true default=true \ No newline at end of file diff --git a/test/issue361-optional-deps/main2/dub.selections.json b/test/issue361-optional-deps/main2/dub.selections.json new file mode 100644 index 0000000..633ce9c --- /dev/null +++ b/test/issue361-optional-deps/main2/dub.selections.json @@ -0,0 +1,6 @@ +{ + "fileVersion": 1, + "versions": { + "a": "~master" + } +} diff --git a/test/issue361-optional-deps/main2/src/main2.d b/test/issue361-optional-deps/main2/src/main2.d new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/test/issue361-optional-deps/main2/src/main2.d