Resolve devImports

[alde]

Can be omitted by using the new flag --without-dev

glide update [--without-dev]
glide install [--without-dev]

[mattfarina]

Note, this implements #320.

[mattfarina]

@alde This is a good start. Thanks. It still needs to have the resolver get the imports from the test files and put them into the devImports.

[alde]

Cheers, I'll try to fix that this weekend, or on monday.

[technosophos]

I was originally going to suggest that we do not include dev dependencies by default (e.g. that we have --include-dev instead). But as I thought about the use case, it feels to me like the cases where we'd not want dev imports are for CI/CD and stuff like that. So I agree with this choice.

When we encounter Glide.yaml files in dependencies, do we obey this flag in regards to those? I'm kinda thinking no. But I could be swayed the other way.

[alde]

My idea with --without-dev was that if you know that you won't be running the tests for example you'd know that you don't want/need them installed

I am however having some issues with the resolver, as in I'm not sure I fully understand what it's meant to do. I assumed at first it went through the .go files and found imports, and as such it would need to go through the _test.go files for dev imports, but that doesn't seem to be the case

[technosophos]

Maybe we should step back a moment and describe what devImports is for. I think @alde may be right: we may not need to go through the _test.go files to get what we want.

Here's my first pass:

• devImports is for packages that are required to develop on the local project (not anything in vendor/)

Examples of devImports:

• An assert library that is used only in testing (for the local project), e.g. Ginkgo
• An external Go tool that is used for something like go generate

My assumption is that by default, when I import with devImports, I can do things like run go test on the project. But I am not assuming that I'd be able to cd into vendor and run go test ./... and have that work.

Does that sound similar to what you were thinking @mattfarina and @alde ?

[alde]

Yes, that was my thought, and the reason I started trying to patch it.

[mattfarina]

@technosophos The external tools become a little more difficult to use for devImports. The generator is a great example. How do you detect it's a tool that should be installed and install it rather than just put it in vendor? I was considering scripts for the case. Then devImports can be things like imports only used in tests.

[technosophos]

So based on the comments of @alde @mattfarina and me, it sounds like we're down to just this use case:

• devImports is for libraries necessary only for running tests on the present project (not on things in vendor)

Does that sound right?

[lpetre]

I wanted to +1 @technosophos's comment for this use-case:

An external Go tool that is used for something like go generate

We do this currently for fresh and I would love for our dependency management solution to cover this use case. Otherwise, do I need to have a separate way (makefile, ant, etc) of telling my developers to go get a half-dozen packages at specific versions/urls/etc?

[mattfarina]

@alde Is this something I should pickup and finish completing or is this something you wanted to finish rounding out?

[alde]

Yeah, sorry, I've got it a bit hectic atm.

