Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make gazelle play well with vendor directories from dep #725

Closed
swsnider opened this issue Aug 16, 2017 · 13 comments
Closed

Make gazelle play well with vendor directories from dep #725

swsnider opened this issue Aug 16, 2017 · 13 comments

Comments

@swsnider
Copy link
Contributor

I use gazelle in vendored mode by running gazelle -external vendored.

When I use dep to generate a vendor directory for a project that uses github.com/sirupsen/logrus, I get a bunch of build failures because vendor/github.com/stretchr/testify/assert doesn't exist (since dep doesn't consider tests of your dependencies to also be dependencies, and didn't download it to vendor/). It would be amazing if gazelle also supported such a mode so that I can stop tearing my hair out about this issue.

@jayconrod
Copy link
Contributor

It sounds like the solution to this would be for Gazelle not to generate go_test rules for dependencies (external or vendored). It seems pretty reasonable to have a flag for this, just to avoid additional test-only dependencies.

@ianthehat
Copy link
Contributor

Also, worth noting that longer term we would rather have a tool that added/updated repository rules in the bazel worspace from dep information, so bazel users never need to see a vendor directory.

@swsnider
Copy link
Contributor Author

@ianthehat that sounds like an anti-feature from my perspective -- I have a hard requirement that I'm not allowed to download dependencies from the internet during build, instead I have to check in vendor/ (and keep a fork of this project to override the location of the go toolchain).

@jayconrod That sounds kinda awesome -- I never really want to run the vendor/... tests anyway. This is more than just dropping go_test though -- I pull in golang.org/x/oauth to do some oauth client stuff, and dep doesn't download one of the transitive dependencies of golang.org/x/oauth/google because I never import that package -- I have to hand-remove the BUILD.bazel file generated by gazelle for that directory.

@jayconrod
Copy link
Contributor

@swsnider I haven't played with dep yet, but it surprises me that they don't download all transitive dependencies of a repository. I thought they were going to be working at the granularity of repos, not packages, but I guess that's just for versioning.

Gazelle currently doesn't keep track of what's used or not, so I don't think this is going to be practical for us, at least in the short term. Removing the generated BUILD.bazel files for unused packages is the right thing to do if you don't want the dependencies. You can add a # gazelle:exclude <dir> directives in the parent BUILD.bazel file to prevent Gazelle from recreating them.

@swsnider
Copy link
Contributor Author

@jayconrod thought of doing that, but dep always overwrites the entire vendor directory in order to ensure consistency, so I wasn't able to do what I want because I need to be able to add them to the great-great-grandparent (or whatever) BUILD file, which isn't supported :(

@jayconrod
Copy link
Contributor

That seems like something we should support, too. I've filed #730 to track that specifically.

@swsnider
Copy link
Contributor Author

I've 'fixed' this on my side by creating a go binary that reads the dep lockfile and deletes all directories not mentioned in the lockfile, and by having a shell script to replace directly invoking dep that invokes dep, invokes this cleaner binary, invokes gazelle on the vendor directory, then invokes buildozer several times to clean up the gazelle-generated files. Specifically, it does the following buildozer invocations:

buildozer delete '//vendor/...:%go_test'
buildozer fix '//vendor/...:*'
buildozer fix '//vendor/...:__pkg__'

Maybe this is the best approach we can have right now without a shitload of work on rules_go to support this potentially moving target?

@kevinburkeotto
Copy link

I'm running into a similar problem, I think.

dep supports a prune method where it only checks in packages that you need. I believe this is based on the actual dependencies of the final compiled product. For example, let's say

golang.org/x/build/a

imports golang.org/x/build/b, but my library does not use the part of the library that requires that import. Gazelle still wants to declare that dependency, which then means I need all of the things that golang.org/x/build/b imports, which is a huge pain. I keep needing to add things to the dep requires array.

I believe this is a separate issue than the one above, which is about test dependencies declared by an import and not recognized by Gazelle.

@kevinburkeotto
Copy link

I'm trying to add Bazel rules to a dependency now and running into this problem I load golang.org/x/crypto/ssh/terminal. Gazelle generates

vendor/golang.org/x/crypto/ssh/BUILD.bazel

which expects

    deps = [
        "//vendor/golang.org/x/crypto/curve25519:go_default_library",
        "//vendor/golang.org/x/crypto/ed25519:go_default_library",
    ],

to be present. I don't need those - I only need golang.org/x/crypto/ssh/terminal.

@sdboyer
Copy link

sdboyer commented Oct 12, 2017

hi folks - thanks @kevinburke for pointing dep folks to this.

i don't quite fully understand all the goals here, but a few notes from what i do think i grok:

  • the binary @swsnider is describing in this comment should be subsumed by the work of dep prune (so, reiterating @kevinburkeotto's comment)
  • dep prune is going to be subsumed into dep ensure, with some more granular controls added - Absorb prune into ensure golang/dep#944 - this should further alleviate some of the needs of that script
  • while it does seem ideal that gazelle could also ignore imports unique to the dependencies' tests, if you're in a pinch, you can always use dep's required property

@jayconrod
Copy link
Contributor

@kevinburkeotto Thanks for posting that example. I think it makes sense for Gazelle to continue generating rules for whatever sources are present in the directory. The only other solution I can think of would be to only generate go_library rules for packages that were referenced outside of vendor. That's feasible but complicated, and I think solving this on the dep side with golang/dep#1113 is a much better solution, since those sources don't need to be there in the first place.

@kevinburke
Copy link
Contributor

(apologies for the 2 different accounts thing - I posted from my work Github account by accident)

@jayconrod
Copy link
Contributor

jayconrod commented Dec 18, 2017

Closing old Gazelle issues. I've migrated the remaining work to bazel-contrib/bazel-gazelle#37. This covers disabling go_test rule generation in dependencies (both vendor and external). Let me know if I left anything out.

Note that the ability to exclude subdirectories in vendor is now implemented via # gazelle:exclude path/to/subdir comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants