[WIP] Report Ineffectual Rules #1494

JackyChiu · 2017-12-29T18:38:34Z

What does this do / why do we need it?

Report ineffectual rules found in the manifest. This happens on dep ensure.

Did this by:

Refactoring the getDirectDependinces method to be a part of the Project struct
Adding a validation method that reports for ineffectual constraints and ignored packages
Calling the validation in ensure and ~~status~~ commands

Would like some intermediate feedback before I start writing the tests and fixing a bunch of tests

What should your reviewer look out for in this PR?

Implementation
Am I reporting in the right place?
Am I reporting the right ineffectual rules?
Is the error message clear?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

fixes #302

Readd DeduceProjectRoot

darkowlzz · 2017-12-29T19:03:03Z

[NOT A REVIEW]
Please add this to the changelog.

And also, I think we would like to have this warning in ensure only, not in status. Could make status annoying. We had some discussion about it sometime back.

darkowlzz

Looks to be in the right direction.
We would also need integration tests for this.

darkowlzz · 2018-01-02T18:10:16Z

manifest.go

@@ -33,6 +33,7 @@ var (
 	errInvalidMetadata     = errors.New("metadata should be a TOML table")

 	errInvalidProjectRoot = errors.New("ProjectRoot name validation failed")
+	errInefficientRules   = errors.New("Inefficient rules found in the manifest")


errors should start with lowercase, unless they are some special term, like ProjectRoot.

darkowlzz · 2018-01-02T18:13:39Z

manifest.go

@@ -324,6 +325,78 @@ func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error {
 	return valErr
 }

+// ValidatePackageRules ensure that there are no ineffectual package declarations
+// in the constraints, overrides, required, and ignored rules in the manifest.


I don't think we should do this with required and ignored, because they can be used to include and ignore packages that are not part of root project code or not directly reachable by the root project.

I reread the spec for required and I agree it shouldn't/ be applied in this case.

But for ignored I can see value in letting you know that one of the packages in that rule isn't being imported.

Let me know where we'd like to go with this when you get a chance!

darkowlzz · 2018-01-02T18:17:33Z

manifest.go

+		}
+	}
+
+	// Check overrides as well


Let's have proper sentences with a period at the end for all the comments :) We are doing it as much as possible for all the new code.

- casing in error message - whole sentences in comments - remove merging or required packages - omit underscore values in ranges

JackyChiu · 2018-01-05T23:55:31Z

I'd like one more persons feedback before I get started on the tests please 😄

jmank88 · 2018-01-06T21:54:53Z

manifest.go

@@ -324,6 +325,71 @@ func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error {
 	return valErr
 }

