Add prune options to manifest #1226

ibrasho · 2017-09-29T08:54:34Z

What does this do / why do we need it?

Add prune options to dep.Manifest.

WIP:

Prevent setting false to root prune options
Per-project prune options
Warn on redundant per-project options
Add more tests

What should your reviewer look out for in this PR?

Generic review.

Do you need help or clarification on anything?

How should prune options be defined for projects?

Here is a list of suggestions:

TOML table under prune:

[prune]
  non-go = true
  unused-packages = true

  ["github.com/golang/protobuf"]
    non-go = true
    go-tests = true

prune table under constraints (what about overrides?)

[[constraint]]
  name = "github.com/jmank88/nuts"
  version = "0.2.0"
  prune = { non-go = true, go-tests = true, unused-packages = true }

or

[[constraint]]
  name = "github.com/jmank88/nuts"
  version = "0.2.0"

  [constraint.prune]
    go-tests = true
    unused-packages = true

Which issue(s) does this PR fix?

One more step towards #944

ibrasho · 2017-09-29T09:03:09Z

Tests should be failing for now and the rules in Gopkg.toml will be removed in a later commit.

I'm still thinking about the best way to set prune options per project. Can anyone suggest other alternatives?

For me, the first option is cleaner, but then we need to make sure that rules are applied (and report on ineffectual rules).

The second option is closer to how metadata are formatted currently.

@sdboyer: Should prune options affect locks? are they added to the lock and do they change the hash? or are they oblivious to them?

jmank88 · 2017-09-29T12:17:00Z

Did you consider a list of strings?

[[constraint]]
  name = "github.com/jmank88/nuts"
  version = "0.2.0"
  prune = { "non-go", "go-tests", "unused-packages" }

Would require a bit more parsing/conversion work, but perhaps is more human friendly.

jmank88 · 2017-09-29T12:22:12Z

Now I'm also wondering about the names themselves.

non-go seems straightforward.

Would go-tests make sense as just tests? (or does that promise something broader?)

Could unused-packages be unused or unused-pkgs?

ibrasho · 2017-09-29T16:02:12Z

Did you consider a list of strings?

I thought about that but parsing looked annoying so I went for a table instead. Didn't really try it though. But I agree that a table of booleans ( = true) is ugly.

I'm more concerned about where to define the per-project rules though.

Another thing that needs will need clarification is how rules cascade...
Say I have prune = { "non-go" } globally. How should a project with prune = { "unused-packages" } be treated? Only prune nested vendor dirs (by default) and unused packages? Or does it inherit the global rules? This applies to both booleans and lists, but it's easier to handle with bools IMO.

jmank88 · 2017-09-30T13:42:55Z

This applies to both booleans and lists, but it's easier to handle with bools IMO.

I was imaging that the lists would still translate to booleans underneath, and just default to false when absent. Are you suggesting that literal false and omission are distinct states?

ibrasho · 2017-09-30T14:23:01Z

Are you suggesting that literal false and omission are distinct states?

Oh, no. Just that with a list it's implicit that omission translates to a false.

The main question is: Should per-project options inherit the global options or no?

But first, let me construct a full example to clarify that question:

[prune]
  unused-packages = true

[[constraint]]
  name = "github.com/project/repository"

  # Booleans
  [constraint.prune]
  non-go = true

  # List
  prune = { "non-go" }

What should the prune options for github.com/project/repository be?

unused-packages & non-go?
Only non-go?

I personally think that if prune options are defined for a project it should reset and override whatever was defined globally. If that's the case, both options (lists or booleans) are equal.

But if per-project rules inherit the global rules, then booleans are more explicit.

I feel I'm overthinking this 😁 , but it was the first thing that came to mind when Sam said that we need per-project options.

jmank88 · 2017-09-30T15:22:11Z

But if per-project rules inherit the global rules, then booleans are more explicit.

But then what would this mean? Seems like it should override the global, but that would make it distinct from omitting it entirely.

[prune]
  unused-packages = true

[[constraint]]
  name = "github.com/project/repository"

  [constraint.prune]
  unused-packages = false

jmank88 · 2017-09-30T15:25:11Z

If overriding the global with a false is a necessary/useful feature, then this makes sense, and can be a *bool internally.

If not, then using a list would remove the option.

sdboyer · 2017-09-30T19:04:19Z

Should prune options affect locks? are they added to the lock and do they change the hash? or are they oblivious to them?

which hash, the inputs-digest? nope, they won't affect that. they will affect the hash we eventually generate as part of vendor verification, but i outlined that already.

yes, we absolutely have to support per-dependency overrides of the global options, so we have no choice but to rely on discrete properties that expect booleans. it is a bit more verbose, which isn't the greatest, but the necessity of expressing it one property per line means makes it more diff-friendly.

my inclination is to say that, in order to avoid spurious declarations (such as those that plague us in #302), we should make sure that we initially ship this feature with the following validations:

in the global prune settings, false is categorically disallowed (meaning, is grounds for dep ensure to immediately exit) as it is the same as an omission
in project-specific prune settings, we warn if there is a redundant true or false declaration for any prune prop (meaning, it just reasserts the same value that is already set globally). this is a warning, rather than an error, because it would otherwise be a giant PITA for folks to fiddle with the properties.

as for the property names themselves, i think we may still have some work to do in articulating exactly what the different prune modes are. when i wrote #944 (comment), i remember thinking that some of these flags may not be entirely orthogonal - that some flags may encompass others. maybe that's not the case, but i need a clear model in my head to be confident about this 😄 it may take a bit of spec-style writing to get there.

ibrasho · 2017-10-13T16:41:54Z

as for the property names themselves, i think we may still have some work to do in articulating exactly what the different prune modes are. when i wrote #944 (comment), i remember thinking that some of these flags may not be entirely orthogonal - that some flags may encompass others. maybe that's not the case, but i need a clear model in my head to be confident about this 😄 it may take a bit of spec-style writing to get there.

@sdboyer I wrote something to personally understand what each option does. I'll try to formalize it early next week.

I'm cleaning this up for now and I had another question.
Can prune be defined on both constraints and overrides? Should we fail when options are defined on the same ProjectRoot in both a constraint and an override?

This made me think it might be better to do something like the following:

[prune]
  non-go = true

  [[project]]
    name = "github.com/project/test"
    non-go = false

@sdboyer @jmank88 What do you think?

ibrasho · 2017-10-14T10:16:30Z

After sleeping on it, I can see many issues coming from the structure I proposed, especially when people try to set prune options on sub-packages... 😞

sdboyer · 2017-10-15T20:21:10Z

oh? I thought that actually might be really good. what shortcomings do you see?

prune options will still be per project, not per package, so that, at least, shouldn't be a problem.

ibrasho · 2017-10-27T11:23:06Z

The biggest issue I'm facing right now is the following (only tests are remaining):

Assuming we have the following manifest:

[[constraint]]
  name = "github.com/golang/dep/internal/gps"
  version = "0.12.0"

[prune]
  non-go = true

  [[prune.project]]
    name = "github.com/golang/dep"
    non-go = false

omitempty will not output any key with a false value. Removing it prints all keys with a false value.

All my attempts to create a representation that outputs the required TOML failed. And github.com/pelletier/go-toml doesn't use an UnmarshalTOML method to give us more flexibility.

Looking forward to any pointers.

sdboyer · 2017-10-27T13:36:21Z

@pelletier any guidance to offer here? 🙏

pelletier · 2017-10-27T16:57:22Z

👋 I'm not sure I fully understand. Given the manifest in #1226 (comment), you want to unmarshal it?

omitempty will not output any key with a false value. Removing it prints all keys with a false value.

That sounds like a bug in the handling of `omitempty. Do you have some code I could use to reproduce + expected behavior?

github.com/pelletier/go-toml doesn't use an UnmarshalTOML method to give us more flexibility.

It does as of pelletier/go-toml@4e9e0ee (3 days ago)! You can add options to the Encoder type and use its Encode() method.

ibrasho · 2017-10-27T23:20:31Z

@pelletier My bad. We need to be able to implement a MarshalTOML method.

Here is the issue to clarify:
Prune options can be defined on the root level, or for a specific project. Root options should be set to true or omitted. Prune options for projects are allowed to be set to false only if the root options is true(as an override). I've tried many things but couldn't find a setup that will marshal the required values.

To give a concrete example:

type rawRootPruneOptions struct {
    NonGoFiles bool `toml:"non-go,omitempty"`

    Projects []rawPruneProjectOptions `toml:"project,omitempty"`
}
type rawPruneProjectOptions struct {
    NonGoFiles bool `toml:"non-go,omitempty"`
}
a := rawRootPruneOptions{
    NonGoFiles: false,
    Projects: []rawPruneProjectOptions{
        rawPruneProjectOptions{
            NonGoFiles: false,
        },
    },
}
b := rawRootPruneOptions{
    NonGoFiles: true,
    Projects: []rawPruneProjectOptions{
        rawPruneProjectOptions{
            NonGoFiles: false,
        },
    },
}

a should result in an empty string (since all values are empty) while b should result in:

[prune]
  non-go = true

  [[prune.project]]
    name = "github.com/golang/dep"
    non-go = false

ibrasho · 2017-11-10T21:21:43Z

@sdboyer @carolynvs This PR is ready for review and merge.

As we discussed, we'll ignore marshaling prune options for the time being since it's not needed. Below is a brief explanation of the current options and what they do:

unused-packages: This option will remove all files[1] in all directories that represents unused packages in the project. Unused packages are computed based on diffing the project directories with LockedProject.Packages(). [2]
non-go: This option will remove all files that are no suffixed by .go[3] and fails the isPreservedFile check. The isPreservedFile check compares the file name to the licenseFilePrefixes list and legalFileSubstrings list to determine if the file should be preserved.
go-tests: This option will delete all files suffixed by _test.go[3] in the project.

Some questions:
[1] Should we only delete Go files?
[2] How should we approach package names that differ from directory names? If the path is github.com/org/project/package but the package name is github.com/org/project/thing? The current behavior ignores that case for the project root, but sub packages' names must match the directory name.
[3] Should these be case-insensitive? Or should we always assume lowercase for _test.go and .go?

darkowlzz · 2017-11-13T15:13:05Z

manifest.go

+			}
+		case "name":
+			if root {
+				warns = append(warns, errors.Errorf("%q should not include a name", "prune"))


Let's create an error constant for this?

I noticed we only created var for errors. But I guess we can do that.

oops! my mistake. Yes, error variable. 😅

darkowlzz · 2017-11-13T15:13:59Z

manifest_test.go

+			  name = "github.com/golang/dep"
+			`,
+			wantWarn: []error{
+				fmt.Errorf("%q should not include a name", "prune"),


Let's replace this with an error constant.

darkowlzz · 2017-11-13T15:14:52Z

manifest.go

+//
+// It will return the root prune options if the project does not have specific
+// options or if it does not exists in the manifest.
+func (m *Manifest) PruneOptionsFor(pr gps.ProjectRoot) gps.PruneOptions {


Do we need this function? I don't see it being used anywhere. Maybe for future usage?

Yes. The other PR will use this function.

darkowlzz · 2017-11-13T15:15:34Z

manifest_test.go

 				}
 			}
 		})
 	}
 }
