Use glide list #21

FugiTech · 2016-09-08T12:06:24Z

I noticed in #14 and #20 you stated that you wished to use glide list to determine what packages to keep rather than glide.lock. This is my attempt to do so.

The only effect I've noticed that this has is that files in the repository root are deleted if they aren't imported, which was my goal.

sgotti · 2016-09-09T07:35:07Z

I noticed in #14 and #20 you stated that you wished to use glide list to determine what packages to keep rather than glide.lock. This is my attempt to do so.

@Fugiman Thanks a lot for your PR! Some notes:
My original idea (as also proposed by @mattfarina in Masterminds/glide#314 (comment)) will be to just call glide list and parse its json output. I'd really like to avoid using and vendoring great part of the glide code (and having to keep it in sync with glide) just for the list function.

The only effect I've noticed that this has is that files in the repository root are deleted if they aren't imported, which was my goal.

That's good since with glide list we can now if the root repo source files are needed (not possible with glide.lock, see Masterminds/glide#314).
On the other side there's the need to know the repository root to keep legal files (when no --no-legal-files is provided). So, as explained in #14, since glide list doesn't provide the root repo information we could get it from the glide.lock (this will also require choosing what to do when a glide list provided repo isn't available in glide.lock probably due to an old/unsynced glide.lock). Another solution will be to add this information to the glide list output (if we are not going to use the glide code).

FugiTech · 2016-09-09T08:02:33Z

just call glide list and parse its json output

Ah, excellent. I avoided that approach since it seemed like you already took the "vendor glide" approach, but I can change this PR to do that.

On the other side there's the need to know the repository root to keep legal files

I believe this PR addresses that concern. It does so by keeping any legal files contained in a directory that is a parent of an imported package, or the package itself. This seems like a simpler solution than trying to determine the repository root, and would only have the side effect of including more legal files than perhaps strictly necessary, which didn't seem too bad to me?

sgotti · 2016-09-09T09:50:31Z

Ah, excellent. I avoided that approach since it seemed like you already took the "vendor glide" approach

Yeah, I tried to import the less as possible to parse glide.lock, so perhaps with this PR also this part could be removed.

but I can change this PR to do that.

Great!

I believe this PR addresses that concern. It does so by keeping any legal files contained in a directory that is a parent of an imported package, or the package itself. This seems like a simpler solution than trying to determine the repository root, and would only have the side effect of including more legal files than perhaps strictly necessary, which didn't seem too bad to me?

Seems good!

FugiTech · 2016-09-10T03:39:31Z

I believe I've addressed your concerns, but unfortunately most of the diff is now deleting vendored code. If this is a problem I'd appreciate suggestions on how to make it more readable :)

sgotti · 2016-09-10T07:05:07Z

gvc.go


 		keep := false
-		for _, name := range pkgList {
-			// If the file's parent directory is a needed package, keep it.


I like how you refactored this part. 👍

sgotti · 2016-09-10T07:37:57Z

@Fugiman I tried it and it worked well! 👍

But I noticed it'll break projects that uses the vendored dirs for keeping additional not needed packages. For example coreos/rkt puts additional vendored programs and builds them (actool, cni plugins). That's probably not a standard practice but sometimes it's needed.
In the past rkt used a dummy.go files with all the imports required to keep that packages, now it relies on the glide.lock (since if you explicitly define a dep in the glide.yaml it'll be vendored also if not required by the code). But glide list will not display these deps (since they're not needed).

I can see two solutions:

Provide an option to fallback to glide.lock instead of glide list output (this can create confusion to the users since there're two different logics).
Change rkt (and suggest this somewhere for other projects) to reintroduce the dummy.go file.

@s-urbaniak thoughts?

but unfortunately most of the diff is now deleting vendored code. If this is a problem I'd appreciate suggestions on how to make it more readable :)

For clearness I usually split the code changes and the vendor changes in two different commits. But the vendor changes could also become a follow up PR.

Additionally there's also the need to change the README.md since it says that glide-vc reads the glide.lock file.

FugiTech · 2016-09-10T08:24:15Z

It feels like using glide to manage tools is incorrect behavior. I previously tried to do this myself and ran into a lot of issues, and my understanding of gps is that it'll make glide ignore packages in glide.yaml that aren't imported, so this will continue to be a problem in the future? I may be incorrect on that part.

My feeling is that glide is really good at managing dependencies for a repository and should focus on that, and another tool should manage vendoring tools. Then again, we built such a tool at my company and are working on open sourcing it, so I may be biased :) In the short term, I think the dummy.go approach is the easiest to understand and the most straightforward to implement.

I'll move the vendor changes to a different PR and update the README

s-urbaniak · 2016-09-12T13:30:04Z

Introducing a dummy.go sounds at least equally wrong as using glide to pull in tools in a specific version, it is doing the same thing, just transitively.

If glide is converging into this direction, then we will have to adapt in rkt, but I'd like to find a better solution other than dummy.go ;-) Introducing yet another tool (we already have two, glide, and glide-vc) to just pull tools in a specific version might be a solution.

@Fugiman I am not aware of gps, can you give some pointers?

FugiTech · 2016-09-12T17:28:00Z

gps (found here) is a package that will eventually decide which versions of each package glide needs to put in the vendor directory. It seems to care deeply about the project and it's imports, so it may change glide's behavior in regards to unimported packages in glide.yaml but I'm not sure.

sdboyer · 2016-09-12T18:13:18Z

@s-urbaniak (you may have seen gps previously referred to as vsolver in the glide queue - i renamed it in mid July)

so it may change glide's behavior in regards to unimported packages

we haven't actually discussed this point yet, but it is listed in the number of major discussion points we need to tackle - Masterminds/glide#565. Interestingly, this topic also happens to be coming up right now in the pkg mgmt committee discussions.

so, sdboyer/gps#42 will add support for designating arbitrary additional packages whose dependencies must be satisfied. idk what choice glide folks will make wrt this feature, once added - maybe you'll be able to explicitly designate a main, e.g. github.com/tinylib/msgp. msgp is a good example, because the binary package at the root of the repo imports github.com/ttacon/chalk, but the package you primarily import in your own code, github.com/tinylib/msgp/msgp does not. Consequently, chalk would get missed in naive import analysis, which would make the msgp binary uncompilable.

The potential problem is, as discussed in Masterminds/glide#418, using that system to designate additional things you want pulled in has a potentially undesirable effect: those dependencies then have to be reconciled with everything else in your actual project depgraph. This probably makes sense for something like msgp, where you want the binary to use the exact same logic to generate msgp structures as your application uses to read them, but makes very little sense for e.g. a linter, where all you really care about is reliably having the same version of that linter associated with the project.

The potential for conflicts here isn't an abstract, either; it's easy to imagine annoying things happening around something like, say, logrus - some linter wants to use logrus, and so does your app. Now, you find yourself having to reconcile logrus version disagreements between your linter and your app, even though it's not harmful (in this case) to have two separate versions of logrus, because they'd never need exist in the same compiled binary.

So, in sum/to recap, I think it's important to separate these two cases:

I simply want to have the same version of some tooling reliably available as part of my project
I also need to make sure the deps of that package are interwoven with my own project's deps.

sdboyer/gps#42 will facilitate both of these together, but not the first one alone. No plan has yet been made as to how glide will interact with that feature, once converted to gps.

s-urbaniak · 2016-09-13T19:16:08Z

@sdboyer Many thanks for the very detailed summary! 👍 You are right, inside rkt we have a "naive mono-repo" view at all dependencies right now, be it libraries, or binaries, that perspective didn't change in the transition from godep to glide, so this problem is not new at all. We were lucky not to be bitten by it. We used to have a separate metadata file, see [1], so I have no problem in re-introducing this, given there is some support for this in glide.

As long as there is no idiomatic solution in glide itself I am fine on the suggestion from @sgotti for now to provide an option to fallback to glide.lock instead of glide list.

[1] https://github.com/coreos/rkt/blob/8b5deb1f0cb61f15aff8e5dc2c77451c20134e30/vendoredApps

FugiTech · 2016-09-14T01:35:14Z

I've added --use-lock-file to revert to the prior behavior. Most of this PR is now just a refactor of the logic in cleanup, and the addition of glideListImports.

sgotti

@Fugiman Really appreciated your refactor and the additonal efftor to fallback to using glide.lock.

I've got just one little question and a doubt on the use of string.HasPrefix.

After clarifying this I think you should squash your commits and I'll merge it.

sgotti · 2016-09-21T07:48:32Z

gvc.go

-					}
-				}
+			// Keep legal files in directories that are the parent of a needed package
+			keep = keep || !opts.noLegalFiles && IsLegalFile(path) && strings.HasPrefix(name, lastVendorPathDir)