If you want to finish it, please feel free, I don't know when I'll have the time to focus on it again :(

[ches]

We do this currently for fresh and I would love for our dependency management solution to cover this use case. Otherwise, do I need to have a separate way (makefile, ant, etc) of telling my developers to go get a half-dozen packages at specific versions/urls/etc?

Ditto.

By comparison, I've been happy using gb on application-only projects, but am hoping Glide can take its place for projects with a library component so they can be go get-able. With gb, executables from vendored dependencies get built into vendor/bin, which is similar to how some analogs like Bundler in Ruby behave. My teams standardized on direnv with a project .envrc like the following, so that the project's executables including vendored tools like ginkgo are readily available:

export GOPATH="$PWD/vendor:$PWD"
PATH_add "$PWD/vendor/bin"
PATH_add "$PWD/bin"

Of course gb eschews GOPATH entirely, so that allows for flexible answers to what to do about the executables (GOPATH in the .envrc is just to make tools like gocode, goimports, etc. play along with the project FWIW).

Would it be viable for Glide to install the devImports executables in vendor/bin (and possible without gross hacks around what go/build wants to do naturally—I'm not familiar with the internals)? It seems reasonably convenient for users to invoke project-local tools from there without needing anything extra like direnv, and there's some similar precedent, but I'm not sure if it would run afoul of any current or foreseeable assumptions/conventions of Glide, go tooling, or others…

I start to wish that Go vendor experiment had followed vendor/{bin,pkg,src} like gb does…

[ches]

I was originally going to suggest that we do not include dev dependencies by default (e.g. that we have --include-dev instead). But as I thought about the use case, it feels to me like the cases where we'd not want dev imports are for CI/CD and stuff like that.

...

• devImports is for libraries necessary only for running tests on the present project (not on things in vendor)

Does that sound right?

I think that a critical conflation of dev versus test has pervaded the thread—I was first confused by @technosophos's quoted mention of CI. I thought, "I certainly do not want to use --without-dev on CI if Ginkgo is in devImport", but @lpetre's example of fresh is exactly a case where you would. (Though, it's not really an "import", you don't import fresh in project code—maybe devPackages?).

Ruby's Bundler supports arbitrary groups with dev and test being common convention; all are installed by default, and typically --without=dev is used on CI and --without=dev,test in production. This is akin to possibilities @xeger proposed in #168.

But presumably Go build tooling does a fine job already of keeping test libraries from bloating production artifacts when they're only imported in _test.go files. So it seems to me like devImports is all that is really needed (arbitrary groups/tags seem overkill for now), but test dependencies do not go there but in import instead (and something must be worked out about executables like the ginkgo runner, as well as dev executables like fresh, as discussed earlier).

In that case I think it becomes the clear choice that devImports is only considered for the local project and not recursively within vendor, as @technosophos's first version above suggested. Thus making the option an opt-out --without-dev seems more convenient for the common use case to me.

The downside is that, if Glide looks at glide.yaml/lock files recursively in dependencies, it would be ideal if it didn't just install everything in import, it should probably filter out deps that it discovers are only imported in _test.go files. @davecgh made the fair point on #103 that go get doesn't waste time and space fetching test dependencies by default, unless you use -t. Maybe it would be easier to additionally have testImport, then? Doesn't help avoid waste for deps that don't use Glide, though, if test imports aren't already being omitted there (I'm not sure). Personally I don't care if go test ./... errors in the default case, I'm practically never going to use that without glide novendor so I support the opt-in -t equivalent.

[mattfarina]

Here's a thought, should it change from devImports to testImports to clear up some confusion?

[sdboyer]

@mattfarina I think that renaming would probably be helpful. I may rejigger the corresponding naming for vsolver, too.

@ches:

The downside is that, if Glide looks at glide.yaml/lock files recursively in dependencies, it would be ideal if it didn't just install everything in import, it should probably filter out deps that it discovers are only imported in _test.go files.

Yeah, this is part of a more general problem - what @technosophos and I have been calling the "bi-modal analysis" problem: do you operate at the project or package level in building the depgraph? When you do the former, you may get excess packages; when you do the latter, you have no version information. The former is wasteful, but ultimately correct; the latter is just wrong (which is why we're all here).

It's not intractable, I don't think; it just involves making some tradeoffs. However, I've deferred work on it in favor of trying to get vsolver integration into glide going ASAP in #384. Once that's in, though, tackling that general problem - which would hopefully take care of this along with - is basically next on my TODO list.

[ches]

Here's a thought, should it change from devImports to testImports to clear up some confusion?

@mattfarina If we've resolved that the semantic is meant to be analogous to go get -t, then yes I think that makes sense and focuses the discussion (and it sounds like it's pretty much what @alde's PR was conceived as, modulo naming).

Consideration of a feature for dev tooling like the go generate support and fresh examples could be cleanly moved to a subsequent ticket, it seems.

[arvenil]

Hi guys,

What's the status of that?

The lack of vendoring imports from tests is pretty big issue for me, as basically you can't run tests that relay on some packages that aren't required in the code itself. To bypass the issue right now I do cp somepackage_test.go somepackage_x.go; glide up; rm somepackage_x.go.

Btw this was working couple months ago, as then glide was fetching ALL dependencies... I love that it doesn't do that anymore but yeah since then it doesn't parse dependencies from test files:/

[arvenil]

Something like this (without this patch) fixes the issue for me

index dc88f62..62b4731 100644
--- a/dependency/resolver.go
+++ b/dependency/resolver.go
@@ -266,7 +266,7 @@ func (r *Resolver) ResolveLocal(deep bool) ([]string, error) {
                return err
            }
        } else {
-           imps = p.Imports
+           imps = append(p.Imports, p.TestImports...)
        }

        // We are only looking for dependencies in vendor. No root, cgo, etc.

I've started to think that maybe a good idea would be to merge just that one line to fix the issue with _test.go deps not being vendored... and then, in next versions, you could guys improve that with that e.g. by separating app and test deps. I don't see a drawback of fixing the issue first, and then working on improvements. Fixing this like that right now doesn't collide with moving later deps from imports to devImports in yaml

[mmindenhall]

Please note that it's also possible for test files in package foo to declare their package as foo_test (from go help test):

Test files that declare a package with the suffix "_test" will be compiled as a
separate package, and then linked and run with the main test binary.

If the list of test imports is being collected from the Package struct, the entire set of imports would include:

    imps := p.Imports                      // imports from GoFiles, CgoFiles
    imps = append(imps, p.TestImports...)  // imports from TestGoFiles
    imps = append(imps, p.XTestImports...) // imports from XTestGoFiles

[tommyminds]

Is there any news on this PR or similar functionality (vendoring test dependencies) going into glide anytime soon?

[sdboyer]

@TommyM yes, this problem is already generally solved in the new engine (unconditionally, though - allowing a flag that would skip installing non-test imports is another can of worms).

so, just gotta get that in :) #384

[kevin-cantwell]

I know this is closed, but perhaps this will be helpful for those trying to vendor their test deps with glide. It's a crazy stupid long one liner in bash that passes all recursive test deps into glide get:

$ glide create
$ ( go list -f '{{join .TestImports "\n"}}' $(glide novendor) | xargs go list -f '{{join .Deps "\n"}}'  | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}' ; go list -f '{{join .XTestImports "\n"}}' $(glide novendor) | xargs go list -f '{{join .Deps "\n"}}'  | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}' ) | grep -v $(go list) | xargs glide get

Unfortunately, this only works if your vendor directory is empty. Otherwise you'll end up with a bunch of subpackages in your glide.yaml with the "vendor/" prefix.

[mattfarina]

Early next week I'll have a PR to more formalize this. The hard part is already done.