gps: Refactor prune and filesystem functions #1219

ibrasho · 2017-09-24T15:55:16Z

What does this do / why do we need it?

This PR moves filesystemState from being a test struct to become part of gps. It's used to optimize the prune function by calculating the filesystem state and determining the files to prune in-memory.

addition: This PR also includes the remaining plumbing required to make prune part of ensure.

What should your reviewer look out for in this PR?

Correctness and optimizations as usual. 😁

Do you need help or clarification on anything?

Any suggestion on how to handle striping vendor directories?

Which issue(s) does this PR fix?

One more step towards #944

ibrasho · 2017-09-24T16:58:04Z

internal/gps/filesystem.go

+
+// calculateFilesystemState returns a filesystemState based on the state of
+// the filesystem on root.
+func calculateFilesystemState(root string) (filesystemState, error) {


I've been using calculate a lot in this context but I'm not very comfortable with it. Any better suggestions?

deriveFilesystemState, analyzeFilesystem, or even just newFilesystemState?

sdboyer · 2017-09-24T17:00:04Z

maybe "derive"?

…

On September 24, 2017 12:58:04 PM EDT, Ibrahim AshShohail ***@***.***> wrote: ibrasho commented on this pull request. > +type filesystemState struct { + root string + dirs []string + files []string + links []fsLink +} + +// fsLink represents a symbolic link. +type fsLink struct { + path string + to string +} + +// calculateFilesystemState returns a filesystemState based on the state of +// the filesystem on root. +func calculateFilesystemState(root string) (filesystemState, error) { I've been using `calculate` a lot in this context but I'm not very comfortable with it. Any better suggestions? -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #1219 (review)

jmank88

LGTM

I wonder how the API would feel with these functions as methods on filesystemState instead.

I also noticed some of the logger args are unused, if you want to clean up while you're in here.

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

I wonder how the API would feel with these functions as methods on filesystemState instead.

Hmm... 🤔 This could look nicer, but I didn't want to couple filesystemState to prune. I started by creating a new struct with methods, but then opted to use the current filesystemState with some modification.

I also noticed some of the logger args are unused, if you want to clean up while you're in here.

I actually planned to use those to log all files that will be deleted. This was added in the last push.

But I'm a bit reluctant now about logging all these lines.
This will print a lot of lines for big projects. Which might make verbose output very verbose.

sdboyer · 2017-10-01T05:38:25Z

internal/gps/prune.go

@@ -107,152 +111,141 @@ func pruneNestedVendorDirs(baseDir string) error {
 	return filepath.Walk(baseDir, stripVendor)
 }

-// pruneUnusedPackages deletes unimported packages found within baseDir.
+// pruneVendorDirs deletes all nested vendor directories within baseDir.


why keep the commented funcs?

That's why this is a [WIP]. 😁

I'm trying to refactor strip vendor funcs to use the filesystemState.

sdboyer · 2017-10-01T05:41:26Z

internal/gps/prune.go

+	for _, path := range toDelete {
+		logger.Printf("  * %s", path)
+
+		if err := os.Remove(path); err != nil && !os.IsNotExist(err) {


how are we ordering the deletion of directories such that we don't leave unnecessary empty ones around, but still do it in the correct order so that directories are only removed once all of their contents have been removed?

This is one of the most annoying parts of pruning for me.

I opted to only delete files in the new prune functions, and empty directories are kept behind.
I think empty directories are a minor issue atm since git and hg ignore empty directories by default (not sure how bzr handles them).

We'll need to replace filesystemState with a hierarchical representation to allow determining which directories are empty efficiently or come up with a different solution.

We'll need to replace filesystemState with a hierarchical representation to allow determining which directories are empty efficiently or come up with a different solution.

If the paths are normalized, can the list just be sorted so that everything is deleted bottom-up? Then whenever the next item is a new dir, empty-check the previous.

Dirs are not added to the list currently. The logic to determine when to add a dir to the list will be more involved with the current struct.

They don't need to be in the list for what I was describing. I just meant that if the next file is in a different dir then the previous file, then it was the last file to delete from that dir (assuming a proper normalized sort), so empty-check it (and possibly its parent, and so on). This doesn't account for directories which were already empty before any pruning, but that seems like a weird edge case.

I think empty directories as a minor issue atm

we may be able to skate by in an initial implementation without them because of git/hg's treatment of empty dirs, but i suspect we'll need to catch up rather quickly. beyond the general annoyance empty dirs will cause for folks, i suspect that correct implementation of #1113 will also demand that we do something a little more sophisticated here, anyway.

i don't have a clear implementation in mind yet, but i have some vague ideas swirling around regarding prefix-based iteration/exploration of a trie.

I've 2 approaches in mind to do this:

Loop through all dirs in the filesystemState struct and check if each dir is empty or not. This can be done once after all options are done, or after each option. I prefer doing it once at the end.

After each option deletes its files, determine the touched dirs from the slice containing the toDelete files, and loop through only those dirs and check for emptiness.

The key operation that I'm trying to minimize is opening the file to use os.File.Readdirname.
I can keep looping over the slices to find files in a dir, but a prefix tree would make that easier. Giving filesystemState hierarchy could be even better and more specialized.

But I'm trying to resist the temptation to change filesystemState to a more complex struct that provides more functionality by finding a simpler solution.

just off the cuff, but maybe it's as simple as a sort function, where records represent dirs and their contents, and dirs that are subpaths of other dir records are moved to the front? that way you can proceed linearly through the sorted slice, assured that you will never visit a dir before any of its ancestors.

sdboyer · 2017-10-01T20:13:58Z

@jmank88 would you mind dismissing your review until this is no longer WIP?

WIP

We now delete anything that looks like a symlink if its called "vendor" while pruning. Hopefully, you didn't make a bad decision by relying on the magical properties of symlinks. Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho · 2017-12-11T13:44:06Z

??? Legal files remain in some otherwise empty nested directories. For example with project github.com/golang/freetype (packages = [".","raster","truetype"]), vendor/github.com/golang/freetype/testdata/COPYING is not pruned, which is a change from the behavior of dep ensure && dep prune on master.

I discussed this with @sdboyer and this is the approach we want to keep just because we can never be sure about the scope of those files.

👎 Pruned packages leave behind an empty directory.

@ChrisHines This should be solved now. Could you please try? If this still doesn't keep some directories (and it doesn't fail in the process) I would really appreciate an example project where this occurs.

ChrisHines · 2017-12-12T01:25:26Z

@ibrasho Pruning unused packages still leaves empty directories for pruned packages that are sub-directories of a bigger repository. Here is a tiny reproducer for you: https://github.com/ChrisHines/dep-test. If you clone that repo and run dep ensure with the current revision of this PR you should see many empty directories left behind.

When I do it on my machine I see the following empty directories under vendor:

vendor\github.com\ajstarks\svgo\float
vendor\github.com\golang\freetype\cmd
vendor\github.com\golang\freetype\example
vendor\github.com\llgcode\draw2d\output
vendor\github.com\llgcode\draw2d\samples
vendor\golang.org\x\image\cmd
vendor\golang.org\x\image\example
vendor\golang.org\x\image\font\gofont
vendor\golang.org\x\image\font\testdata
vendor\golang.org\x\image\webp
vendor\gonum.org\v1\plot\internal\cmpimg
vendor\gonum.org\v1\plot\palette\moreland
vendor\gonum.org\v1\plot\plotter
vendor\gonum.org\v1\plot\tools
vendor\gonum.org\v1\plot\vg\vgtex

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

sdboyer · 2017-12-12T13:05:26Z

can we feasibly add a test case to cover what's represented by @ChrisHines repository example there?

ibrasho · 2017-12-12T18:04:56Z

@ChrisHines Thanks for the repo. Found the issue and fixed it now. Turns out the nested empty dirs were not handled properly.

@sdboyer Sure. The basic issue was very simple and a small example can cover it.

ChrisHines · 2017-12-13T02:42:15Z

@ibrasho It looks like it's working perfectly now. I don't see any more problems.

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

sdboyer · 2017-12-15T03:17:12Z

wow. codeclimate has gotta go.

ibrasho · 2017-12-15T17:50:33Z

@sdboyer Any update on the fix from their side?

sdboyer · 2017-12-15T18:30:17Z

no and now they've got all these additional issues that are much less clear to fix - e.g. the code duplication complaints for table based tests. I don't see there being an easy solution there; I'd much rather we just move back to something strictly for code coverage. I think I was using codecov before with gps.

alexkay · 2017-12-15T22:14:54Z

@ibrasho (and others!), than you so much for this PR! I'm moving a decently sized Go monorepo at $WORK from using godep to dep, and this change allowed us to keep the ./vendor diff minimal.

One edge case I'm seeing: Some larger libraries have license and legal files not only in the root, but also in the nested packages. We should probably delete such files if the nested package is not otherwise used.

An example library if you need it for troubleshooting is git.apache.org/thrift.git.

Gopkg.toml fragment:

[[constraint]]
  name = "git.apache.org/thrift.git"
  revision = "fe9222a6ec20d23d9cfd3ec9c793887f7212b313"

Gopkg.lock fragment:

[[projects]]
  name = "git.apache.org/thrift.git"
  packages = ["lib/go/thrift"]
  revision = "fe9222a6ec20d23d9cfd3ec9c793887f7212b313"

find vendor/git.apache.org/thrift.git -type f fragment, after running dep ensure:

vendor/git.apache.org/thrift.git/contrib/fb303/LICENSE
vendor/git.apache.org/thrift.git/debian/copyright
vendor/git.apache.org/thrift.git/LICENSE
vendor/git.apache.org/thrift.git/NOTICE
vendor/git.apache.org/thrift.git/lib/dart/LICENSE_HEADER
vendor/git.apache.org/thrift.git/lib/go/thrift/socket.go
vendor/git.apache.org/thrift.git/lib/go/thrift/exception.go
(many more .go files)
vendor/git.apache.org/thrift.git/lib/go/thrift/messagetype.go
vendor/git.apache.org/thrift.git/lib/hs/LICENSE

sdboyer · 2017-12-15T23:17:43Z

@alexkay so glad this PR is helping you make the jump! I realize it can be a bit annoying, but because there is no well-defined relationship between legal files and the code they are intended to cover, in order to err on the side of not pissing off lawyers unnecessarily, we're opting to just always keep those files.

carolynvs · 2017-12-18T18:32:35Z

I have updated the target branch for this PR to the prune-release-prep branch.

sdboyer · 2017-12-18T18:44:13Z

ok, we're good on this - pulling it into the release prep branch! 🎉 🎉

glerchundi · 2018-01-05T10:44:48Z

such a great job!

@sdboyer which is the ETA for the next release including these changes?

sdboyer · 2018-01-05T11:42:14Z

I've been trying to get a release out for a couple weeks now 😢. everything's currently held up on docs. i have an opportunity to blitz those on a plane this weekend, so I'm hoping... next week.

…

On January 5, 2018 5:44:49 AM EST, Gorka Lerchundi Osa ***@***.***> wrote: such a great job! @sdboyer which is the ETA for the next release including these changes? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1219 (comment)

glerchundi · 2018-01-05T12:57:04Z

don't worry, would be a huge win so it's worth waiting! 👍

TobiasBales · 2018-02-02T07:00:26Z

Did this get released yet? I can't seem to find it

sdboyer · 2018-02-02T07:02:40Z

yes, it now happens automatically when you set appropriate values in Gopkg.toml. more info in release notes and on the new docs site and in the release announcement

ibrasho added the area: gps label Sep 24, 2017

ibrasho self-assigned this Sep 24, 2017

ibrasho requested a review from sdboyer as a code owner September 24, 2017 15:55

googlebot added the cla: yes label Sep 24, 2017

ibrasho force-pushed the optimize-gps-prune branch from 6f898f0 to 07a3587 Compare September 24, 2017 16:04

ibrasho changed the title ~~internal/gps: refactor prune and filesystem functions~~ [WIP] internal/gps: refactor prune and filesystem functions Sep 24, 2017

ibrasho force-pushed the optimize-gps-prune branch 2 times, most recently from 4e92699 to 4db54a5 Compare September 24, 2017 16:46

ibrasho commented Sep 24, 2017

View reviewed changes

jmank88 previously approved these changes Sep 28, 2017

View reviewed changes

ibrasho force-pushed the optimize-gps-prune branch 3 times, most recently from b76ed0f to 4d7004b Compare September 29, 2017 09:05

ibrasho force-pushed the optimize-gps-prune branch 11 times, most recently from c4d36a0 to 92e138a Compare September 30, 2017 08:09

sdboyer previously requested changes Oct 1, 2017

View reviewed changes

ibrasho force-pushed the optimize-gps-prune branch from 92e138a to 12a1b64 Compare October 6, 2017 17:44

ChrisHines mentioned this pull request Dec 9, 2017

dep: WARNING: Unknown field in manifest: prune #1442

Closed

ibrasho force-pushed the optimize-gps-prune branch 2 times, most recently from a6ad0cc to b50a8b5 Compare December 10, 2017 10:59

gps: handle symlinks properly

b4fca9b

We now delete anything that looks like a symlink if its called "vendor" while pruning. Hopefully, you didn't make a bad decision by relying on the magical properties of symlinks. Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho force-pushed the optimize-gps-prune branch from b50a8b5 to b4fca9b Compare December 11, 2017 13:35

gps: fix a bug that kept empty dirs after pruning

c7c3fd8

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

gps: add tests for deleteEmptyDirs

cef261f

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

ibrasho mentioned this pull request Dec 16, 2017

Restore prune docs #1456

Merged

carolynvs mentioned this pull request Dec 18, 2017

Add default prune options for init cmd #1460

Merged

carolynvs changed the base branch from master to prune-release-prep December 18, 2017 18:32

sdboyer merged commit bf5b9fc into golang:prune-release-prep Dec 18, 2017

ldez mentioned this pull request Jan 9, 2018

Switch to golang/dep. traefik/traefik#2681

Merged

1 task

ibrasho mentioned this pull request Jan 24, 2018

dep: update TestReadManifest to actually check prune options #1567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gps: Refactor prune and filesystem functions #1219

gps: Refactor prune and filesystem functions #1219

ibrasho commented Sep 24, 2017 •

edited

Loading

ibrasho Sep 24, 2017

jmank88 Sep 28, 2017 •

edited

Loading

sdboyer commented Sep 24, 2017 via email

jmank88 left a comment

ibrasho commented Sep 29, 2017 •

edited

Loading

sdboyer Oct 1, 2017

ibrasho Oct 1, 2017 •

edited

Loading

sdboyer Oct 1, 2017

ibrasho Oct 1, 2017 •

edited

Loading

jmank88 Oct 1, 2017

ibrasho Oct 1, 2017

jmank88 Oct 1, 2017 •

edited

Loading

sdboyer Oct 1, 2017

ibrasho Oct 6, 2017

sdboyer Oct 7, 2017

sdboyer commented Oct 1, 2017

ibrasho commented Dec 11, 2017 •

edited

Loading

ChrisHines commented Dec 12, 2017

sdboyer commented Dec 12, 2017

ibrasho commented Dec 12, 2017

ChrisHines commented Dec 13, 2017

sdboyer commented Dec 15, 2017

ibrasho commented Dec 15, 2017

sdboyer commented Dec 15, 2017

alexkay commented Dec 15, 2017 •

edited

Loading

sdboyer commented Dec 15, 2017

carolynvs commented Dec 18, 2017

sdboyer commented Dec 18, 2017

glerchundi commented Jan 5, 2018

sdboyer commented Jan 5, 2018 via email

glerchundi commented Jan 5, 2018

TobiasBales commented Feb 2, 2018

sdboyer commented Feb 2, 2018

gps: Refactor prune and filesystem functions #1219

gps: Refactor prune and filesystem functions #1219

Conversation

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

Choose a reason for hiding this comment

jmank88 Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

sdboyer commented Sep 24, 2017 via email

jmank88 left a comment

Choose a reason for hiding this comment

ibrasho commented Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

ibrasho Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmank88 Oct 1, 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

sdboyer commented Oct 1, 2017

ibrasho commented Dec 11, 2017 • edited Loading

ChrisHines commented Dec 12, 2017

sdboyer commented Dec 12, 2017

ibrasho commented Dec 12, 2017

ChrisHines commented Dec 13, 2017

sdboyer commented Dec 15, 2017

ibrasho commented Dec 15, 2017

sdboyer commented Dec 15, 2017

alexkay commented Dec 15, 2017 • edited Loading

sdboyer commented Dec 15, 2017

carolynvs commented Dec 18, 2017

sdboyer commented Dec 18, 2017

glerchundi commented Jan 5, 2018

sdboyer commented Jan 5, 2018 via email

glerchundi commented Jan 5, 2018

TobiasBales commented Feb 2, 2018

sdboyer commented Feb 2, 2018

ibrasho commented Sep 24, 2017 •

edited

Loading

jmank88 Sep 28, 2017 •

edited

Loading

ibrasho commented Sep 29, 2017 •

edited

Loading

ibrasho Oct 1, 2017 •

edited

Loading

ibrasho Oct 1, 2017 •

edited

Loading

jmank88 Oct 1, 2017 •

edited

Loading

ibrasho commented Dec 11, 2017 •

edited

Loading

alexkay commented Dec 15, 2017 •

edited

Loading