I will do a loop on filepath.Dir instead of strings.HasPrefix to not wrongly keep unrelated files.
For example if the pkg name is github.com/repoAAAAA/pkg and the file path is github.com/repo/LICENSE then lastVendorPathDir will be github.com/repo (no trailing slash) and strings.HasPrefix(github.com/repoAAAAA/pkg, github.com/repo) will match keeping that file.

peanut gallery: heh, this bit me so often in gps, i made a utility function for it (now largely subsumed into trie logic)

sgotti · 2016-09-21T07:50:14Z

gvc.go

-				}
+			// If a directory is a needed package then keep it
+			keep = keep || info.IsDir() && name == lastVendorPath
+


just noticed than in the previous iteration you called break after setting keep to true. This avoided some unneeded looping on the other packages since the file was already marked to be kept. Did you removed this because it's not a big deal and to keep the code cleaner?

Yes, I figured the comparisons were all operating on things already in memory, so there wouldn't be a huge performance hit, and I felt it made the code easier to follow. It should be pretty easy to add the breaks back if needed.

FugiTech · 2016-09-21T22:14:47Z

I've added isParentDirectory to address the github.com/foo vs github.com/foobar case, and added tests for it. Hopefully it catches all the edge cases :)

I'm also personally a big fan of the "squash+merge" functionality of github, but if you prefer I squash & force-push I can do that.

sgotti · 2016-09-22T08:35:47Z

