From 578a0f2bb009f8885154feec5dd1918dccb6faf8 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Tue, 6 Dec 2016 13:35:15 -0500 Subject: [PATCH 1/6] Add RequiredPackages() to RootManifest Also add rudimentary implementations and error handling in Prepare(). --- manifest.go | 24 +++++++++++++++++++++++- solver.go | 30 +++++++++++++++++++++++------- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/manifest.go b/manifest.go index 791f002..17fd4b3 100644 --- a/manifest.go +++ b/manifest.go @@ -43,7 +43,22 @@ type RootManifest interface { // paths can be within the root project, or part of other projects. Ignoring // a package means that both it and its (unique) imports will be disregarded // by all relevant solver operations. + // + // It is an error to include a package in both the ignored and required + // sets. IgnoredPackages() map[string]bool + + // RequiredPackages returns a set of import paths to require. These packages + // are required to be present in any solution. The list can include main + // packages. + // + // It is meaningless to specify packages that are within the + // PackageTree of the ProjectRoot (though not an error, because the + // RootManifest itself does not report a ProjectRoot). + // + // It is an error to include a package in both the ignored and required + // sets. + RequiredPackages() map[string]bool } // SimpleManifest is a helper for tools to enumerate manifest data. It's @@ -72,7 +87,7 @@ func (m SimpleManifest) TestDependencyConstraints() ProjectConstraints { // Also, for tests. type simpleRootManifest struct { c, tc, ovr ProjectConstraints - ig map[string]bool + ig, req map[string]bool } func (m simpleRootManifest) DependencyConstraints() ProjectConstraints { @@ -87,12 +102,16 @@ func (m simpleRootManifest) Overrides() ProjectConstraints { func (m simpleRootManifest) IgnoredPackages() map[string]bool { return m.ig } +func (m simpleRootManifest) RequiredPackages() map[string]bool { + return m.req +} func (m simpleRootManifest) dup() simpleRootManifest { m2 := simpleRootManifest{ c: make(ProjectConstraints, len(m.c)), tc: make(ProjectConstraints, len(m.tc)), ovr: make(ProjectConstraints, len(m.ovr)), ig: make(map[string]bool, len(m.ig)), + req: make(map[string]bool, len(m.req)), } for k, v := range m.c { @@ -107,6 +126,9 @@ func (m simpleRootManifest) dup() simpleRootManifest { for k, v := range m.ig { m2.ig[k] = v } + for k, v := range m.req { + m2.req[k] = v + } return m2 } diff --git a/solver.go b/solver.go index 272b2ea..4f9526b 100644 --- a/solver.go +++ b/solver.go @@ -128,10 +128,12 @@ type solver struct { // removal. unsel *unselected - // Map of packages to ignore. Derived by converting SolveParameters.Ignore - // into a map during solver prep - which also, nicely, deduplicates it. + // Map of packages to ignore. ig map[string]bool + // Map of packages to require. + req map[string]bool + // A stack of all the currently active versionQueues in the solver. The set // of projects represented here corresponds closely to what's in s.sel, // although s.sel will always contain the root project, and s.vqs never @@ -216,11 +218,29 @@ func Prepare(params SolveParameters, sm SourceManager) (Solver, error) { s := &solver{ params: params, ig: params.Manifest.IgnoredPackages(), + req: params.Manifest.RequiredPackages(), ovr: params.Manifest.Overrides(), tl: params.TraceLogger, rpt: params.RootPackageTree.dup(), } + if len(s.ig) != 0 { + var both []string + for pkg := range params.Manifest.RequiredPackages() { + if s.ig[pkg] { + both = append(both, pkg) + } + } + switch len(both) { + case 0: + break + case 1: + return nil, badOptsFailure(fmt.Sprintf("%q was given as both a required and ignored package", both[0])) + default: + return nil, badOptsFailure(fmt.Sprintf("multiple packages given as both required and ignored: %q", strings.Join(both, "\", \""))) + } + } + // Ensure the ignore and overrides maps are at least initialized if s.ig == nil { s.ig = make(map[string]bool) @@ -481,11 +501,7 @@ func (s *solver) selectRoot() error { // If we're looking for root's deps, get it from opts and local root // analysis, rather than having the sm do it mdeps := s.ovr.overrideAll(s.rm.DependencyConstraints().merge(s.rm.TestDependencyConstraints())) - - // Err is not possible at this point, as it could only come from - // listPackages(), which if we're here already succeeded for root reach := s.rpt.ExternalReach(true, true, s.ig).ListExternalImports() - deps, err := s.intersectConstraintsWithImports(mdeps, reach) if err != nil { // TODO(sdboyer) this could well happen; handle it with a more graceful error @@ -661,7 +677,7 @@ func (s *solver) createVersionQueue(bmi bimodalIdentifier) (*versionQueue, error // Project exists only in vendor (and in some manifest somewhere) // TODO(sdboyer) mark this for special handling, somehow? } else { - return nil, fmt.Errorf("Project '%s' could not be located.", id) + return nil, fmt.Errorf("project '%s' could not be located", id) } } From 8f645c129a3f2abaa66cabbe774032eea8121485 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Wed, 7 Dec 2016 22:16:51 -0500 Subject: [PATCH 2/6] Add bimodal tests for requires logic --- solve_bimodal_test.go | 93 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/solve_bimodal_test.go b/solve_bimodal_test.go index 53b9fb5..7f6288e 100644 --- a/solve_bimodal_test.go +++ b/solve_bimodal_test.go @@ -748,6 +748,97 @@ var bimodalFixtures = map[string]bimodalFixture{ "bar from baz 1.0.0", ), }, + "require package": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0", "bar 1.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("baz 1.0.0"), + pkg("baz")), + }, + require: []string{"baz"}, + r: mksolution( + "foo 1.0.0", + "bar 1.0.0", + "baz 1.0.0", + ), + }, + "require subpackage": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0", "bar 1.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo", "bar")), + dsp(mkDepspec("bar 1.0.0"), + pkg("bar")), + dsp(mkDepspec("baz 1.0.0"), + pkg("baz", "baz/qux"), + pkg("baz/qux")), + }, + require: []string{"baz/qux"}, + r: mksolution( + "foo 1.0.0", + "bar 1.0.0", + mklp("baz 1.0.0", "baz/qux"), + ), + }, + "require impossible subpackage": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0", "baz 1.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0"), + pkg("foo")), + dsp(mkDepspec("baz 1.0.0"), + pkg("baz")), + dsp(mkDepspec("baz 2.0.0"), + pkg("baz", "baz/qux"), + pkg("baz/qux")), + }, + require: []string{"baz/qux"}, + fail: &noVersionError{ + pn: mkPI("baz"), + //fails: , // TODO new fail type for failed require + }, + }, + "require subpkg conflicts with other dep constraint": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0", "baz 1.0.0"), + pkg("foo", "baz")), + dsp(mkDepspec("baz 1.0.0"), + pkg("baz")), + dsp(mkDepspec("baz 2.0.0"), + pkg("baz", "baz/qux"), + pkg("baz/qux")), + }, + require: []string{"baz/qux"}, + fail: &noVersionError{ + pn: mkPI("baz"), + //fails: , // TODO new fail type for failed require + }, + }, + "require independent subpkg conflicts with other dep constraint": { + ds: []depspec{ + dsp(mkDepspec("root 0.0.0"), + pkg("root", "foo")), + dsp(mkDepspec("foo 1.0.0", "baz 1.0.0"), + pkg("foo", "baz")), + dsp(mkDepspec("baz 1.0.0"), + pkg("baz")), + dsp(mkDepspec("baz 2.0.0"), + pkg("baz"), + pkg("baz/qux")), + }, + require: []string{"baz/qux"}, + fail: &noVersionError{ + pn: mkPI("baz"), + //fails: , // TODO new fail type for failed require + }, + }, } // tpkg is a representation of a single package. It has its own import path, as @@ -783,6 +874,8 @@ type bimodalFixture struct { changeall bool // pkgs to ignore ignore []string + // pkgs to require + require []string } func (f bimodalFixture) name() string { From b6f7c444c25a8883a243bc9380e0cf2f310a1a2c Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 8 Dec 2016 09:45:55 -0500 Subject: [PATCH 3/6] Impl reqs on bimodalFixture Also touchup docs and remove project root prefix from mklp() call. --- solve_basic_test.go | 2 +- solve_bimodal_test.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/solve_basic_test.go b/solve_basic_test.go index 28374d7..38ce56e 100644 --- a/solve_basic_test.go +++ b/solve_basic_test.go @@ -294,7 +294,7 @@ func mkrevlock(pairs ...string) fixLock { return l } -// mksolution makes creates a map of project identifiers to their LockedProject +// mksolution creates a map of project identifiers to their LockedProject // result, which is sufficient to act as a solution fixture for the purposes of // most tests. // diff --git a/solve_bimodal_test.go b/solve_bimodal_test.go index 7f6288e..51774f9 100644 --- a/solve_bimodal_test.go +++ b/solve_bimodal_test.go @@ -782,7 +782,7 @@ var bimodalFixtures = map[string]bimodalFixture{ r: mksolution( "foo 1.0.0", "bar 1.0.0", - mklp("baz 1.0.0", "baz/qux"), + mklp("baz 1.0.0", "qux"), ), }, "require impossible subpackage": { @@ -900,10 +900,14 @@ func (f bimodalFixture) rootmanifest() RootManifest { tc: pcSliceToMap(f.ds[0].devdeps), ovr: f.ovr, ig: make(map[string]bool), + req: make(map[string]bool), } for _, ig := range f.ignore { m.ig[ig] = true } + for _, req := range f.require { + m.req[req] = true + } return m } From a1f1eb65be6749c8937d15f46911e8da36f24d14 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 8 Dec 2016 09:50:25 -0500 Subject: [PATCH 4/6] Implement requires logic in solver This was far easier than I expected. Requires really are the equivalent of imports, just taken from a manifest instead of static analysis. --- solver.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/solver.go b/solver.go index 4f9526b..75af783 100644 --- a/solver.go +++ b/solver.go @@ -502,6 +502,31 @@ func (s *solver) selectRoot() error { // analysis, rather than having the sm do it mdeps := s.ovr.overrideAll(s.rm.DependencyConstraints().merge(s.rm.TestDependencyConstraints())) reach := s.rpt.ExternalReach(true, true, s.ig).ListExternalImports() + + // If there are any requires, slide them into the reach list, as well. + if len(s.req) > 0 { + reqs := make([]string, 0, len(s.req)) + + // Make a map of both imported and required pkgs to skip, to avoid + // duplication. Technically, a slice would probably be faster (given + // small size and bounds check elimination), but this is a one-time op, + // so it doesn't matter. + skip := make(map[string]bool, len(s.req)) + for _, r := range reach { + if s.req[r] { + skip[r] = true + } + } + + for r := range s.req { + if !skip[r] { + reqs = append(reqs, r) + } + } + + reach = append(reach, reqs...) + } + deps, err := s.intersectConstraintsWithImports(mdeps, reach) if err != nil { // TODO(sdboyer) this could well happen; handle it with a more graceful error From ef31d916c83ca4fa4a7dec8100ae985d1102ca16 Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 8 Dec 2016 11:49:11 -0500 Subject: [PATCH 5/6] Add failure specifics to requires fixtures --- solve_bimodal_test.go | 75 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 3 deletions(-) diff --git a/solve_bimodal_test.go b/solve_bimodal_test.go index 51774f9..211887a 100644 --- a/solve_bimodal_test.go +++ b/solve_bimodal_test.go @@ -800,7 +800,30 @@ var bimodalFixtures = map[string]bimodalFixture{ require: []string{"baz/qux"}, fail: &noVersionError{ pn: mkPI("baz"), - //fails: , // TODO new fail type for failed require + fails: []failedVersion{ + { + v: NewVersion("2.0.0"), + f: &versionNotAllowedFailure{ + goal: mkAtom("baz 2.0.0"), + failparent: []dependency{mkDep("root", "baz 1.0.0", "baz/qux")}, + c: NewVersion("1.0.0"), + }, + }, + { + v: NewVersion("1.0.0"), + f: &checkeeHasProblemPackagesFailure{ + goal: mkAtom("baz 1.0.0"), + failpkg: map[string]errDeppers{ + "baz/qux": errDeppers{ + err: nil, // nil indicates package is missing + deppers: []atom{ + mkAtom("root"), + }, + }, + }, + }, + }, + }, }, }, "require subpkg conflicts with other dep constraint": { @@ -818,7 +841,30 @@ var bimodalFixtures = map[string]bimodalFixture{ require: []string{"baz/qux"}, fail: &noVersionError{ pn: mkPI("baz"), - //fails: , // TODO new fail type for failed require + fails: []failedVersion{ + { + v: NewVersion("2.0.0"), + f: &versionNotAllowedFailure{ + goal: mkAtom("baz 2.0.0"), + failparent: []dependency{mkDep("foo 1.0.0", "baz 1.0.0", "baz")}, + c: NewVersion("1.0.0"), + }, + }, + { + v: NewVersion("1.0.0"), + f: &checkeeHasProblemPackagesFailure{ + goal: mkAtom("baz 1.0.0"), + failpkg: map[string]errDeppers{ + "baz/qux": errDeppers{ + err: nil, // nil indicates package is missing + deppers: []atom{ + mkAtom("root"), + }, + }, + }, + }, + }, + }, }, }, "require independent subpkg conflicts with other dep constraint": { @@ -836,7 +882,30 @@ var bimodalFixtures = map[string]bimodalFixture{ require: []string{"baz/qux"}, fail: &noVersionError{ pn: mkPI("baz"), - //fails: , // TODO new fail type for failed require + fails: []failedVersion{ + { + v: NewVersion("2.0.0"), + f: &versionNotAllowedFailure{ + goal: mkAtom("baz 2.0.0"), + failparent: []dependency{mkDep("foo 1.0.0", "baz 1.0.0", "baz")}, + c: NewVersion("1.0.0"), + }, + }, + { + v: NewVersion("1.0.0"), + f: &checkeeHasProblemPackagesFailure{ + goal: mkAtom("baz 1.0.0"), + failpkg: map[string]errDeppers{ + "baz/qux": errDeppers{ + err: nil, // nil indicates package is missing + deppers: []atom{ + mkAtom("root"), + }, + }, + }, + }, + }, + }, }, }, } From 7adc45756cf872a2ffa6daecfeed13c6245fb3cb Mon Sep 17 00:00:00 2001 From: sam boyer Date: Thu, 8 Dec 2016 12:15:56 -0500 Subject: [PATCH 6/6] Tests for conflicting req/ignore in Prepare() --- solve_test.go | 24 ++++++++++++++++++++++++ solver.go | 2 +- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/solve_test.go b/solve_test.go index f6a0b7a..db315fb 100644 --- a/solve_test.go +++ b/solve_test.go @@ -283,6 +283,8 @@ func TestRootLockNoVersionPairMatching(t *testing.T) { fixtureSolveSimpleChecks(fix, res, err, t) } +// TestBadSolveOpts exercises the different possible inputs to a solver that can +// be determined as invalid in Prepare(), without any further work func TestBadSolveOpts(t *testing.T) { pn := strconv.FormatInt(rand.Int63(), 36) fix := basicFixtures["no dependencies"] @@ -354,6 +356,28 @@ func TestBadSolveOpts(t *testing.T) { } else if !strings.Contains(err.Error(), "foo, but without any non-zero properties") { t.Error("Prepare should have given error override with empty ProjectProperties, but gave:", err) } + + params.Manifest = simpleRootManifest{ + ig: map[string]bool{"foo": true}, + req: map[string]bool{"foo": true}, + } + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on pkg both ignored and required") + } else if !strings.Contains(err.Error(), "was given as both a required and ignored package") { + t.Error("Prepare should have given error with single ignore/require conflict error, but gave:", err) + } + + params.Manifest = simpleRootManifest{ + ig: map[string]bool{"foo": true, "bar": true}, + req: map[string]bool{"foo": true, "bar": true}, + } + _, err = Prepare(params, sm) + if err == nil { + t.Errorf("Should have errored on pkg both ignored and required") + } else if !strings.Contains(err.Error(), "multiple packages given as both required and ignored: foo, bar") { + t.Error("Prepare should have given error with multiple ignore/require conflict error, but gave:", err) + } params.Manifest = nil params.ToChange = []ProjectRoot{"foo"} diff --git a/solver.go b/solver.go index 75af783..008347f 100644 --- a/solver.go +++ b/solver.go @@ -237,7 +237,7 @@ func Prepare(params SolveParameters, sm SourceManager) (Solver, error) { case 1: return nil, badOptsFailure(fmt.Sprintf("%q was given as both a required and ignored package", both[0])) default: - return nil, badOptsFailure(fmt.Sprintf("multiple packages given as both required and ignored: %q", strings.Join(both, "\", \""))) + return nil, badOptsFailure(fmt.Sprintf("multiple packages given as both required and ignored: %s", strings.Join(both, ", "))) } }