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

Warn on ineffectual constraints #1534

Merged
merged 7 commits into from
Jan 17, 2018
Merged

Warn on ineffectual constraints #1534

merged 7 commits into from
Jan 17, 2018

Conversation

sdboyer
Copy link
Member

@sdboyer sdboyer commented Jan 16, 2018

What does this do / why do we need it?

Warns the user when they have one or more [[constraint]] stanzas in Gopkg.toml, but do not have a direct dependency on what's named in there.

There are also a few unrelated refactors in here, because it's just pointless to do those small things on their own.

What should your reviewer look out for in this PR?

i mean, i'm skipping tests - we can come back to those later, as long as hand-testing works. We need to ship the next release.

Do you need help or clarification on anything?

Error wording, especially. i was intentionally verbose here - i hope our errors will get more Elm-ish in this way going forward.

Which issue(s) does this PR fix?

#302

This method is a a complete and correct implementation for retrieving
the list of direct dependency names. It should be able to be used by all
dep commands, if needed.
Specifically, we need to switch map[string]bool for
map[gps.ProjectRoot]bool. Trivial refactor, but it's preferable to do
this anyway as this is a situation where gps.ProjectRoot should be used
to denote that strings in this map are assumed to be project roots. Case
in point, one of the importers was making the assumption that it had
packages, not root paths, but it wasn't obvious without the type
hinting.
cmd/dep/init.go Outdated
@@ -137,12 +99,15 @@ func (cmd *initCommand) Run(ctx *dep.Ctx, args []string) error {
if ctx.Verbose {
ctx.Out.Println("Getting direct dependencies...")
}
pkgT, directDeps, err := getDirectDependencies(sm, p)

// If this errors, the next call will too; don't bother handling it twice.
Copy link
Collaborator

@jmank88 jmank88 Jan 16, 2018

Choose a reason for hiding this comment

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

This seems brittle.

Since GetDirectDependencyNames calls ParseRootPackageTree internally, what about an additional ptree return from GetDirectDependencyNames?

GetDirectDependencyNames(sm gps.SourceManager) (pkgtree.PackageTree, map[gps.ProjectRoot]bool, error) {

Edit: If this was the only reason for caching the result internally, then that could be simplified out too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like that nil pointer is from the cache field too: if len(p.RootPackageTree.Packages)

Copy link
Member Author

Choose a reason for hiding this comment

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

i waffled over this a bit. it was between that signature, and what i did here - because we absolutely can't create a circumstance where casual use of the API results in hitting the disk twice for data that, at least in dep's model, hasn't changed.

however, i didn't particularly like having the extra return value - it felt like exposing a bit too much detail of the underlying behavior. the ptree isn't what we asked for here. but, now that i've finished writing it and looking back, i'd agree that the cost is less than the complexity arising from adding this new statefulness dimension to *dep.Project.

so yeah, i'll switch it around.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually looking at the code again, i've flipped back. not caching it internally also makes the FindIneffectualConstraints() method much more complicated to implement.

there are a lot of different possible paths through this code, and orders in which different use cases might call it. we can't reasonably take out the caching. but i suppose that also adding the additional return value doesn't hurt - at minimum, it's one less instance of error handling (which was your original concern).

@@ -220,7 +220,10 @@ func (g *gopathScanner) scanGopathForDependencies() (projectData, error) {
return projectData{}, nil
}

for ip := range g.directDeps {
for ippr := range g.directDeps {
// TODO(sdboyer) these are not import paths by this point, they've
Copy link
Member Author

Choose a reason for hiding this comment

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

@carolynvs i noticed this while working on this. do you have any immediate thoughts about bugs we might have as a result of this pseudotype mismatch?

A convenience, as callers often need both the RootPackageTree and the
direct dependencies map. Though this does further suggest that we ought
to be able to stitch this up neatly in gps, at some point.

Also, handle an error that was previously dropped.
Copy link
Collaborator

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Tried it and works as expected.

This is how the warning looks:

Warning: the following project(s) have [[constraint]] stanzas in Gopkg.toml:

  ✗  golang.org/x/net

However, these projects are not direct dependencies of the current project:
they are not imported in any .go files, nor are they in the 'required' list in
Gopkg.toml. Dep only applies [[constraint]] rules to direct dependencies, so
these rules will have no effect.

Either or import/require packages from these projects to make them into direct
dependencies, or convert the [[constraint]] to an [[override]] to enforce rules
on these projects if they are transitive dependencies,

LGTM!

@sdboyer sdboyer merged commit 3b8fbe4 into golang:master Jan 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants