Report when manifests contain ineffectual constraints

[goller]

Hey there... I may have found an issue regarding ensure at a revision using master.

After running a dep ensure the manifest revision changes but the lock does not.

Here is how to reproduce

$ go get github.com/influxdata/kapacitor

$ echo 'package main

import "github.com/influxdata/kapacitor/tick/ast"

func main() {
	ast.Parse("stream|from()|alert().slack()")
}' > main.go

$ dep init
Cached github.com/influxdata/kapacitor

$ dep ensure github.com/influxdata/influxdb@e4628bb69266dbd624dc27d674b52705ce0dcbf2

manifest.json is:

{
    "dependencies": {
        "github.com/influxdata/influxdb": {
            "revision": "e4628bb69266dbd624dc27d674b52705ce0dcbf2"
        },
        "github.com/influxdata/kapacitor": {
            "branch": "master"
        }
    }
}

and lock.json is:

{
    "memo": "b9eaf92e4ba015df09c7113e8a3a11eabfaf54feaa1d8814f6441ee34b892d29",
    "projects": [
        {
            "name": "github.com/gogo/protobuf",
            "version": "v0.3",
            "revision": "909568be09de550ed094403c2bf8a261b5bb730a",
            "packages": [
                "proto"
            ]
        },
        {
            "name": "github.com/influxdata/influxdb",
            "version": "v1.2.0",
            "revision": "d40a9df8057f5f4124f5709cb8238591e2abcb8e",
            "packages": [
                "influxql",
                "influxql/internal",
                "influxql/neldermead",
                "models",
                "pkg/escape"
            ]
        },
        {
            "name": "github.com/influxdata/kapacitor",
            "branch": "master",
            "revision": "73b05c5e24439f19663ffc8f95582352309db79e",
            "packages": [
                "tick/ast"
            ]
        },
        {
            "name": "go.uber.org/atomic",
            "version": "v1.0.0",
            "revision": "0c9e689d64f004564b79d9a663634756df322902",
            "packages": [
                "."
            ]
        },
        {
            "name": "go.uber.org/zap",
            "version": "v1.0.0-rc.3",
            "revision": "4e517d38b2138e59475d6468e7a6b626eecdaa66",
            "packages": [
                ".",
                "buffer",
                "internal/bufferpool",
                "internal/color",
                "internal/exit",
                "internal/multierror",
                "zapcore"
            ]
        }
    ]
}

I was expecting the revision of github.com/influxdata/influxdb in lock.json to change to e4628bb69266dbd624dc27d674b52705ce0dcbf2.

[sdboyer]

Ahh, that was the issue.

Constraints specified in manifest.json only apply to your direct dependencies. That's by design. The problem right now is that a) we don't tell you that your constraints are ineffectual and b) dep ensure is still writing to the manifest.

I'll keep this open (but retitled) as a reminder that we need better feedback about this.

[goller]

@sdboyer Right on. I worked around this by having my desired version checked out into my go path before running dep init.

[mikegleasonjr]

I hand edited my lock.json for one of my indirect dependency, removed my ./vendor and re-ran dep ensure

[mikijov]

I will take a look at this.

[sdboyer]

@mikijov awesome! a quick pointer - there's already logic in gps that computes ineffectual constraints in more or less the way discussed here - we need to exclude such constraints from input hashing. That logic is in rootdata. getApplicableConstraints():

dep/internal/gps/rootdata.go

Line 82 in 18ebb09

func (rd rootdata) getApplicableConstraints(stdLibFn func(string) bool) []workingConstraint {

We don't want to surface that whole struct, or that method directly, as it's deeply enmeshed in gps' internal systems. However, it may be worth surfacing a standalone func that does the work, moving the body of getApplicableConstraints() into that func, and then just calling it from the method.

[mikijov]

Hi @sdboyer. Thanks for the pointer. I do need a bit more guidance.

a) gps package does not have a way to report warnings. Solution is sanitized from the internal details so I cannot call methonds on it for details of it's run. So I see few ways to do this:

• output to stderr when detecting ineffectuals (yuck)
• expand Solution or Lock interface to contain ineffectual constraints
• expand Solution to have a generic []Warnings and use them
  What's your preference?

b) From your problem definitio above b) dep ensure is still writing to the manifest. I don't see anywhere where ensure writes to manifest. I presume dep has changed since the issue was posted. Or...?

[sdboyer]

@mikijov hi, sorry for delay - i'm just busy and getting busier these days :/

I think you're trying to reach further than is necessary with this 😄

I think the goal here is a func, with a signature something like this:

func FindIneffectualConstraints(m Manifest, ptree pkgtree.PackageTree) ProjectConstraints

Where the returned map is all the projects that have constraints declared which will have no effect on solving. That function can then be called from anywhere, and the caller can decide what to do with logging. Let's start with the function, and we can worry about where to actually call it from next.

[mikijov]

Hi @sdboyer. No worries, it's not a simple codebase, so I understand we should keep it as clean/simple as possible.

I did hear your directon the first time. Bit I thought I would have to expose too much of gps' internals if I went that way. You helped a bit by suggesting different signature. So I was able to come up with something (see below). However, second part of the getApplicableConstraints starts using internal gps data structs, rootdata, workingConstraints, maybe more. So I need bit more guidance.

• Do I need to apply the overrides? Doing so starts using workingConstraints, which I believe loses ProjectProperties which I need to return ProjectConstraints.
• Can you quickly explain why searching using radix tree is required, and why simply comparing project roots like below is not sufficient?

func FindIneffectualConstraints(manifest Manifest, packageTree pkgtree.PackageTree) ProjectConstraints {
	// Merge the normal and test constraints together
	constraints := manifest.DependencyConstraints().merge(manifest.TestDependencyConstraints())

	// if manifest is a RootManifest, then apply the overrides
	if rootManifest := manifest.(RootManifest); rootManifest != nil {
		// Include any overrides that are not already in the constraint list.
		// Doing so makes input hashes equal in more useful cases.
		for projectRoot, projectProperties := range rootManifest.Overrides() {
			if _, exists := constraints[projectRoot]; !exists {
				pp := ProjectProperties{
					Constraint: projectProperties.Constraint,
					Source:     projectProperties.Source,
				}
				if pp.Constraint == nil {
					pp.Constraint = anyConstraint{}
				}

				constraints[projectRoot] = pp
			}
		}
	}

	var ineffectuals ProjectConstraints
	for packageRoot, packageProperties := range constraints {
		if _, used := packageTree.Packages[string(packageRoot)]; !used {
			ineffectuals[packageRoot] = packageProperties
		}
	}

	return ineffectuals
}

[sdboyer]

@mikijov thanks for sticking with it 😄

However, second part of the getApplicableConstraints starts using internal gps data structs, rootdata, workingConstraints, maybe more. So I need bit more guidance.

I think we can ignore all of that. The goal of this func isn't actually to do any of that complex overriding behavior - it's just to identify which ProjectRoots have constraints that aren't going to have any effect. So (sorry) I think we could even revise the signature again:

func FindIneffectualConstraints(manifest Manifest, packageTree pkgtree.PackageTree) []ProjectRoot

Because all we want is that list.

Do I need to apply the overrides? Doing so starts using workingConstraints, which I believe loses ProjectProperties which I need to return ProjectConstraints.

Hmm...good question. This really ends up being a question about what "ineffectual" means.

Let's start with "no", and see how things go.

[mikijov]

Hi @sdboyer. I'm back. :-) So, first off, I have to say that I thought of you few times while trying to work out what various 1-2 letter variables mean. But I will persist.

So, let's talk ineffectuals. Do you mind taking a quick look if the direction I am taking is close to what you had in mind. I think I am fairly close. However, I am stuck on one point. Constraints by definition point to root folder. However PackageTree points to actual packages. I see no reliable way of getting the project root from the package. Is this the good place to use Radix tree or create path aware .HasPrefix? Or do you have a better suggestion?

[JackyChiu]

@sdboyer Cool if I pick this up? Looks like no one is on it

[sdboyer]

you're welcome to give it a go! though please note that if you don't end up getting to this in the next couple days, I may end up doing it. this is really quite pressing, and it was an error on my part to leave it open for so long, instead of just tackling it myself.

…

On December 28, 2017 3:00:33 PM EST, Jacky Chiu ***@***.***> wrote: @sdboyer Cool if I pick this up? Looks like no one is on it -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #302 (comment)

[sdboyer]

FINALLY! 🎉 🎉