+// ValidatePackageRules ensure that there are no ineffectual package declarations
+// in the constraints, overrides, and ignored rules in the manifest.
+func ValidatePackageRules(c *Ctx, proj *Project, sm gps.SourceManager) (err error) {


What about separating responsibilities here by dropping the c *Ctx argument and instead returning an extended errInefficientRules error that knows how to print itself the same way?

I like the idea, of dropping Ctx, but to clarify do you mean appending more lines to the errInefficentRules before returning it?

I was thinking expand var errInefficentRules error from a single static error instance to something like type errInefficentRules struct{constraints, ignores []gps.ProjectRoot} with an Error() string method (which e.g. builds a string via bytes.Buffer rather than logging). So, essentially the same result as adding lines to errInefficentRules before returning, but underneath it would actually be deferred until the error is printed.

^ This would also set us up to potentially export errInefficentRules and its fields later on, giving callers even more flexibility when interpreting and handling the error.

Yeah that sounds pretty good to me, will do

jmank88 · 2018-01-06T21:57:50Z

manifest.go

+
+// FindIneffectualConstraints looks for constraints decleared in the project
+// manifest that aren't directly used in the project.
+func FindIneffectualConstraints(m *Manifest, directDeps map[string]bool) []gps.ProjectRoot {


It looks like the FindIneffectual* functions don't need to be exported, is there a compelling reason to export them?

They don't need to be, I'll fix that

- move display string logic in errInefficientRules.Error() - remove Ctx from ValidatePackageRules - unexport FindIneffectual* funcs

JackyChiu · 2018-01-09T04:14:30Z

@darkowlzz @jmank88 should we get rid of checking for ignores as a part of the validation?

Also I'll have to open up a new PR to fix the current integration tests, many of them call ensure and have unimported constraints :/. Or is there a better way of doing this?

darkowlzz · 2018-01-10T15:00:20Z

Sorry for the delay. I have new thoughts now:

About ineffectual required, we can do it but it's not the same as ineffectual constraint. required and ignored are package based and constraint is project based. So your previous implementation would need some tweaking.

If a project is a transitive dependency, the project would have an entry in the lock file with all the subpackages of it that are needed for the main project to build. Adding a required for any package in this project would result in ineffectual required if the package already exists in the packages list in the lock. I'm talking about this packages list from lock, to make it clear

[[projects]]
  name = "github.com/golang/dep"
  packages = [
    ".",
    "cmd/dep",
    "internal/feedback",
    "internal/fs",
    "internal/gps",
    "internal/gps/internal/pb",
    "internal/gps/paths",
    "internal/gps/pkgtree",
    "internal/importers",
    "internal/importers/base",
    "internal/importers/glide",
    "internal/importers/godep",
    "internal/importers/govend",
    "internal/importers/gvt",
    "internal/importers/vndr"
  ]
  revision = "8ddfc8afb2d520d41997ebddd921b52152706c01"
  version = "v0.3.2"

Also, given a manifest and lock, it's difficult to tell if a given project is in the lock due to the required package in manifest or if it's a transitive dependency. I think a solve without applying required would help answer this. But as you can see, finding ineffectual for required is not as simple as for constraints with only reachability check.
So, I would recommend to keep this PR for ineffectual constrains only for now. We can work on ineffectual required later separately.

Addressing one of your old comments:

But for ignored I can see value in letting you know that one of the packages in that rule isn't being imported.

ignored isn't about the current project. ignored could be anything that's never imported in the main project. So, to answer your recent question

should we get rid of checking for ignores as a part of the validation?

yes, let's not do it and focus on ineffectual constraints only.

Also, let's change ValidatePackageRules to have "Project" or "Constraint" in the name. Package is only for required and ignored, constraint is project based.

Also I'll have to open up a new PR to fix the current integration tests, many of them call ensure and have unimported constraints :/. Or is there a better way of doing this?

I would recommend building clean sets of changes as multiple commits and squashing your existing commits to cleanup. Keeping new implementation, old test fixes and new test changes into cleanly separated commits would help while reviewing and avoid failing tests across PRs. I like commits that build stories :)

darkowlzz · 2018-01-11T11:48:52Z

Thoughts about ineffectual ignores:

ignored is very different from required or constraint. It defines what we want dep to not see in the dep graph, resulting in no static analysis of that package. An ignore would be ineffectual if an ignore is already covered by another ignore. Say we have github.com/foo/bar/pkgA* and github.com/foo/bar/pkgA/pkgAx in ignored. In this case, the first ignore covers the second ignore and that makes the second ignore ineffectual. This is one way in which we can find ineffectual ignores.

Another way in which an ignore would be ineffectual is if the ignored package is not part of root project, direct dependency or transitive dependency. A totally unrelated package would also be an ineffectual ignore.

Leaving the idea here, we can do it as a separate PR in the future.

JackyChiu · 2018-01-15T14:54:44Z

@darkowlzz sounds good, thanks for explaining!

sdboyer

sorry, there's a couple too many things here - in particular, getDirectDependencies is actually the wrong implementation to be using here - that should only be used by init, as-written.

i'm going to close this in favor of my PR, #1534

sdboyer · 2018-01-15T19:49:37Z

manifest.go

+	return nil
+}
+
+// findIneffectualConstraints looks for constraints decleared in the project


sdboyer · 2018-01-15T19:49:55Z

manifest.go

@@ -324,6 +324,90 @@ func ValidateProjectRoots(c *Ctx, m *Manifest, sm gps.SourceManager) error {
 	return valErr
 }

+type errInefficientRules struct {


ineffectual, not inefficient

sdboyer · 2018-01-15T19:51:16Z

manifest.go

+	return ineffectuals
+}
+
+// findIneffectualIgnores looks for ignored packages decleared in the project


sdboyer · 2018-01-15T19:51:41Z

manifest.go

+
+// findIneffectualIgnores looks for ignored packages decleared in the project
+// manifest that aren't directly used in the project.
+func findIneffectualIgnores(m *Manifest, directDeps map[string]bool) []gps.ProjectRoot {


not possible to determine if ignores are ineffectual at this point

JackyChiu added 3 commits December 26, 2017 19:49

Add DirectDependencies method on project

a25a9ba

Refactor to use new DirectDependencies func

d855d65

Readd DeduceProjectRoot

Add package rule validation

0a3251f

JackyChiu requested review from carolynvs, darkowlzz, ibrasho and sdboyer as code owners December 29, 2017 18:38

googlebot added the cla: yes label Dec 29, 2017

JackyChiu added 2 commits December 29, 2017 16:41

Don't validate package rules on dep status

1149dea

Add pr improvements to the changelog

baf1731

darkowlzz reviewed Jan 2, 2018

View reviewed changes

Address PR comments:

29857b2

- casing in error message - whole sentences in comments - remove merging or required packages - omit underscore values in ranges

JackyChiu force-pushed the warn_for_ignored_constraints branch from ee72b5f to 29857b2 Compare January 4, 2018 02:38

jmank88 reviewed Jan 6, 2018

View reviewed changes

- make errInefficientRules it's own err type

6651d40

- move display string logic in errInefficientRules.Error() - remove Ctx from ValidatePackageRules - unexport FindIneffectual* funcs

darkowlzz mentioned this pull request Jan 9, 2018

status should not show ineffectual constraints #1510

Closed

sdboyer added this to the v0.4.0 milestone Jan 16, 2018

sdboyer suggested changes Jan 16, 2018

View reviewed changes

sdboyer closed this Jan 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Report Ineffectual Rules #1494

[WIP] Report Ineffectual Rules #1494

JackyChiu commented Dec 29, 2017 •

edited

Loading

darkowlzz commented Dec 29, 2017

darkowlzz left a comment

darkowlzz Jan 2, 2018

darkowlzz Jan 2, 2018

JackyChiu Jan 2, 2018

JackyChiu Jan 4, 2018

darkowlzz Jan 2, 2018

JackyChiu commented Jan 5, 2018

jmank88 Jan 6, 2018

JackyChiu Jan 7, 2018

jmank88 Jan 8, 2018

jmank88 Jan 8, 2018

JackyChiu Jan 9, 2018

jmank88 Jan 6, 2018

JackyChiu Jan 7, 2018

JackyChiu commented Jan 9, 2018

darkowlzz commented Jan 10, 2018

darkowlzz commented Jan 11, 2018

JackyChiu commented Jan 15, 2018

sdboyer left a comment

sdboyer Jan 15, 2018

sdboyer Jan 15, 2018

sdboyer Jan 15, 2018

sdboyer Jan 15, 2018

[WIP] Report Ineffectual Rules #1494

[WIP] Report Ineffectual Rules #1494

Conversation

JackyChiu commented Dec 29, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

darkowlzz commented Dec 29, 2017

darkowlzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackyChiu commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackyChiu commented Jan 9, 2018

darkowlzz commented Jan 10, 2018

darkowlzz commented Jan 11, 2018

JackyChiu commented Jan 15, 2018

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackyChiu commented Dec 29, 2017 •

edited

Loading