internal/gps: Add prune functions to gps #1020

ibrasho · 2017-08-16T20:35:41Z

What does this do / why do we need it?

Add different functions to gps that implement different pruning modes.

The different modes are as follows:

pruneNestedVendorDirs: Delete any directory named vendor.
pruneNonGoFiles: Delete any file that doesn't end with .go (will keep files matching common names for legal documents)
pruneGoTestFiles: Prune Go test files (any file ending with _test.go)
pruneUnusedPackages: Delete all files in unused Go packages. Since we might import a sub package we only delete files and keep directories.

What should your reviewer look out for in this PR?

Correctness.

Do you need help or clarification on anything?

Refer to https://github.com/sgotti/glide-vc/ for comparison and ideas.

Is adding an option to allow overriding deletion of specific files a good idea?

Which issue(s) does this PR fix?

Breaking down #952

jmank88

Looks correct. Just a few nits.

jmank88 · 2017-08-17T10:57:13Z

internal/gps/prune.go

+
+	if err := pruneEmptyDirs(baseDir, logger); err != nil {
+		return errors.Wrap(err, "failed to prune empty dirs")
+	}


Is this one extra, since there is one at the end?

jmank88 · 2017-08-17T11:01:11Z

internal/gps/prune.go

+// on the PruneOptions passed.
+//
+// A Lock must be passed if PruneUnusedPackages is toggled on.
+func Prune(baseDir string, options PruneOptions, l Lock, logger *log.Logger) error {


How fast is this? Would it be worthwhile to try to do some of the filepath.Walks concurrently and/or combine them?

I already asked about this in #952. I believe that we can merge them but I was wondering why they were separated in the first place.

Also, I still haven't done any benchmarking on this yet. I wanted to ensure that the behavior is correct first.

we'll likely, though not certainly, be able to interlace the logic eventually - but i want to take it one step at a time. verification needs to be integrated first, which opens the door to partial writes of the tree. combining that with pruning will be fairly complicated, and keeping pruning standalone initially will help us walk before we run.

I'm not sure we are talking about the same thing. I was, and I assume @jmank88, talking about the fact that we do multiple filepath.Walk per prune option. Can we do one filepath.Walk and calculate all the files to be pruned?

ohhhhh, i see. sorry, i understand now. yeah, definitely different things.

ok, so, yeah, we're definitely going to need to combine the filepath.Walk() operations. maybe it's OK not to do it for this first PR, but the cost of even one walk will be order(s) of magnitude higher than any of the actual ops being performed within any of the walks.

jmank88 · 2017-08-17T11:07:40Z

internal/gps/prune.go

+		if err := os.Remove(dir); err != nil {
+			return err
+		}
+	}


Could call return deleteFiles(empty). Otherwise, can the loops be combined?

It should have been one loop.

jmank88 · 2017-08-17T11:08:12Z

internal/gps/prune.go

+
+	if err := deleteFiles(files); err != nil {
+		return err
+	}


Could just return deleteFiles(files).

jmank88 · 2017-08-17T11:08:30Z

internal/gps/prune.go

+
+	if err := deleteFiles(files); err != nil {
+		return err
+	}


Could just return deleteFiles(files).

sdboyer · 2017-08-28T02:11:09Z

internal/gps/prune.go

+// on the PruneOptions passed.
+//
+// A Lock must be passed if PruneUnusedPackages is toggled on.
+func Prune(baseDir string, options PruneOptions, l Lock, logger *log.Logger) error {


ok, so here's one big thing: this is currently focused on an entire dep/vendor tree. we need to split it up, so that it instead deals with a single project. this is arguably a better design in general, but whether you agree with that or not, our Gopkg.toml-driven pruning implementation will need to allow different pruning options to be specified globally and per dep. that means calling this func with different PruneOptions.

in terms of the interface, i think it may be as simple as changing from taking an l Lock to an lp LockedProject.

I think doing per-project prune options is a good thing, but I'm wondering what implications it might have.

I'll take an attempt at that tomorrow to see what I can do.

i think the implications should be fairly contained, but i've not thought it all the way through. hopefully not too bad 😄

ibrasho · 2017-09-02T07:01:56Z

I'm back to working on this. Should update it today.

ibrasho · 2017-09-05T19:10:34Z

@carolynvs Added some simple tests now.

carolynvs · 2017-09-05T20:43:50Z

internal/gps/prune_test.go

+			false,
+		},
+		{
+			"nested-package",


aaronbee · 2017-09-07T18:55:34Z

internal/gps/prune.go

+
+var (
+	// licenseFilePrefixes is a list of name prefixes for license files.
+	licenseFilePrefixes = []string{


I'd suggest you include "contributors" and "authors" to the file list here or in legalFileSubstrings. These files are legal in nature. For example, dep's own AUTHORS file starts with

This source code refers to The Go Authors for copyright purposes.

sdboyer · 2017-09-09T03:41:13Z

internal/gps/prune.go

+	// PruneUnusedPackages indicates if unused Go packages should be pruned.
+	PruneUnusedPackages
+	// PruneNonGoFiles indicates if non-Go files should be pruned.
+	// LICENSE & COPYING files are kept for convience.


need to update this note to include the contributors & authors files. also, not for convenience, but "to try to comply with legal requirements".

sdboyer · 2017-09-09T04:00:20Z

internal/gps/prune.go

+
+// pruneProject remove excess files according to the options passed, from
+// the lp directory in baseDir.
+func pruneProject(baseDir string, lp LockedProject, options PruneOptions, logger *log.Logger) error {


i think it's worth exporting this. even if we don't have immediate plans to use it in dep, it just feels shortsighted to only expose the top-level call.

sdboyer · 2017-09-09T04:09:44Z

internal/gps/prune.go

+		if err != nil {
+			return errors.Wrap(err, "unexpected error while calculating unused packages")
+		}
+		fmt.Println(pkg)


debugging artifact?

sdboyer · 2017-09-09T04:16:20Z

internal/gps/prune.go

+	return files, err
+}
+
+// isPreservedFile checks if the file name idicates that the file should be


comment needs cleaning up, it seems duplicated right now. also s/idicates/indicates/

sdboyer · 2017-09-09T04:18:02Z

internal/gps/prune.go

+func collectUnusedPackagesFiles(projectDir string, unusedPackages map[string]struct{}) ([]string, error) {
+	// TODO(ibrasho): is this useful?
+	files := make([]string, 0, len(unusedPackages))
+	fmt.Println(unusedPackages)


debugging artifact

sdboyer · 2017-09-09T04:26:00Z

internal/gps/prune.go

+
+// collectGoTestsFile returns a slice contains all Go test files (any files
+// prefixed with _test.go) in baseDir.
+func collectGoTestsFile(baseDir string) ([]string, error) {


nit: s/TestsFile/TestFiles/

sdboyer · 2017-09-09T17:47:34Z

internal/gps/prune.go

+// on the PruneOptions passed.
+//
+// A Lock must be passed if PruneUnusedPackages is toggled on.
+func Prune(baseDir string, options PruneOptions, l Lock, logger *log.Logger) error {


ok, so, yeah, we're definitely going to need to combine the filepath.Walk() operations. maybe it's OK not to do it for this first PR, but the cost of even one walk will be order(s) of magnitude higher than any of the actual ops being performed within any of the walks.

sdboyer · 2017-09-09T17:55:11Z

internal/gps/strip_vendor.go

@@ -16,29 +16,27 @@ func stripVendor(path string, info os.FileInfo, err error) error {
 		return err
 	}

-	if info.Name() == "vendor" {
-		if _, err := os.Lstat(path); err != nil {


why is it OK to drop this lstat?

It was pretty much not doing anything. lstat has the same error responses as stat. If we already have a FileInfo then stat has succeeded.

Anyhow, I can restore it if we are unsure and don't want to break anything.

cc: @spenczar Any idea what's the original intent of this?

@ibrasho Just checking in to confirm that the original purpose of this got lost after a few rewrites of the surrounding code (including my own rewrites!). Your fix here looks correct to me. 👍

ibrasho · 2017-09-09T20:24:42Z

@sdboyer
To improve performance, I was thinking of doing a single filepath.Walk to collect file and directory names and then running the operations on that list.

All the current rules are based on file names anyway so stating multiple times is wasteful.

sdboyer · 2017-09-10T01:48:58Z

@ibrasho if you think that general approach is feasible, then i'd say it's vastly preferable.

seems like it might be good to reuse dirwalk code for this purpose, once #1084 is done.

sdboyer · 2017-09-18T02:43:39Z

kicking CI to retest, that last one seems like an error?

i'd like to merge this soon, even if it's sorta WIP, as i think we can improve as we go - in particular, i don't think we need to fix the filepath.Walk() duplication right now. however, i do want to get the various smaller cleanup issues addressed, as it seems quite unlikely that we'll remember to grab those in a later PR.

ibrasho · 2017-09-20T18:30:46Z

I've pushed a patch of changes.

I'll keep the changes related to deduplicating filepath.Walk for another PR.

sdboyer · 2017-09-20T23:26:05Z

ahh, some new linting errors, as we added some linters - once we have fixes for those, i think we should be ready to go with this.

ibrasho · 2017-09-21T19:41:54Z

I'm not sure these linter errors are valid...

/home/travis/gopath/src/github.com/golang/dep/internal/gps/vcs_source.go:379:2: redundant if ...; err != nil check, just return error instead.
/home/travis/gopath/src/github.com/golang/dep/internal/gps/vcs_source.go:460:2: redundant if ...; err != nil check, just return error instead.
internal/gps/prune.go:21:2: const PruneNestedVendorDirs is unused (U1000)
internal/gps/prune.go:23:2: const PruneUnusedPackages is unused (U1000)
internal/gps/prune.go:27:2: const PruneNonGoFiles is unused (U1000)
internal/gps/prune.go:29:2: const PruneGoTestFiles is unused (U1000)
internal/gps/prune.go:60:6: func Prune is unused (U1000)
internal/gps/prune.go:75:6: func PruneProject is unused (U1000)
internal/gps/prune.go:106:6: func pruneNestedVendorDirs is unused (U1000)

sdboyer · 2017-09-22T00:44:17Z

ahhhhh so the errors are at least in part because this predates #1157, which changed some of the logic. however, there is one valid complaint in there, which actually points to a real bug:

internal/gps/prune.go:162:19: ToSlash is a pure function but its return value is ignored (SA4017)

once we address that, we can merge - the other complaints will go away. (if we want to be really sure, though, we can rebase)

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

ibrasho · 2017-09-22T06:20:34Z

I just added that a few hours ago because calculateUnusedPackages had a bug on Windows, but forgot to assign it to pkg. 😓

I've rebased. Should be fixed and ready to merge now.

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

sdboyer · 2017-09-22T13:05:08Z

ok so it really seems like those travis linting failures must be erroneous, even though i can't immediately figure out how that's possible post-rebase. merging this now, and we'll keep an eye on subsequent PRs to see if we have a real problem or not.

ibrasho requested a review from sdboyer as a code owner August 16, 2017 20:35

googlebot added the cla: yes label Aug 16, 2017

ibrasho force-pushed the add-prune-to-gps branch from cf31c33 to dcd5efc Compare August 16, 2017 20:35

jmank88 suggested changes Aug 17, 2017

View reviewed changes

ibrasho mentioned this pull request Aug 17, 2017

Invalid memory address or nil pointer dereference when running dep ensure #1022

Closed

ibrasho force-pushed the add-prune-to-gps branch 3 times, most recently from f0ff2f5 to 6a9bc1e Compare August 18, 2017 11:38

sdboyer suggested changes Aug 28, 2017

View reviewed changes

ibrasho mentioned this pull request Sep 2, 2017

Prune should remove contents of root package directory when only subpackages are used #1113

Closed

ibrasho force-pushed the add-prune-to-gps branch 2 times, most recently from 9bd639a to ef5d7a6 Compare September 5, 2017 19:10

carolynvs reviewed Sep 5, 2017

View reviewed changes

internal/gps/prune_test.go

false,

},

{

"nested-package",

Copy link

Collaborator

carolynvs Sep 5, 2017

Choose a reason for hiding this comment

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

🎉

jmank88 approved these changes Sep 5, 2017

View reviewed changes

ibrasho force-pushed the add-prune-to-gps branch from ef5d7a6 to 3492a04 Compare September 5, 2017 22:09

aaronbee reviewed Sep 7, 2017

View reviewed changes

sdboyer suggested changes Sep 9, 2017

View reviewed changes

ibrasho added the area: gps label Sep 9, 2017

ibrasho self-assigned this Sep 9, 2017

darkowlzz mentioned this pull request Sep 14, 2017

"dep ensure -update foo/bar/baz && dep prune" takes ages to complete #1170

Closed

sdboyer closed this Sep 18, 2017

sdboyer reopened this Sep 18, 2017

ibrasho force-pushed the add-prune-to-gps branch from bc90972 to 3690bd8 Compare September 20, 2017 18:29

ibrasho force-pushed the add-prune-to-gps branch from 3690bd8 to 6b8ca98 Compare September 20, 2017 18:32

ibrasho force-pushed the add-prune-to-gps branch from 6b8ca98 to e79c0bd Compare September 20, 2017 18:33

ibrasho force-pushed the add-prune-to-gps branch 3 times, most recently from 56acbf0 to 45126f4 Compare September 21, 2017 22:07

ibrasho added 4 commits September 22, 2017 09:18

internal/gps: Add prune functions to gps

1315bd8

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

internal/gps: update stripVendor functions

b14d646

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

internal/gps: add pruneProject func

feac5a4

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

Add AUTHORS and CONTRIBUTERS to legal files list

98dd01e

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

ibrasho force-pushed the add-prune-to-gps branch from 45126f4 to 24bf56f Compare September 22, 2017 06:19

ibrasho force-pushed the add-prune-to-gps branch from 8ad6e0e to 2b7d61b Compare September 22, 2017 06:40

internal/gps: export PruneProject and fix minor nits

9b6892c

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

ibrasho force-pushed the add-prune-to-gps branch from 2b7d61b to 9b6892c Compare September 22, 2017 06:47

sdboyer approved these changes Sep 22, 2017

View reviewed changes

sdboyer merged commit f749c0c into golang:master Sep 22, 2017

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

internal/gps: Add prune functions to gps #1020

internal/gps: Add prune functions to gps #1020

Conversation

ibrasho commented Aug 16, 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?

jmank88 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 Aug 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho commented Sep 2, 2017 • edited Loading

ibrasho commented Sep 5, 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

Choose a reason for hiding this comment

spenczar Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

ibrasho commented Sep 9, 2017

sdboyer commented Sep 10, 2017

sdboyer commented Sep 18, 2017

ibrasho commented Sep 20, 2017 • edited Loading

sdboyer commented Sep 20, 2017

ibrasho commented Sep 21, 2017

sdboyer commented Sep 22, 2017 • edited Loading

ibrasho commented Sep 22, 2017 • edited Loading

sdboyer commented Sep 22, 2017 • edited Loading

ibrasho commented Aug 16, 2017 •

edited

Loading

ibrasho Aug 17, 2017 •

edited

Loading

ibrasho commented Sep 2, 2017 •

edited

Loading

spenczar Sep 11, 2017 •

edited

Loading

ibrasho commented Sep 20, 2017 •

edited

Loading

sdboyer commented Sep 22, 2017 •

edited

Loading

ibrasho commented Sep 22, 2017 •

edited

Loading

sdboyer commented Sep 22, 2017 •

edited

Loading