+
+func TestPruneOptionsFor(t *testing.T) {


We can remove this too if PruneOptionsFor() isn't required.

darkowlzz · 2017-11-13T15:16:14Z

manifest_test.go

+
+func containsErr(s []error, e error) bool {
+	for _, a := range s {
+		if a.Error() == e.Error() {


I think we can compare the error values directly and not the error message?

Won't that compare pointers? Unless they refer to same pointer value they won't match.

If we create variables for all the possible errors (which I think we almost do), the comparison would happen of the same pointer values. But, if we have some dynamic error, then this is fine. 🙂

We still have some dynamic errors sadly.

sdboyer · 2017-11-16T05:04:14Z

@ibrasho did you land on a choice as to whether we stick with just .go (and .asm), or if we should do all the filetypes?

ibrasho · 2017-11-16T16:14:01Z

I think we should add the whole switch statement to be on the safe side. The implementation is part of the other PR (#1219) though.

Should we change the configuration key ("non-go") to reflect that change?

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho · 2017-11-17T07:56:22Z

To keep a trail of our discussion on Slack (this affect #1219):

Should we only delete Go files?

I'll add all the extensions found in the import logic in go build.

How should we approach package names that differ from directory names? If the file path is github.com/org/project/package but the package name is github.com/org/project/my-package? The current behavior ignores that case for the project root, but sub packages' names must match the directory name.

We haven't discussed this point, @sdboyer. Any input?

Should these be case-insensitive? Or should we always assume lowercase for _test.go and .go?

Go requires lower-case .go and _test.go.

sdboyer · 2017-11-17T13:50:55Z

@ibrasho oh yeah, we forgot to talk about package names. fortunately, that one's simple: we don't care if they differ, because we almost never care about package name. almost everything else in dep is based on import path positioning, not package name, and this should follow that pattern.

re: case sensitivity, first look and see if the language spec is explicit on the topic. if not, follow the standard compiler/go/build behavior.

ibrasho · 2017-11-18T10:01:34Z

I've checked the spec before and it doesn't mention the extension at all. However, the stdlib implementation is always assuming .go and _test.go.
Even the testing package) says:

To write a new test suite, create a file whose name ends _test.go that contains the TestXxx functions as described here.

About package name/directory name:
I wasn't sure how Go handle package names that differ from the encapsulating directory name, but the current behavior is as you said. I was worried we might delete a wrong file by mistake.

ibrasho · 2017-11-18T10:22:08Z

For the options naming (we discussed that the options are a mix of positive and negative rules). I was thinking of it in the following manner:

[prune]
  non-go= true # Prune non-Go build files globally

[prune]
  [project]
    name = "github.com/awesome/project"
    go-tests = true # Prune Go test files in this project

[prune]
  unused-packages = true # Prune unused packages globally
  [project]
    name = "github.com/awesome/project"
    unused-packages = false # Don't prune unused packages in this project

The issue I have now is with non-go. I'm trying to think of a better name since we are keeping more files now.

non-go-source, non-go-package, non-go-compilable or any other suggestions?

sdboyer · 2017-11-20T18:18:14Z

i think we're good with non-go.

sdboyer · 2017-11-20T18:21:33Z

weeee!

ibrasho requested a review from sdboyer as a code owner September 29, 2017 08:54

googlebot added the cla: yes label Sep 29, 2017

darkowlzz added this to the v0.3.3 milestone Oct 20, 2017

sdboyer modified the milestone: v0.3.3 Oct 24, 2017

ibrasho requested a review from carolynvs as a code owner October 27, 2017 10:57

ibrasho force-pushed the add-prune-options-to-manifest branch 4 times, most recently from fac4a7b to 4471656 Compare October 27, 2017 11:16

ibrasho mentioned this pull request Oct 27, 2017

Switch to github.com/BurntSushi/toml #1323

Closed

ibrasho force-pushed the add-prune-options-to-manifest branch 2 times, most recently from 16dc0cd to 505c236 Compare October 27, 2017 23:26

ibrasho self-assigned this Oct 30, 2017

ibrasho changed the title ~~[WIP] Add prune options to manifest~~ Add prune options to manifest Oct 30, 2017

ibrasho force-pushed the add-prune-options-to-manifest branch 2 times, most recently from 3d69984 to f2e422e Compare November 10, 2017 20:33

ibrasho force-pushed the add-prune-options-to-manifest branch from f2e422e to 7f7cd4d Compare November 10, 2017 21:24

ibrasho mentioned this pull request Nov 11, 2017

Absorb prune into ensure #944

Closed

1 task

darkowlzz reviewed Nov 13, 2017

View reviewed changes

ibrasho added 3 commits November 17, 2017 10:25

nits: letter casing and test table names

4724b1f

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

dep: add prune options to manifests

13aa6fc

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

dep: update tests for adding prune to manifest

7ed9049

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho force-pushed the add-prune-options-to-manifest branch from 7f7cd4d to fc2d241 Compare November 17, 2017 07:25

ibrasho added 2 commits November 17, 2017 10:49

dep: convert some errors to a variable

09e6f3b

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ignore (*Manifest).PruneOptionsFor is unused error

b439b8d

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho force-pushed the add-prune-options-to-manifest branch from cc93dba to b439b8d Compare November 17, 2017 07:49

sdboyer approved these changes Nov 20, 2017

View reviewed changes

sdboyer merged commit 9d269a6 into golang:master Nov 20, 2017

carolynvs mentioned this pull request Nov 27, 2017

Use default prune settings during dep init #1404

Closed

ibrasho deleted the add-prune-options-to-manifest branch November 29, 2017 06:18

dshulyak mentioned this pull request Jan 24, 2018

Introduce the dependency vendoring tool: dep. status-im/status-go#551

Merged

5 tasks

mandrigin mentioned this pull request Jan 24, 2018

Add an option to keep non-go files to dep status-im/status-go#569

Closed

Add prune options to manifest #1226

Add prune options to manifest #1226

Conversation

ibrasho commented Sep 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?

ibrasho commented Sep 29, 2017 • edited Loading

jmank88 commented Sep 29, 2017

jmank88 commented Sep 29, 2017

ibrasho commented Sep 29, 2017 • edited Loading

jmank88 commented Sep 30, 2017

ibrasho commented Sep 30, 2017 • edited Loading

jmank88 commented Sep 30, 2017

jmank88 commented Sep 30, 2017 • edited Loading

sdboyer commented Sep 30, 2017

ibrasho commented Oct 13, 2017

ibrasho commented Oct 14, 2017

sdboyer commented Oct 15, 2017

ibrasho commented Oct 27, 2017 • edited Loading

sdboyer commented Oct 27, 2017

pelletier commented Oct 27, 2017

ibrasho commented Oct 27, 2017 • edited Loading

ibrasho commented Nov 10, 2017

darkowlzz Nov 13, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkowlzz Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Nov 16, 2017

ibrasho commented Nov 16, 2017 • edited Loading

ibrasho commented Nov 17, 2017

sdboyer commented Nov 17, 2017 • edited Loading

ibrasho commented Nov 18, 2017

ibrasho commented Nov 18, 2017

sdboyer commented Nov 20, 2017

sdboyer commented Nov 20, 2017

ibrasho commented Sep 29, 2017 •

edited

Loading

ibrasho commented Sep 29, 2017 •

edited

Loading

ibrasho commented Sep 29, 2017 •

edited

Loading

ibrasho commented Sep 30, 2017 •

edited

Loading

jmank88 commented Sep 30, 2017 •

edited

Loading

ibrasho commented Oct 27, 2017 •

edited

Loading

ibrasho commented Oct 27, 2017 •

edited

Loading

darkowlzz Nov 13, 2017 •

edited

Loading

darkowlzz Nov 13, 2017 •

edited

Loading

ibrasho commented Nov 16, 2017 •

edited

Loading

sdboyer commented Nov 17, 2017 •

edited

Loading