Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Proposal: Allow transitive constraints #1489

Closed
wants to merge 6 commits into from
Closed

Proposal: Allow transitive constraints #1489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c496efc
gps: apply transitive constraints
carolynvs Dec 27, 2017
00ede10
Test dep ensure with a transitive constraint
carolynvs Dec 27, 2017
e075a3f
Test dep init with a transitive constraint
carolynvs Dec 27, 2017
5b8fdcb
Failing test for unused transitive constraint
carolynvs Dec 27, 2017
0b71dd2
NEEDS REVIEW: DO NOT MERGE
carolynvs Dec 28, 2017
d93117f
Update exists (pre-PR) tests to pass
carolynvs Dec 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/transitive-constraint/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
21 changes: 21 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/transitive-constraint/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
7 changes: 7 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
15 changes: 15 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/transitive-constraint/initial/a.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
}
9 changes: 9 additions & 0 deletions cmd/dep/testdata/harness_tests/ensure/transitive-constraint/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"commands": [
["ensure"]
],
"vendor-final": [
"github.com/carolynvs/deptest-transcons-b",
"github.com/carolynvs/deptest-transcons-c"
]
}
7 changes: 7 additions & 0 deletions cmd/dep/testdata/harness_tests/init/transitive-constraint/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
27 changes: 27 additions & 0 deletions cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions cmd/dep/testdata/harness_tests/init/transitive-constraint/final/Gopkg.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@

[[constraint]]
name = "github.com/carolynvs/deptest-transcons-a"
version = "1.0.0"
15 changes: 15 additions & 0 deletions cmd/dep/testdata/harness_tests/init/transitive-constraint/initial/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
}
10 changes: 10 additions & 0 deletions cmd/dep/testdata/harness_tests/init/transitive-constraint/testcase.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
]
}
22 changes: 7 additions & 15 deletions gps/identifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so here's where i think the problem is gonna be. This is effectively precaching the path to a given atom from the root - but there's no guarantee that it's the only possible path to the atom. That itself may not necessarily be a problem (i haven't thought it through in at the level of the purest white driven snow algorithm), but it likely will not work well in terms of the way we populate the unselected queue.

Have a look at selectAtom(). We only add a bmi to the unselected queue if it induces a new package requirement on the target atom. i believe that'll mean that it's possible for a transitive constraint on P to be ignored if project A that expresses it induces no new packages to be selected within P.

That may not be true, but it's not covered by the current set of tests. It also may be the case that this isn't true, but i'm reasoning that it is by relying on cached thoughts around the the restricted purpose of bimodalIdentifier. If that's the case, it's evidence for why the reuse of similarly-shaped types for different purposes inside the algorithm is not a great idea - it complicates the already-difficult task of reasoning about the algorithm.

}

type atom struct {
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't fully grokked yet, but the complexity of this area is sufficient that, unless there is copy-avoiding performance argument to the contrary, it is preferable to not reuse types even if they share many fields. instead, types should be created and clearly described for the specific purpose for which they are used.

}

// completeDep (name hopefully to change) provides the whole picture of a
Expand All @@ -215,12 +205,14 @@ 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
// 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
}
4 changes: 2 additions & 2 deletions gps/rootdata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
}
}
43 changes: 21 additions & 22 deletions gps/satisfy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
}
}
Expand Down Expand Up @@ -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,
}
Expand All @@ -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},
}
}
}
Expand All @@ -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) {
Expand All @@ -160,15 +159,15 @@ 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)
}
}

return &disjointConstraintFailure{
goal: dependency{depender: a.a, dep: cdep},
goal: dependency{depender: a, dep: cdep},
failsib: failsib,
nofailsib: nofailsib,
c: constraint,
Expand All @@ -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,
}
}
Expand All @@ -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{
Expand Down Expand Up @@ -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
Expand All @@ -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,
}
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
Loading