Absorb prune into ensure #952

ibrasho · 2017-08-04T18:08:10Z

What does this do / why do we need it?

This PR updates gps.WriteDepTree and Write on dep.*SafeWriterto accept a gps.PruneOptions and a *log.Logger to determine the pruning options when writing vendor/ and to enable verbose pruning.

The manifest is updated to accept a new table, prune. This table defines pruning options accepted by dep. A constructor for dep.Manifest is added to set the default PruneOptions.

By default, dep will always prune nested vendor directories.

Users can define 3 additional rules:

non-go: Prune non-Go files (will keep LICENSE & COPYING files)
go-tests: Prune Go test files
unused-packages: Prune unused Go packages.

What should your reviewer look out for in this PR?

Design comments and correctness as usual.

Which issue(s) does this PR fix?

fixes #944

ibrasho · 2017-08-05T08:58:11Z

@sdboyer I'm wondering how I should pass the PruneOptions to the SafeWriter?

I'm contemplating the choice between passing it in NewSafeWriter or when calling SafeWriter.Write... What do you think?

sdboyer · 2017-08-06T11:57:10Z

I'm contemplating the choice between passing it in NewSafeWriter or when calling SafeWriter.Write... What do you think?

eh, i don't have a strong feeling either way 🤷‍♂️

carolynvs · 2017-08-07T16:08:30Z

cmd/dep/gopath_scanner.go

-		Ovr:         make(gps.ProjectConstraints),
+		Constraints:  g.pd.constraints,
+		Ovr:          make(gps.ProjectConstraints),
+		PruneOptions: gps.PruneNestedVendorDirs,


Now that we want to have a default value for manifests (pruning nested vendor dirs), I'd prefer that we put that logic into a constructur dep.NewManifest, instead of relying upon every caller to remember to set it and get the default right.

Also, I didn't see that you updated the importers (they all make new manifests) and it's not clear to me what will even happen in that case if they leave the prune option off?

I was basing this on the assumption that pruning is opt-in. So we only remove nested vendor directories by default.

Sorry I'm late to the prune party and need a refresher course. When you say "pruning is opt-in" and also "we remove nested directories by default", I am not sure what will happen if someone doesn't explicitly set PruneOptions.

Basically I am concerned that if it's supposed to be a default, then it would be helpful to have it set by default in a constructor, rather than having to remember to set it everywhere as it's currently done in this PR. Some places aren't setting any PruneOptions (like the importers) and I can't really tell if they should, or if it doesn't matter because it's the default anyway.

If it's opt-in, do you mean that nothing happens (including removing nested vendor dirs) unless the user specifies a flag on the CLI or in the manifest? If so then why is it being set in code, that seems like a default, not opt-in...

None of this is criticism. I'm just confused. 😊

I've already added a Manifest constructor based on your comments, seems like a great idea.
I've updated the importers to use the new constructor. 😁

I'll rebase and restructure the commits to allow sane review since this PR contains a lot of changes. 😓

carolynvs · 2017-08-07T16:15:59Z

internal/gps/prune.go

+		}
+	}
+
+	// if (options & PruneNonGoFiles) != 0 {


Is this commented out because the PR is still a WIP?

Yes. 😓

I actually completed the PR but did a git checkout -- . by mistake and now I have to do it again. 😫

I'm hoping that I'll be able to complete the remaining changes, again, tomorrow.

Oh noes! Are any of your changes still hiding in the reflog perhaps?

carolynvs · 2017-08-07T16:20:14Z

manifest.go

+		Ovr:          make(gps.ProjectConstraints, len(raw.Overrides)),
+		Ignored:      raw.Ignored,
+		Required:     raw.Required,
+		PruneOptions: gps.PruneNestedVendorDirs,


Seems like it's not just a default but a required "base flag" that is always set and manifest cannot unset this flag?

Good catch. I could change this so that coming through dep/cmd/dep always toggle that flag in PruneOptions when passing it to SafeWriter.Write or gps.WriteDepTree.

I understood from @sdboyer that we want gps to have control over this flag, but dep should always keep it on.

ooooooh, knowing that gps may want to use it differently than dep makes this make a whole lot more sense!

I don't know what's the best way to make the change, you are much closer to this than I am. But at a high level, having a way for dep to make sure that optional is always passed in properly seems useful.

Don't let me tell you how to implement, I'm just asking questions and thinking out loud. 😃

ibrasho · 2017-08-09T05:01:48Z

@sdboyer Just to clarify and confirm the effect of each pruning option:

PruneNestedVendorOptions: Remove any vendor directory existing at any level. This is always on.
PruneNonGoFiles: Remove any file that doesn't end in .go, except for files that start with either LICENSE or COPYING. (just in case files have an extension, e.g. LICENSE.md...).
PruneGoTestFiles: Remove any files that end in _test.go.
PruneUnusedPackages: This is the annoying one. We can't delete an unused package if it contains an imported subpackage, so we do DFS and for each unimported Go package:
- if more or more sub packages are imported, delete all files but keep the folders of the imported packages.
- if no sub packages are imported, delete the directory and its content.

