From c496efc61a3ef95f851e2ee31d119a9aa2350da0 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 27 Dec 2017 11:20:27 -0600 Subject: [PATCH 1/6] gps: apply transitive constraints --- gps/identifier.go | 2 ++ gps/solve_bimodal_test.go | 20 ++++++++++++++++++++ gps/solver.go | 18 ++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/gps/identifier.go b/gps/identifier.go index cf3ca23513..a7481bc803 100644 --- a/gps/identifier.go +++ b/gps/identifier.go @@ -215,6 +215,8 @@ type completeDep struct { workingConstraint // The specific packages required from the ProjectDep pl []string + // Flags the constraint as being defined on an indirect/transitive dependency + isTransitive bool } // dependency represents an incomplete edge in the depgraph. It has a diff --git a/gps/solve_bimodal_test.go b/gps/solve_bimodal_test.go index 7dfa756279..1174160512 100644 --- a/gps/solve_bimodal_test.go +++ b/gps/solve_bimodal_test.go @@ -132,6 +132,26 @@ var bimodalFixtures = map[string]bimodalFixture{ "b 1.0.0", ), }, + "transitive constraint": { + ds: []depspec{ + dsp(mkDepspec("root 1.0.0", "foo 1.0.0"), + pkg("root", "foo"), + ), + dsp(mkDepspec("foo 1.0.0", "bar 1.0.0", "baz =1.0.0"), + pkg("foo", "bar"), + ), + dsp(mkDepspec("bar 1.0.0", "baz >=1.0.0"), + pkg("bar", "baz"), + ), + dsp(mkDepspec("baz 1.0.1"), pkg("baz")), + dsp(mkDepspec("baz 1.0.0"), pkg("baz")), + }, + r: mksolution( + "foo 1.0.0", + "bar 1.0.0", + "baz 1.0.0", + ), + }, // Constraints apply only if the project that declares them has a // reachable import "constraints activated by import": { diff --git a/gps/solver.go b/gps/solver.go index bede9d53b4..71a4adc274 100644 --- a/gps/solver.go +++ b/gps/solver.go @@ -645,6 +645,13 @@ func (s *solver) selectRoot() error { } s.sel.pushDep(dependency{depender: awp.a, dep: dep}) + + if dep.isTransitive { + // Do not add transitive dependencies to the queue immediately, + // instead wait until they are directly used + continue + } + // Add all to unselected queue heap.Push(s.unsel, bimodalIdentifier{id: dep.Ident, pl: dep.pl, fromRoot: true}) } @@ -792,6 +799,17 @@ func (s *solver) intersectConstraintsWithImports(deps []workingConstraint, reach } } + // Include transitive constraints, flagging them as transitive for special handling later on + for _, wc := range deps { + root := wc.Ident.ProjectRoot + if _, ok := dmap[root]; !ok { + dmap[root] = completeDep{ + workingConstraint: wc, // TODO(carolynvs): deal with overrides and package prefix-foo (not sure if that's needed?) + isTransitive: true, + } + } + } + // Dump all the deps from the map into the expected return slice cdeps := make([]completeDep, 0, len(dmap)) for _, cdep := range dmap { From 00ede102e8a9aaf96df10288fb266085eae4495a Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 27 Dec 2017 10:37:05 -0600 Subject: [PATCH 2/6] Test dep ensure with a transitive constraint --- .../ensure/transitive-constraint/README.md | 5 +++++ .../transitive-constraint/final/Gopkg.lock | 21 +++++++++++++++++++ .../transitive-constraint/final/Gopkg.toml | 7 +++++++ .../transitive-constraint/initial/Gopkg.toml | 7 +++++++ .../ensure/transitive-constraint/initial/a.go | 15 +++++++++++++ .../transitive-constraint/testcase.json | 9 ++++++++ 6 files changed, 64 insertions(+) create mode 100644 cmd/dep/testdata/harness_tests/ensure/transitive-constraint/README.md create mode 100644 cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.lock create mode 100644 cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.toml create mode 100644 cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/Gopkg.toml create mode 100644 cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/a.go create mode 100644 cmd/dep/testdata/harness_tests/ensure/transitive-constraint/testcase.json diff --git a/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/README.md b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/README.md new file mode 100644 index 0000000000..f54848a7a4 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/README.md @@ -0,0 +1,5 @@ +# Ensure - Transitive Constraint + +[github.com/carolynvs/deptest-transcons-c](github.com/carolynvs/deptest-transcons-c) +has a bug in the latest release. I am a library and need to define a constraint +so that consumers of my library don't pull in the bad version of C. diff --git a/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.lock b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.lock new file mode 100644 index 0000000000..9b7f01d789 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.lock @@ -0,0 +1,21 @@ +# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. + + +[[projects]] + name = "github.com/carolynvs/deptest-transcons-b" + packages = ["."] + revision = "bc9969bda2dd54b67470c2f199b14d2665c0f7a1" + version = "v1.0.0" + +[[projects]] + name = "github.com/carolynvs/deptest-transcons-c" + packages = ["."] + revision = "24ede1a3d5a6a036793bfcadf8802f60148adf90" + version = "v1.0.0" + +[solve-meta] + analyzer-name = "dep" + analyzer-version = 1 + inputs-digest = "bd219aa442b67cb568bd608a1c39644db8835170c8c5b83bfd3e0f8f0caf07f3" + solver-name = "gps-cdcl" + solver-version = 1 diff --git a/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.toml b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.toml new file mode 100644 index 0000000000..a7a1930daa --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.toml @@ -0,0 +1,7 @@ +[[constraint]] + name = "github.com/carolynvs/deptest-transcons-b" + version = "1.0.0" + +[[constraint]] + name = "github.com/carolynvs/deptest-transcons-c" + version = "<1.0.1" diff --git a/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/Gopkg.toml b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/Gopkg.toml new file mode 100644 index 0000000000..a7a1930daa --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/Gopkg.toml @@ -0,0 +1,7 @@ +[[constraint]] + name = "github.com/carolynvs/deptest-transcons-b" + version = "1.0.0" + +[[constraint]] + name = "github.com/carolynvs/deptest-transcons-c" + version = "<1.0.1" diff --git a/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/a.go b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/a.go new file mode 100644 index 0000000000..49c38a5558 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/a.go @@ -0,0 +1,15 @@ +// Copyright 2016 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package a + +import ( + "fmt" + "github.com/carolynvs/deptest-transcons-b" +) + +func A() { + fmt.Println("a did a thing") + b.B() +} diff --git a/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/testcase.json b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/testcase.json new file mode 100644 index 0000000000..f9b242b288 --- /dev/null +++ b/cmd/dep/testdata/harness_tests/ensure/transitive-constraint/testcase.json @@ -0,0 +1,9 @@ +{ + "commands": [ + ["ensure"] + ], + "vendor-final": [ + "github.com/carolynvs/deptest-transcons-b", + "github.com/carolynvs/deptest-transcons-c" + ] +} From e075a3f78f3da906feee4ff94d50c9d046704ca4 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 27 Dec 2017 11:38:27 -0600 Subject: [PATCH 3/6] Test dep init with a transitive constraint --- .../init/transitive-constraint/README.md | 7 +++++ .../transitive-constraint/final/Gopkg.lock | 27 +++++++++++++++++++ .../transitive-constraint/final/Gopkg.toml | 4 +++ .../transitive-constraint/initial/main.go | 15 +++++++++++ .../init/transitive-constraint/testcase.json | 10 +++++++ 5 files changed, 63 insertions(+) create mode 100644 cmd/dep/testdata/harness_tests/init/transitive-constraint/README.md create mode 100644 cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.lock create mode 100644 cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.toml create mode 100644 cmd/dep/testdata/harness_tests/init/transitive-constraint/initial/main.go create mode 100644 cmd/dep/testdata/harness_tests/init/transitive-constraint/testcase.json diff --git a/cmd/dep/testdata/harness_tests/init/transitive-constraint/README.md b/cmd/dep/testdata/harness_tests/init/transitive-constraint/README.md new file mode 100644 index 0000000000..30fa4fd9da --- /dev/null +++ b/cmd/dep/testdata/harness_tests/init/transitive-constraint/README.md @@ -0,0 +1,7 @@ +# Init - Transitive Constraint + +[C](github.com/carolynvs/deptest-transcons-c) +has a bug in the latest release. I am an end-user of [A](github.com/carolynvs/deptest-transcons-a) +which transitively depends on C. A has a constraint on C which avoids a bad release of C. +End-users like me should be able to use A, and have `dep init` pick a version of C +that doesn't have the bug, without manually adding overrides. diff --git a/cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.lock b/cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.lock new file mode 100644 index 0000000000..92959cf74a --- /dev/null +++ b/cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.lock @@ -0,0 +1,27 @@ +# This file is autogenerated, do not edit; changes may be undone by the next 'dep ensure'. + + +[[projects]] + name = "github.com/carolynvs/deptest-transcons-a" + packages = ["."] + revision = "750587f3002d87affe4971e0a0f1652272633e76" + version = "v1.0.0" + +[[projects]] + name = "github.com/carolynvs/deptest-transcons-b" + packages = ["."] + revision = "bc9969bda2dd54b67470c2f199b14d2665c0f7a1" + version = "v1.0.0" + +[[projects]] + name = "github.com/carolynvs/deptest-transcons-c" + packages = ["."] + revision = "24ede1a3d5a6a036793bfcadf8802f60148adf90" + version = "v1.0.0" + +[solve-meta] + analyzer-name = "dep" + analyzer-version = 1 + inputs-digest = "8d17577c18791655d6aa0f6fcb1034e51f9e47f57b55ce7091e077a1c4dc2da2" + solver-name = "gps-cdcl" + solver-version = 1 diff --git a/cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.toml b/cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.toml new file mode 100644 index 0000000000..f904aeebfc --- /dev/null +++ b/cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.toml @@ -0,0 +1,4 @@ + +[[constraint]] + name = "github.com/carolynvs/deptest-transcons-a" + version = "1.0.0" diff --git a/cmd/dep/testdata/harness_tests/init/transitive-constraint/initial/main.go b/cmd/dep/testdata/harness_tests/init/transitive-constraint/initial/main.go new file mode 100644 index 0000000000..e4bd0d335f --- /dev/null +++ b/cmd/dep/testdata/harness_tests/init/transitive-constraint/initial/main.go @@ -0,0 +1,15 @@ +// Copyright 2016 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import ( + "fmt" + "github.com/carolynvs/deptest-transcons-a" +) + +func main () { + fmt.Println("Stand back, I'm gonna do a thing!") + a.A() +} diff --git a/cmd/dep/testdata/harness_tests/init/transitive-constraint/testcase.json b/cmd/dep/testdata/harness_tests/init/transitive-constraint/testcase.json new file mode 100644 index 0000000000..d87ecde5ad --- /dev/null +++ b/cmd/dep/testdata/harness_tests/init/transitive-constraint/testcase.json @@ -0,0 +1,10 @@ +{ + "commands": [ + ["init", "--no-examples"] + ], + "vendor-final": [ + "github.com/carolynvs/deptest-transcons-a", + "github.com/carolynvs/deptest-transcons-b", + "github.com/carolynvs/deptest-transcons-c" + ] +} From 5b8fdcb8645cf7cd9466b7a9cecf793dd79ee6ff Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 27 Dec 2017 16:12:38 -0600 Subject: [PATCH 4/6] Failing test for unused transitive constraint --- gps/solve_bimodal_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/gps/solve_bimodal_test.go b/gps/solve_bimodal_test.go index 1174160512..f2f5948282 100644 --- a/gps/solve_bimodal_test.go +++ b/gps/solve_bimodal_test.go @@ -152,6 +152,22 @@ var bimodalFixtures = map[string]bimodalFixture{ "baz 1.0.0", ), }, + "unused transitive constraint": { + ds: []depspec{ + dsp(mkDepspec("root 1.0.0", "foo 1.0.0"), + pkg("root", "foo", "baz"), + ), + dsp(mkDepspec("foo 1.0.0", "baz =1.0.0"), + pkg("foo"), + ), + dsp(mkDepspec("baz 1.0.1"), pkg("baz")), + dsp(mkDepspec("baz 1.0.0"), pkg("baz")), + }, + r: mksolution( + "foo 1.0.0", + "baz 1.0.1", + ), + }, // Constraints apply only if the project that declares them has a // reachable import "constraints activated by import": { From 0b71dd2444bd2096caf773334e1eb99bb5e42ac8 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Wed, 27 Dec 2017 18:51:01 -0600 Subject: [PATCH 5/6] NEEDS REVIEW: DO NOT MERGE This is a prototype for keeping track of the path through the selection process to a project. It is used to help dep ignore "stale" transitive constraints: constraints that when created applied to a descendent but should no longer apply now that that the project has moved to another location in the dependency graph. Questions: * I put bmi on atomWithPackages because it already had a bmi method to recreate the original bmi. Not sure if it's safe to live there considering the comments on the original bmi function about avoiding copys of the package list. So it may need to shift elsewhere or path should be be split out from the bmi struct so that it can be attached directly to an atom. * Is a non-bimodal solve ever a possibility in dep (outside of the unit tests?). I think we could drop "dep.isTransitive" in favor of using bmi.path exclusively but the unit tests have a non-bimodal set of tests that cause this to fail. --- gps/identifier.go | 20 +++--------- gps/rootdata.go | 4 +-- gps/satisfy.go | 43 +++++++++++++------------- gps/selection.go | 27 +++++++++++++++-- gps/solve_basic_test.go | 5 +-- gps/solve_bimodal_test.go | 2 +- gps/solve_failures.go | 52 +++++++++++++++---------------- gps/solver.go | 64 ++++++++++++++++++++++----------------- gps/trace.go | 4 +-- 9 files changed, 121 insertions(+), 100 deletions(-) diff --git a/gps/identifier.go b/gps/identifier.go index a7481bc803..093eb852d2 100644 --- a/gps/identifier.go +++ b/gps/identifier.go @@ -177,6 +177,8 @@ type bimodalIdentifier struct { prefv Version // Indicates that the bmi came from the root project originally fromRoot bool + // The path to the atom in the graph, e.g. root -> foo -> bar + path []atom } type atom struct { @@ -190,20 +192,8 @@ var nilpa = atom{ } type atomWithPackages struct { - a atom - pl []string -} - -// bmi converts an atomWithPackages into a bimodalIdentifier. -// -// This is mostly intended for (read-only) trace use, so the package list slice -// is not copied. It is the callers responsibility to not modify the pl slice, -// lest that backpropagate and cause inconsistencies. -func (awp atomWithPackages) bmi() bimodalIdentifier { - return bimodalIdentifier{ - id: awp.a.id, - pl: awp.pl, - } + a atom + bmi bimodalIdentifier } // completeDep (name hopefully to change) provides the whole picture of a @@ -223,6 +213,6 @@ type completeDep struct { // fully-realized atom as the depender (the tail/source of the edge), and a set // of requirements that any atom to be attached at the head/target must satisfy. type dependency struct { - depender atom + depender atomWithPackages dep completeDep } diff --git a/gps/rootdata.go b/gps/rootdata.go index ee78bdf265..20550d7ca6 100644 --- a/gps/rootdata.go +++ b/gps/rootdata.go @@ -198,7 +198,7 @@ func (rd rootdata) rootAtom() atomWithPackages { sort.Strings(list) return atomWithPackages{ - a: a, - pl: list, + a: a, + bmi: bimodalIdentifier{id: a.id, pl: list}, } } diff --git a/gps/satisfy.go b/gps/satisfy.go index abac0ea7ef..555e2b6887 100644 --- a/gps/satisfy.go +++ b/gps/satisfy.go @@ -11,8 +11,7 @@ package gps // The goal is to determine whether selecting the atom would result in a state // where all the solver requirements are still satisfied. func (s *solver) check(a atomWithPackages, pkgonly bool) error { - pa := a.a - if nilpa == pa { + if nilpa == a.a { // This shouldn't be able to happen, but if it does, it unequivocally // indicates a logical bug somewhere, so blowing up is preferable panic("canary - checking version of empty ProjectAtom") @@ -30,7 +29,7 @@ func (s *solver) check(a atomWithPackages, pkgonly bool) error { // If we're pkgonly, then base atom was already determined to be allowable, // so we can skip the checkAtomAllowable step. if !pkgonly { - if err = s.checkAtomAllowable(pa); err != nil { + if err = s.checkAtomAllowable(a); err != nil { return err } } @@ -78,24 +77,24 @@ func (s *solver) check(a atomWithPackages, pkgonly bool) error { // checkAtomAllowable ensures that an atom itself is acceptable with respect to // the constraints established by the current solution. -func (s *solver) checkAtomAllowable(pa atom) error { - constraint := s.sel.getConstraint(pa.id) - if s.vUnify.matches(pa.id, constraint, pa.v) { +func (s *solver) checkAtomAllowable(awp atomWithPackages) error { + constraint := s.sel.getConstraint(awp.a.id, awp.bmi) + if s.vUnify.matches(awp.a.id, constraint, awp.a.v) { return nil } // TODO(sdboyer) collect constraint failure reason (wait...aren't we, below?) - deps := s.sel.getDependenciesOn(pa.id) + deps := s.sel.getDependenciesOn(awp.a.id) var failparent []dependency for _, dep := range deps { - if !s.vUnify.matches(pa.id, dep.dep.Constraint, pa.v) { - s.fail(dep.depender.id) + if !s.vUnify.matches(awp.a.id, dep.dep.Constraint, awp.a.v) { + s.fail(dep.depender.a.id) failparent = append(failparent, dep) } } err := &versionNotAllowedFailure{ - goal: pa, + goal: awp.a, failparent: failparent, c: constraint, } @@ -120,14 +119,14 @@ func (s *solver) checkRequiredPackagesExist(a atomWithPackages) error { for _, dep := range deps { for _, pkg := range dep.dep.pl { if errdep, seen := fp[pkg]; seen { - errdep.deppers = append(errdep.deppers, dep.depender) + errdep.deppers = append(errdep.deppers, dep.depender.a) fp[pkg] = errdep } else { perr, has := ptree.Packages[pkg] if !has || perr.Err != nil { fp[pkg] = errDeppers{ err: perr.Err, - deppers: []atom{dep.depender}, + deppers: []atom{dep.depender.a}, } } } @@ -147,7 +146,7 @@ func (s *solver) checkRequiredPackagesExist(a atomWithPackages) error { // given dep are valid with respect to existing constraints. func (s *solver) checkDepsConstraintsAllowable(a atomWithPackages, cdep completeDep) error { dep := cdep.workingConstraint - constraint := s.sel.getConstraint(dep.Ident) + constraint := s.sel.getConstraint(dep.Ident, a.bmi) // Ensure the constraint expressed by the dep has at least some possible // intersection with the intersection of existing constraints. if s.vUnify.matchesAny(dep.Ident, constraint, dep.Constraint) { @@ -160,7 +159,7 @@ func (s *solver) checkDepsConstraintsAllowable(a atomWithPackages, cdep complete var nofailsib []dependency for _, sibling := range siblings { if !s.vUnify.matchesAny(dep.Ident, sibling.dep.Constraint, dep.Constraint) { - s.fail(sibling.depender.id) + s.fail(sibling.depender.a.id) failsib = append(failsib, sibling) } else { nofailsib = append(nofailsib, sibling) @@ -168,7 +167,7 @@ func (s *solver) checkDepsConstraintsAllowable(a atomWithPackages, cdep complete } return &disjointConstraintFailure{ - goal: dependency{depender: a.a, dep: cdep}, + goal: dependency{depender: a, dep: cdep}, failsib: failsib, nofailsib: nofailsib, c: constraint, @@ -185,7 +184,7 @@ func (s *solver) checkDepsDisallowsSelected(a atomWithPackages, cdep completeDep s.fail(dep.Ident) return &constraintNotAllowedFailure{ - goal: dependency{depender: a.a, dep: cdep}, + goal: dependency{depender: a, dep: cdep}, v: selected.a.v, } } @@ -206,7 +205,7 @@ func (s *solver) checkIdentMatches(a atomWithPackages, cdep completeDep) error { // Fail all the other deps, as there's no way atom can ever be // compatible with them for _, d := range deps { - s.fail(d.depender.id) + s.fail(d.depender.a.id) } return &sourceMismatchFailure{ @@ -236,7 +235,7 @@ func (s *solver) checkRootCaseConflicts(a atomWithPackages, cdep completeDep) er curid, _ := s.sel.getIdentFor(current) deps := s.sel.getDependenciesOn(curid) for _, d := range deps { - s.fail(d.depender.id) + s.fail(d.depender.a.id) } // If a project has multiple packages that import each other, we treat that @@ -260,13 +259,13 @@ func (s *solver) checkRootCaseConflicts(a atomWithPackages, cdep completeDep) er if current == a.a.id.ProjectRoot { return &wrongCaseFailure{ correct: pr, - goal: dependency{depender: a.a, dep: cdep}, + goal: dependency{depender: a, dep: cdep}, badcase: deps, } } return &caseMismatchFailure{ - goal: dependency{depender: a.a, dep: cdep}, + goal: dependency{depender: a, dep: cdep}, current: current, failsib: deps, } @@ -289,7 +288,7 @@ func (s *solver) checkPackageImportsFromDepExist(a atomWithPackages, cdep comple e := &depHasProblemPackagesFailure{ goal: dependency{ - depender: a.a, + depender: a, dep: cdep, }, v: sel.a.v, @@ -329,7 +328,7 @@ func (s *solver) checkRevisionExists(a atomWithPackages, cdep completeDep) error return &nonexistentRevisionFailure{ goal: dependency{ - depender: a.a, + depender: a, dep: cdep, }, r: r, diff --git a/gps/selection.go b/gps/selection.go index a74c60ae8d..a3d76fdc62 100644 --- a/gps/selection.go +++ b/gps/selection.go @@ -4,6 +4,10 @@ package gps +import ( + "fmt" +) + type selection struct { // projects is a stack of the atoms that have currently been selected by the // solver. It can also be thought of as the vertex set of the current @@ -134,7 +138,7 @@ func (s *selection) getSelectedPackagesIn(id ProjectIdentifier) map[string]int { uniq := make(map[string]int) for _, p := range s.projects { if p.a.a.id.eq(id) { - for _, pkg := range p.a.pl { + for _, pkg := range p.a.bmi.pl { uniq[pkg] = uniq[pkg] + 1 } } @@ -143,12 +147,18 @@ func (s *selection) getSelectedPackagesIn(id ProjectIdentifier) map[string]int { return uniq } -func (s *selection) getConstraint(id ProjectIdentifier) Constraint { +func (s *selection) getConstraint(id ProjectIdentifier, bmi bimodalIdentifier) Constraint { deps, exists := s.deps[id.ProjectRoot] if !exists || len(deps) == 0 { return any } + // Enable quick lookup of where in the depgraph a constraint was defined + ancestors := map[ProjectRoot]bool{} + for _, ancestor := range bmi.path { + ancestors[ancestor.id.ProjectRoot] = true + } + // TODO(sdboyer) recomputing this sucks and is quite wasteful. Precompute/cache it // on changes to the constraint set, instead. @@ -159,6 +169,19 @@ func (s *selection) getConstraint(id ProjectIdentifier) Constraint { // Start with the open set var ret Constraint = any for _, dep := range deps { + if dep.dep.isTransitive { + // TODO(carolynvs): Remove print statements. It's just to help debug the transitive constraint prototype. + path := "" + for ancestor := range ancestors { + path += "/" + string(ancestor) + } + if _, isAncestor := ancestors[dep.depender.a.id.ProjectRoot]; !isAncestor { + fmt.Printf("*** Ignoring unreachable constraint %s on %s (path: %s) from %s ***\n", dep.dep.Constraint.String(), dep.dep.workingConstraint.Ident.ProjectRoot, path, dep.depender.a.id.ProjectRoot) + continue + } + fmt.Printf("*** Applying transitive constraint %s on %s (path: %s) from %s ***\n", dep.dep.Constraint.String(), dep.dep.workingConstraint.Ident.ProjectRoot, path, dep.depender.a.id.ProjectRoot) + } + ret = s.vu.intersect(id, ret, dep.dep.Constraint) } diff --git a/gps/solve_basic_test.go b/gps/solve_basic_test.go index 910b9d622b..d457aa4c0c 100644 --- a/gps/solve_basic_test.go +++ b/gps/solve_basic_test.go @@ -226,14 +226,15 @@ func mkDepspec(pi string, deps ...string) depspec { func mkDep(atom, pdep string, pl ...string) dependency { return dependency{ - depender: mkAtom(atom), + depender: atomWithPackages{a: mkAtom(atom)}, dep: mkCDep(pdep, pl...), } } func mkADep(atom, pdep string, c Constraint, pl ...string) dependency { return dependency{ - depender: mkAtom(atom), + // TODO(carolynvs): I don't think we need to set the bmi (specifically the path to the atom) in the fixture data. + depender: atomWithPackages{a: mkAtom(atom)}, dep: completeDep{ workingConstraint: workingConstraint{ Ident: ProjectIdentifier{ diff --git a/gps/solve_bimodal_test.go b/gps/solve_bimodal_test.go index f2f5948282..0ad9800971 100644 --- a/gps/solve_bimodal_test.go +++ b/gps/solve_bimodal_test.go @@ -132,7 +132,7 @@ var bimodalFixtures = map[string]bimodalFixture{ "b 1.0.0", ), }, - "transitive constraint": { + "used transitive constraint": { ds: []depspec{ dsp(mkDepspec("root 1.0.0", "foo 1.0.0"), pkg("root", "foo"), diff --git a/gps/solve_failures.go b/gps/solve_failures.go index 05daedd707..1026bdc139 100644 --- a/gps/solve_failures.go +++ b/gps/solve_failures.go @@ -78,16 +78,16 @@ type caseMismatchFailure struct { func (e *caseMismatchFailure) Error() string { if len(e.failsib) == 1 { str := "Could not introduce %s due to a case-only variation: it depends on %q, but %q was already established as the case variant for that project root by depender %s" - return fmt.Sprintf(str, a2vs(e.goal.depender), e.goal.dep.Ident.ProjectRoot, e.current, a2vs(e.failsib[0].depender)) + return fmt.Sprintf(str, a2vs(e.goal.depender.a), e.goal.dep.Ident.ProjectRoot, e.current, a2vs(e.failsib[0].depender.a)) } var buf bytes.Buffer str := "Could not introduce %s due to a case-only variation: it depends on %q, but %q was already established as the case variant for that project root by the following other dependers:\n" - fmt.Fprintf(&buf, str, a2vs(e.goal.depender), e.goal.dep.Ident.ProjectRoot, e.current) + fmt.Fprintf(&buf, str, a2vs(e.goal.depender.a), e.goal.dep.Ident.ProjectRoot, e.current) for _, c := range e.failsib { - fmt.Fprintf(&buf, "\t%s\n", a2vs(c.depender)) + fmt.Fprintf(&buf, "\t%s\n", a2vs(c.depender.a)) } return buf.String() @@ -97,7 +97,7 @@ func (e *caseMismatchFailure) traceString() string { var buf bytes.Buffer fmt.Fprintf(&buf, "case-only variation in dependency on %q; %q already established by:\n", e.goal.dep.Ident.ProjectRoot, e.current) for _, f := range e.failsib { - fmt.Fprintf(&buf, "%s\n", a2vs(f.depender)) + fmt.Fprintf(&buf, "%s\n", a2vs(f.depender.a)) } return buf.String() @@ -122,16 +122,16 @@ type wrongCaseFailure struct { func (e *wrongCaseFailure) Error() string { if len(e.badcase) == 1 { str := "Could not introduce %s; imports amongst its packages establish %q as the canonical casing for root, but %s tried to import it as %q" - return fmt.Sprintf(str, a2vs(e.goal.depender), e.correct, a2vs(e.badcase[0].depender), e.badcase[0].dep.Ident.ProjectRoot) + return fmt.Sprintf(str, a2vs(e.goal.depender.a), e.correct, a2vs(e.badcase[0].depender.a), e.badcase[0].dep.Ident.ProjectRoot) } var buf bytes.Buffer str := "Could not introduce %s; imports amongst its packages establish %q as the canonical casing for root, but the following projects tried to import it as %q" - fmt.Fprintf(&buf, str, a2vs(e.goal.depender), e.correct, e.badcase[0].dep.Ident.ProjectRoot) + fmt.Fprintf(&buf, str, a2vs(e.goal.depender.a), e.correct, e.badcase[0].dep.Ident.ProjectRoot) for _, c := range e.badcase { - fmt.Fprintf(&buf, "\t%s\n", a2vs(c.depender)) + fmt.Fprintf(&buf, "\t%s\n", a2vs(c.depender.a)) } return buf.String() @@ -141,7 +141,7 @@ func (e *wrongCaseFailure) traceString() string { var buf bytes.Buffer fmt.Fprintf(&buf, "internal imports establish %q as correct casing; %q was used by:\n", e.correct, e.goal.dep.Ident.ProjectRoot) for _, f := range e.badcase { - fmt.Fprintf(&buf, "%s\n", a2vs(f.depender)) + fmt.Fprintf(&buf, "%s\n", a2vs(f.depender.a)) } return buf.String() @@ -171,7 +171,7 @@ type disjointConstraintFailure struct { func (e *disjointConstraintFailure) Error() string { if len(e.failsib) == 1 { str := "Could not introduce %s, as it has a dependency on %s with constraint %s, which has no overlap with existing constraint %s from %s" - return fmt.Sprintf(str, a2vs(e.goal.depender), e.goal.dep.Ident, e.goal.dep.Constraint.String(), e.failsib[0].dep.Constraint.String(), a2vs(e.failsib[0].depender)) + return fmt.Sprintf(str, a2vs(e.goal.depender.a), e.goal.dep.Ident, e.goal.dep.Constraint.String(), e.failsib[0].dep.Constraint.String(), a2vs(e.failsib[0].depender.a)) } var buf bytes.Buffer @@ -181,16 +181,16 @@ func (e *disjointConstraintFailure) Error() string { sibs = e.failsib str := "Could not introduce %s, as it has a dependency on %s with constraint %s, which has no overlap with the following existing constraints:\n" - fmt.Fprintf(&buf, str, a2vs(e.goal.depender), e.goal.dep.Ident, e.goal.dep.Constraint.String()) + fmt.Fprintf(&buf, str, a2vs(e.goal.depender.a), e.goal.dep.Ident, e.goal.dep.Constraint.String()) } else { sibs = e.nofailsib str := "Could not introduce %s, as it has a dependency on %s with constraint %s, which does not overlap with the intersection of existing constraints from other currently selected packages:\n" - fmt.Fprintf(&buf, str, a2vs(e.goal.depender), e.goal.dep.Ident, e.goal.dep.Constraint.String()) + fmt.Fprintf(&buf, str, a2vs(e.goal.depender.a), e.goal.dep.Ident, e.goal.dep.Constraint.String()) } for _, c := range sibs { - fmt.Fprintf(&buf, "\t%s from %s\n", c.dep.Constraint.String(), a2vs(c.depender)) + fmt.Fprintf(&buf, "\t%s from %s\n", c.dep.Constraint.String(), a2vs(c.depender.a)) } return buf.String() @@ -204,7 +204,7 @@ func (e *disjointConstraintFailure) traceString() string { &buf, "%s from %s (no overlap)\n", f.dep.Constraint.String(), - a2vs(f.depender), + a2vs(f.depender.a), ) } for _, f := range e.nofailsib { @@ -212,7 +212,7 @@ func (e *disjointConstraintFailure) traceString() string { &buf, "%s from %s (some overlap)\n", f.dep.Constraint.String(), - a2vs(f.depender), + a2vs(f.depender.a), ) } @@ -234,7 +234,7 @@ type constraintNotAllowedFailure struct { func (e *constraintNotAllowedFailure) Error() string { return fmt.Sprintf( "Could not introduce %s, as it has a dependency on %s with constraint %s, which does not allow the currently selected version of %s", - a2vs(e.goal.depender), + a2vs(e.goal.depender.a), e.goal.dep.Ident, e.goal.dep.Constraint, e.v, @@ -244,7 +244,7 @@ func (e *constraintNotAllowedFailure) Error() string { func (e *constraintNotAllowedFailure) traceString() string { return fmt.Sprintf( "%s depends on %s with %s, but that's already selected at %s", - a2vs(e.goal.depender), + a2vs(e.goal.depender.a), e.goal.dep.Ident.ProjectRoot, e.goal.dep.Constraint, e.v, @@ -274,7 +274,7 @@ func (e *versionNotAllowedFailure) Error() string { "Could not introduce %s, as it is not allowed by constraint %s from project %s.", a2vs(e.goal), e.failparent[0].dep.Constraint.String(), - e.failparent[0].depender.id, + e.failparent[0].depender.a.id, ) } @@ -283,7 +283,7 @@ func (e *versionNotAllowedFailure) Error() string { fmt.Fprintf(&buf, "Could not introduce %s, as it is not allowed by constraints from the following projects:\n", a2vs(e.goal)) for _, f := range e.failparent { - fmt.Fprintf(&buf, "\t%s from %s\n", f.dep.Constraint.String(), a2vs(f.depender)) + fmt.Fprintf(&buf, "\t%s from %s\n", f.dep.Constraint.String(), a2vs(f.depender.a)) } return buf.String() @@ -294,7 +294,7 @@ func (e *versionNotAllowedFailure) traceString() string { fmt.Fprintf(&buf, "%s not allowed by constraint %s:\n", a2vs(e.goal), e.c.String()) for _, f := range e.failparent { - fmt.Fprintf(&buf, " %s from %s\n", f.dep.Constraint.String(), a2vs(f.depender)) + fmt.Fprintf(&buf, " %s from %s\n", f.dep.Constraint.String(), a2vs(f.depender.a)) } return buf.String() @@ -333,7 +333,7 @@ type sourceMismatchFailure struct { func (e *sourceMismatchFailure) Error() string { var cur []string for _, c := range e.sel { - cur = append(cur, string(c.depender.id.ProjectRoot)) + cur = append(cur, string(c.depender.a.id.ProjectRoot)) } str := "Could not introduce %s, as it depends on %s from %s, but %s is already marked as coming from %s by %s" @@ -346,7 +346,7 @@ func (e *sourceMismatchFailure) traceString() string { fmt.Fprintf(&buf, " %s from %s\n", e.mismatch, e.prob.id) for _, dep := range e.sel { - fmt.Fprintf(&buf, " %s from %s\n", e.current, dep.depender.id) + fmt.Fprintf(&buf, " %s from %s\n", e.current, dep.depender.a.id) } return buf.String() @@ -485,7 +485,7 @@ func (e *depHasProblemPackagesFailure) Error() string { return fmt.Sprintf( "Could not introduce %s, as it requires package %s from %s, but in version %s that package %s", - a2vs(e.goal.depender), + a2vs(e.goal.depender.a), pkg, e.goal.dep.Ident, e.v, @@ -496,7 +496,7 @@ func (e *depHasProblemPackagesFailure) Error() string { var buf bytes.Buffer fmt.Fprintf( &buf, "Could not introduce %s, as it requires problematic packages from %s (current version %s):", - a2vs(e.goal.depender), + a2vs(e.goal.depender.a), e.goal.dep.Ident, e.v, ) @@ -526,7 +526,7 @@ func (e *depHasProblemPackagesFailure) traceString() string { fmt.Fprintf( &buf, "%s depping on %s at %s has problem subpkg(s):", - a2vs(e.goal.depender), + a2vs(e.goal.depender.a), e.goal.dep.Ident, e.v, ) @@ -556,7 +556,7 @@ type nonexistentRevisionFailure struct { func (e *nonexistentRevisionFailure) Error() string { return fmt.Sprintf( "Could not introduce %s, as it requires %s at revision %s, but that revision does not exist", - a2vs(e.goal.depender), + a2vs(e.goal.depender.a), e.goal.dep.Ident, e.r, ) @@ -565,7 +565,7 @@ func (e *nonexistentRevisionFailure) Error() string { func (e *nonexistentRevisionFailure) traceString() string { return fmt.Sprintf( "%s wants missing rev %s of %s", - a2vs(e.goal.depender), + a2vs(e.goal.depender.a), e.r, e.goal.dep.Ident, ) diff --git a/gps/solver.go b/gps/solver.go index 71a4adc274..44126d13fc 100644 --- a/gps/solver.go +++ b/gps/solver.go @@ -538,7 +538,7 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error id: queue.id, v: queue.current(), }, - pl: bmi.pl, + bmi: bmi, } err = s.selectAtom(awp, false) s.mtr.pop() @@ -566,7 +566,7 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error id: bmi.id, v: awp.a.v, }, - pl: bmi.pl, + bmi: bmi, } s.traceCheckPkgs(bmi) @@ -610,7 +610,7 @@ func (s *solver) solve(ctx context.Context) (map[atom]map[string]struct{}, error projs[sel.a.a] = pm } - for _, path := range sel.a.pl { + for _, path := range sel.a.bmi.pl { pm[path] = struct{}{} } } @@ -644,7 +644,10 @@ func (s *solver) selectRoot() error { go s.b.SyncSourceFor(dep.Ident) } - s.sel.pushDep(dependency{depender: awp.a, dep: dep}) + s.sel.pushDep(dependency{ + depender: awp, + dep: dep, + }) if dep.isTransitive { // Do not add transitive dependencies to the queue immediately, @@ -653,7 +656,7 @@ func (s *solver) selectRoot() error { } // Add all to unselected queue - heap.Push(s.unsel, bimodalIdentifier{id: dep.Ident, pl: dep.pl, fromRoot: true}) + heap.Push(s.unsel, bimodalIdentifier{id: dep.Ident, pl: dep.pl, fromRoot: true, path: []atom{awp.a}}) } s.traceSelectRoot(s.rd.rpt, deps) @@ -684,7 +687,7 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com // Use maps to dedupe the unique internal and external packages. exmap, inmap := make(map[string]struct{}), make(map[string]struct{}) - for _, pkg := range a.pl { + for _, pkg := range a.bmi.pl { inmap[pkg] = struct{}{} for _, ipkg := range rm[pkg].Internal { inmap[ipkg] = struct{}{} @@ -694,8 +697,8 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com var pl []string // If lens are the same, then the map must have the same contents as the // slice; no need to build a new one. - if len(inmap) == len(a.pl) { - pl = a.pl + if len(inmap) == len(a.bmi.pl) { + pl = a.bmi.pl } else { pl = make([]string, 0, len(inmap)) for pkg := range inmap { @@ -706,7 +709,7 @@ func (s *solver) getImportsAndConstraintsOf(a atomWithPackages) ([]string, []com // Add to the list those packages that are reached by the packages // explicitly listed in the atom - for _, pkg := range a.pl { + for _, pkg := range a.bmi.pl { // Skip ignored packages if s.rd.ir.IsIgnored(pkg) { continue @@ -845,7 +848,7 @@ func (s *solver) createVersionQueue(bmi bimodalIdentifier) (*versionQueue, error var lockv Version if len(s.rd.rlm) > 0 { - lockv, err = s.getLockVersionIfValid(id) + lockv, err = s.getLockVersionIfValid(id, bmi) if err != nil { // Can only get an error here if an upgrade was expressly requested on // code that exists only in vendor @@ -862,11 +865,11 @@ func (s *solver) createVersionQueue(bmi bimodalIdentifier) (*versionQueue, error // TODO(sdboyer) nested loop; prime candidate for a cache somewhere for _, dep := range s.sel.getDependenciesOn(bmi.id) { // Skip the root, of course - if s.rd.isRoot(dep.depender.id.ProjectRoot) { + if s.rd.isRoot(dep.depender.a.id.ProjectRoot) { continue } - _, l, err := s.b.GetManifestAndLock(dep.depender.id, dep.depender.v, s.rd.an) + _, l, err := s.b.GetManifestAndLock(dep.depender.a.id, dep.depender.a.v, s.rd.an) if err != nil || l == nil { // err being non-nil really shouldn't be possible, but the lock // being nil is quite likely @@ -920,7 +923,7 @@ func (s *solver) createVersionQueue(bmi bimodalIdentifier) (*versionQueue, error // TODO(sdboyer) while this does work, it bypasses the interface-implied guarantees // of the version queue, and is therefore not a great strategy for API // coherency. Folding this in to a formal interface would be better. - if tc, ok := s.sel.getConstraint(bmi.id).(Revision); ok && q.pi[0] != tc { + if tc, ok := s.sel.getConstraint(bmi.id, bmi).(Revision); ok && q.pi[0] != tc { // We know this is the only thing that could possibly match, so put it // in at the front - if it isn't there already. // TODO(sdboyer) existence of the revision is guaranteed by checkRevisionExists(); restore that call. @@ -929,7 +932,7 @@ func (s *solver) createVersionQueue(bmi bimodalIdentifier) (*versionQueue, error // Having assembled the queue, search it for a valid version. s.traceCheckQueue(q, bmi, false, 1) - return q, s.findValidVersion(q, bmi.pl) + return q, s.findValidVersion(q, bmi) } // findValidVersion walks through a versionQueue until it finds a version that @@ -938,7 +941,7 @@ func (s *solver) createVersionQueue(bmi bimodalIdentifier) (*versionQueue, error // The satisfiability checks triggered from here are constrained to operate only // on those dependencies induced by the list of packages given in the second // parameter. -func (s *solver) findValidVersion(q *versionQueue, pl []string) error { +func (s *solver) findValidVersion(q *versionQueue, bmi bimodalIdentifier) error { if nil == q.current() { // this case should not be reachable, but reflects improper solver state // if it is, so panic immediately @@ -955,7 +958,7 @@ func (s *solver) findValidVersion(q *versionQueue, pl []string) error { id: q.id, v: cur, }, - pl: pl, + bmi: bmi, }, false) if err == nil { // we have a good version, can return safely @@ -972,7 +975,7 @@ func (s *solver) findValidVersion(q *versionQueue, pl []string) error { } } - s.fail(s.sel.getDependenciesOn(q.id)[0].depender.id) + s.fail(s.sel.getDependenciesOn(q.id)[0].depender.a.id) // Return a compound error of all the new errors encountered during this // attempt to find a new, valid version @@ -991,7 +994,7 @@ func (s *solver) findValidVersion(q *versionQueue, pl []string) error { // // If any of these three conditions are true (or if the id cannot be found in // the root lock), then no atom will be returned. -func (s *solver) getLockVersionIfValid(id ProjectIdentifier) (Version, error) { +func (s *solver) getLockVersionIfValid(id ProjectIdentifier, bmi bimodalIdentifier) (Version, error) { // If the project is specifically marked for changes, then don't look for a // locked version. if _, explicit := s.rd.chng[id.ProjectRoot]; explicit || s.rd.chngall { @@ -1022,7 +1025,7 @@ func (s *solver) getLockVersionIfValid(id ProjectIdentifier) (Version, error) { return nil, nil } - constraint := s.sel.getConstraint(id) + constraint := s.sel.getConstraint(id, bmi) v := lp.Version() if !constraint.Matches(v) { var found bool @@ -1100,7 +1103,7 @@ func (s *solver) backtrack(ctx context.Context) (bool, error) { } return false, err } - s.traceBacktrack(awp.bmi(), !proj) + s.traceBacktrack(awp.bmi, !proj) } } @@ -1120,7 +1123,7 @@ func (s *solver) backtrack(ctx context.Context) (bool, error) { } return false, err } - s.traceBacktrack(awp.bmi(), !proj) + s.traceBacktrack(awp.bmi, !proj) } if !q.id.eq(awp.a.id) { @@ -1131,8 +1134,8 @@ func (s *solver) backtrack(ctx context.Context) (bool, error) { // TODO(sdboyer) is it feasible to make available the failure reason here? if q.advance(nil) == nil && !q.isExhausted() { // Search for another acceptable version of this failed dep in its queue - s.traceCheckQueue(q, awp.bmi(), true, 0) - if s.findValidVersion(q, awp.pl) == nil { + s.traceCheckQueue(q, awp.bmi, true, 0) + if s.findValidVersion(q, awp.bmi) == nil { // Found one! Put it back on the selected queue and stop // backtracking @@ -1149,7 +1152,7 @@ func (s *solver) backtrack(ctx context.Context) (bool, error) { } } - s.traceBacktrack(awp.bmi(), false) + s.traceBacktrack(awp.bmi, false) // No solution found; continue backtracking after popping the queue // we just inspected off the list @@ -1269,7 +1272,7 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) error { s.mtr.push("select-atom") s.unsel.remove(bimodalIdentifier{ id: a.a.id, - pl: a.pl, + pl: a.bmi.pl, }) pl, deps, err := s.getImportsAndConstraintsOf(a) @@ -1283,7 +1286,7 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) error { } // Assign the new internal package list into the atom, then push it onto the // selection stack - a.pl = pl + a.bmi.pl = pl s.sel.pushSelection(a, pkgonly) // If this atom has a lock, pull it out so that we can potentially inject @@ -1325,7 +1328,11 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) error { go s.b.SyncSourceFor(dep.Ident) } - s.sel.pushDep(dependency{depender: a.a, dep: dep}) + s.sel.pushDep(dependency{ + depender: a, + dep: dep, + }) + // Go through all the packages introduced on this dep, selecting only // the ones where the only depper on them is what the preceding line just // pushed in. Then, put those into the unselected queue. @@ -1353,6 +1360,7 @@ func (s *solver) selectAtom(a atomWithPackages, pkgonly bool) error { // This puts in a preferred version if one's in the map, else // drops in the zero value (nil) prefv: lmap[dep.Ident], + path: append(a.bmi.path, a.a), } heap.Push(s.unsel, bmi) } @@ -1368,7 +1376,7 @@ func (s *solver) unselectLast() (atomWithPackages, bool, error) { s.mtr.push("unselect") defer s.mtr.pop() awp, first := s.sel.popSelection() - heap.Push(s.unsel, bimodalIdentifier{id: awp.a.id, pl: awp.pl}) + heap.Push(s.unsel, bimodalIdentifier{id: awp.a.id, pl: awp.bmi.pl, path: awp.bmi.path}) _, deps, err := s.getImportsAndConstraintsOf(awp) if err != nil { diff --git a/gps/trace.go b/gps/trace.go index 4c579d30aa..bfd0bf6b7a 100644 --- a/gps/trace.go +++ b/gps/trace.go @@ -140,9 +140,9 @@ func (s *solver) traceSelect(awp atomWithPackages, pkgonly bool) { var msg string if pkgonly { - msg = fmt.Sprintf("%s%s include %v more pkgs from %s", innerIndent, successChar, len(awp.pl), a2vs(awp.a)) + msg = fmt.Sprintf("%s%s include %v more pkgs from %s", innerIndent, successChar, len(awp.bmi.pl), a2vs(awp.a)) } else { - msg = fmt.Sprintf("%s select %s w/%v pkgs", successChar, a2vs(awp.a), len(awp.pl)) + msg = fmt.Sprintf("%s select %s w/%v pkgs", successChar, a2vs(awp.a), len(awp.bmi.pl)) } prefix := getprei(len(s.sel.projects) - 1) From d93117f2f3a0de232ce5ac14220933b00e90f654 Mon Sep 17 00:00:00 2001 From: Carolyn Van Slyck Date: Thu, 28 Dec 2017 10:19:49 -0600 Subject: [PATCH 6/6] Update exists (pre-PR) tests to pass One test relied upon constraints NOT being applied transitively. The other was named incorrectly, it was testing a non-root scenario but was named "transitive" which was misleading. --- gps/solve_bimodal_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gps/solve_bimodal_test.go b/gps/solve_bimodal_test.go index 0ad9800971..cc1996ffd1 100644 --- a/gps/solve_bimodal_test.go +++ b/gps/solve_bimodal_test.go @@ -188,12 +188,12 @@ var bimodalFixtures = map[string]bimodalFixture{ }, r: mksolution( "a 1.0.0", - "b 1.1.0", + "b 1.0.0", // Now that constraints can be applied transitively, the constraint from root applies ), }, // Constraints apply only if the project that declares them has a // reachable import - non-root - "constraints activated by import, transitive": { + "constraints activated by import, non-root": { ds: []depspec{ dsp(mkDepspec("root 0.0.0"), pkg("root", "root/foo", "b"),