I've added isParentDirectory to address the github.com/foo vs github.com/foobar case, and added tests for it. Hopefully it catches all the edge cases :)

Great! LGTM!

I'm also personally a big fan of the "squash+merge" functionality of github, but if you prefer I squash & force-push I can do that.

I tried this just now but it looks strange that I'll be the one creating the squashed commit message instead of leaving this to you 😝 . What's your preferred commit message? Just the title or something more detailed?

The old glide.lock behavior is still available via --use-lock-file. This also refactors the cleanup function to hopefully be easier to follow. Also adds more tests around getLastVendorPath and isParentDirectory.

FugiTech · 2016-09-23T00:59:27Z

Squashed :)

sgotti · 2016-09-23T08:19:03Z

@Fugiman Thanks a lot! Merged.

Given that I absolutely don't care about windows, this fixes code and tests on it. PR #21 tried to set the PATH env var in an unix like way with the effect of disabling the go test execution. This patch: * Fixes PATH env to add $GOPATH/bin * Fixes isParentDirectory * Fixes TestGetLastVendorPath and TestIsParentDirectory * Handles missing error check on `glide list` execution

Given that I absolutely don't care about windows, this fixes code and tests on it. PR #21 tried to set the PATH env var in an unix like way with the effect of disabling the go test execution without an error code. This patch: * Fixes PATH env to add $GOPATH/bin * Fixes isParentDirectory * Fixes TestGetLastVendorPath and TestIsParentDirectory * Handles missing error check on `glide list` execution

sgotti reviewed Sep 10, 2016
View reviewed changes

sgotti reviewed Sep 21, 2016

View reviewed changes

Use glide list as the primary source of package dependencies

272f0d2

The old glide.lock behavior is still available via --use-lock-file. This also refactors the cleanup function to hopefully be easier to follow. Also adds more tests around getLastVendorPath and isParentDirectory.

sgotti merged commit c614322 into sgotti:master Sep 23, 2016

sgotti mentioned this pull request Sep 23, 2016

Fix code and tests on windows. #23

Merged

FugiTech deleted the use_glide_list branch September 23, 2016 21:11

sgotti mentioned this pull request Sep 24, 2016

Unneeded vendor packages in glide.lock are kept #14

Closed

sgotti modified the milestone: v0.1.0 Sep 25, 2016

s-urbaniak mentioned this pull request Oct 25, 2016

scripts: update glide vendor script rkt/rkt#3313

Merged

sgotti mentioned this pull request Dec 21, 2016

Don't remove main packages explicitly declared in glide.yaml #27

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use glide list #21

Use glide list #21

FugiTech commented Sep 8, 2016

sgotti commented Sep 9, 2016

FugiTech commented Sep 9, 2016

sgotti commented Sep 9, 2016

FugiTech commented Sep 10, 2016

sgotti Sep 10, 2016

sgotti commented Sep 10, 2016 •

edited

Loading

FugiTech commented Sep 10, 2016

s-urbaniak commented Sep 12, 2016

FugiTech commented Sep 12, 2016

sdboyer commented Sep 12, 2016 •

edited

Loading

s-urbaniak commented Sep 13, 2016

FugiTech commented Sep 14, 2016

sgotti left a comment

sgotti Sep 21, 2016 •

edited

Loading

sdboyer Sep 21, 2016 •

edited

Loading

sgotti Sep 21, 2016 •

edited

Loading

FugiTech Sep 21, 2016

FugiTech commented Sep 21, 2016 •

edited

Loading

sgotti commented Sep 22, 2016 •

edited

Loading

FugiTech commented Sep 23, 2016

sgotti commented Sep 23, 2016

Use glide list #21

Use glide list #21

Conversation

FugiTech commented Sep 8, 2016

sgotti commented Sep 9, 2016

FugiTech commented Sep 9, 2016

sgotti commented Sep 9, 2016

FugiTech commented Sep 10, 2016

sgotti Sep 10, 2016

Choose a reason for hiding this comment

sgotti commented Sep 10, 2016 • edited Loading

FugiTech commented Sep 10, 2016

s-urbaniak commented Sep 12, 2016

FugiTech commented Sep 12, 2016

sdboyer commented Sep 12, 2016 • edited Loading

s-urbaniak commented Sep 13, 2016

FugiTech commented Sep 14, 2016

sgotti left a comment

Choose a reason for hiding this comment

sgotti Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

sdboyer Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

sgotti Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

FugiTech Sep 21, 2016

Choose a reason for hiding this comment

FugiTech commented Sep 21, 2016 • edited Loading

sgotti commented Sep 22, 2016 • edited Loading

FugiTech commented Sep 23, 2016

sgotti commented Sep 23, 2016

sgotti commented Sep 10, 2016 •

edited

Loading

sdboyer commented Sep 12, 2016 •

edited

Loading

sgotti Sep 21, 2016 •

edited

Loading

sdboyer Sep 21, 2016 •

edited

Loading

sgotti Sep 21, 2016 •

edited

Loading

FugiTech commented Sep 21, 2016 •

edited

Loading

sgotti commented Sep 22, 2016 •

edited

Loading