Finally, I always walk over baseDir and delete any empty directories. Is this necessary?

carolynvs · 2017-08-09T23:07:33Z

internal/gps/result.go

-func WriteDepTree(basedir string, l Lock, sm SourceManager, sv bool) error {
+// It requires a SourceManager to do the work, and takes a PruneOptions
+// indicating the pruning options required for the exported dependencies.
+func WriteDepTree(baseDir string, l Lock, sm SourceManager, prune PruneOptions, logger *log.Logger) error {


Oops, this looks like unmerged commits from #968 snuck in? Or did you intend to combine these PRs?

This change is needed in both PRs since we need to log what was pruned.

carolynvs · 2017-08-09T23:08:31Z

cmd/dep/godep_importer.go

-	manifest := &dep.Manifest{
-		Constraints: make(gps.ProjectConstraints),
-	}
+	manifest := dep.NewManifest()


Yay! Thank you for the constructor. 😃

ibrasho · 2017-08-11T20:28:34Z

Sorry for the slow updates. I was in quite the bind at work last week. 😩

Rebased and updated the commits. I'll be working on the tests now.
@carolynvs & @sdboyer: Let me know if you have any further comments.

ibrasho · 2017-08-11T20:30:00Z

Note: 4094432 is the result of running dep ensure with the prune options in Gopkg.toml.

ebati · 2017-08-11T21:44:54Z

internal/gps/result.go

+
+	if len(errCh) > 0 {
+		logger.Println("Failed to write dep tree. The following errors occurred:")
+		for err := range errCh {


Since channel is not closed, this will be blocked? Channel should be closed after wg.Wait() and return below should return an error since closed channel yields nil.

Good catch. 👍

ibrasho · 2017-08-11T22:58:19Z

Some questions I've:

All pruning operations are split into 2 parts: calculating files (or directories) and deleting. Is this necessary? Or can they be merged?
Should I add a new list, I'm thinking preserved-files, to allow the user to specify files that should be kept regardless on the prune options? (values should be file paths relative to vendor/)

Signed-off-by: Ibrahim AshShohail <[email protected]>

This change updates the manifest to accept a new table: prune. This table defines pruning options accepted by dep. By default, dep will always prune nested vendor directories. Three additional options can be toggled in the manifest: * non-go: Prune non-Go files (will keep LICENSE & COPYING files) * go-tests: Prune Go test files * unused-packages: Prune unused Go packages from dependencies. The implementation of these flags is not part of this commit. A constructor for dep.Manifest was also added. Signed-off-by: Ibrahim AshShohail <[email protected]>

Signed-off-by: Ibrahim AshShohail <[email protected]>

sdboyer

tbh, this is too big for me to be able to review easily 😢

is there any chance we can break this down into more incremental PRs? like, could we make just the changes to allow pruning within gps first, then in a subsequent PR get rid of prune and change ensure's behavior?

sdboyer · 2017-08-12T18:30:24Z

internal/gps/prune.go

+	return false
+}
+
+// pruneGoTestFiles deletes all Go test files (*_test.go) within baseDirr.


typo: Basedirr

sdboyer · 2017-08-15T02:49:26Z

internal/gps/prune.go

+)
+
+var (
+	// licenseFilePrefixes is a list of name prefixes for licesnse files.


typo: licesnse

sdboyer · 2017-08-15T02:52:19Z

internal/gps/result.go

 		return err
 	}

-	// TODO(sdboyer) parallelize
+	var wg sync.WaitGroup


also doing the parallelizing here? in general, would prefer to keep down the number of major changes in a single PR.

ibrasho · 2017-08-16T20:37:10Z

Closing this in favor of smaller PRs. 😁

ibrasho requested a review from sdboyer as a code owner August 4, 2017 18:08

googlebot added the cla: yes label Aug 4, 2017

ibrasho changed the title ~~[WIP] internal/gps: add PruneOptinos and WriteDepTree~~ [WIP] internal/gps: add PruneOptions and WriteDepTree Aug 4, 2017

ibrasho force-pushed the absorb-prune-into-ensure branch 3 times, most recently from df7bd86 to cb6d779 Compare August 4, 2017 18:23

ibrasho changed the title ~~[WIP] internal/gps: add PruneOptions and WriteDepTree~~ [WIP] internal/gps: add PruneOptions and update WriteDepTree Aug 4, 2017

ibrasho requested a review from carolynvs as a code owner August 5, 2017 21:52

carolynvs reviewed Aug 7, 2017

View reviewed changes

ibrasho force-pushed the absorb-prune-into-ensure branch from d762e3c to 6c30103 Compare August 7, 2017 19:56

carolynvs mentioned this pull request Aug 7, 2017

Add verbose logging option to WriteDepTree #968

Merged

ibrasho mentioned this pull request Aug 7, 2017

Add --dry-run option to dep prune #966

Closed

ibrasho force-pushed the absorb-prune-into-ensure branch 2 times, most recently from 7db1353 to 6e672a2 Compare August 8, 2017 18:52

ibrasho changed the title ~~[WIP] internal/gps: add PruneOptions and update WriteDepTree~~ internal/gps: add PruneOptions and update WriteDepTree Aug 9, 2017

ibrasho force-pushed the absorb-prune-into-ensure branch 3 times, most recently from f4bef4e to 33b99af Compare August 9, 2017 04:59

ibrasho force-pushed the absorb-prune-into-ensure branch 2 times, most recently from 83b92ab to c417150 Compare August 9, 2017 05:48

carolynvs reviewed Aug 9, 2017

View reviewed changes

carolynvs mentioned this pull request Aug 9, 2017

Add importer for github.com/LK4D4/vndr #978

Merged

ibrasho force-pushed the absorb-prune-into-ensure branch from cac5f61 to 49ea538 Compare August 11, 2017 20:21

ibrasho force-pushed the absorb-prune-into-ensure branch from 49ea538 to 4094432 Compare August 11, 2017 20:53

carolynvs approved these changes Aug 11, 2017

View reviewed changes

ebati reviewed Aug 11, 2017

View reviewed changes

ibrasho changed the title ~~internal/gps: add PruneOptions and update WriteDepTree~~ Absorb prune into ensure Aug 12, 2017

ibrasho force-pushed the absorb-prune-into-ensure branch 3 times, most recently from b46be92 to b3cd781 Compare August 12, 2017 15:33

ebati mentioned this pull request Aug 13, 2017

Fixes #1003 #1005

Merged

ibrasho added 3 commits August 13, 2017 20:33

cmd/dep: remove dep prune command

bfe75b3

Signed-off-by: Ibrahim AshShohail <[email protected]>

internal/gps: implement Prune and the related functions

40838f5

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the absorb-prune-into-ensure branch from 03db61c to 82decf7 Compare August 13, 2017 17:40

dep, internal/gps: update SafeWriter.Write and gps.WriteDepTree

7a92654

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the absorb-prune-into-ensure branch from 82decf7 to ec26c91 Compare August 13, 2017 17:44

internal/gps: update prune tests

b306f42

Signed-off-by: Ibrahim AshShohail <[email protected]>

ibrasho force-pushed the absorb-prune-into-ensure branch from ec26c91 to b306f42 Compare August 13, 2017 17:45

sdboyer suggested changes Aug 15, 2017

View reviewed changes

This was referenced Aug 16, 2017

dep: Add Manifest constructor #1019

Merged

internal/gps: Add prune functions to gps #1020

Merged

ibrasho closed this Aug 16, 2017

ibrasho mentioned this pull request Aug 16, 2017

internal/gps: parallelize WriteDepTree #1021

Merged

carolynvs mentioned this pull request Nov 27, 2017

Use default prune settings during dep init #1404

Closed

ibrasho deleted the absorb-prune-into-ensure branch November 29, 2017 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Absorb prune into ensure #952

Absorb prune into ensure #952

ibrasho commented Aug 4, 2017 •

edited

Loading

ibrasho commented Aug 5, 2017

sdboyer commented Aug 6, 2017

carolynvs Aug 7, 2017

ibrasho Aug 7, 2017

carolynvs Aug 7, 2017

ibrasho Aug 7, 2017

carolynvs Aug 7, 2017

ibrasho Aug 7, 2017

carolynvs Aug 7, 2017

carolynvs Aug 7, 2017

ibrasho Aug 7, 2017

carolynvs Aug 7, 2017

carolynvs Aug 7, 2017

ibrasho commented Aug 9, 2017 •

edited

Loading

carolynvs Aug 9, 2017

ibrasho Aug 11, 2017

carolynvs Aug 9, 2017

ibrasho commented Aug 11, 2017

ibrasho commented Aug 11, 2017 •

edited

Loading

ebati Aug 11, 2017

ibrasho Aug 13, 2017

ibrasho commented Aug 11, 2017

sdboyer left a comment

sdboyer Aug 12, 2017

sdboyer Aug 15, 2017

sdboyer Aug 15, 2017

ibrasho commented Aug 16, 2017

Absorb prune into ensure #952

Absorb prune into ensure #952

Conversation

ibrasho commented Aug 4, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

ibrasho commented Aug 5, 2017

sdboyer commented Aug 6, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho commented Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho commented Aug 11, 2017

ibrasho commented Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho commented Aug 11, 2017

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

ibrasho commented Aug 16, 2017

ibrasho commented Aug 4, 2017 •

edited

Loading

ibrasho commented Aug 9, 2017 •

edited

Loading

ibrasho commented Aug 11, 2017 •

edited

Loading