bazel-deps icon indicating copy to clipboard operation
bazel-deps copied to clipboard

Make indirect dependencies have restricted visibility

Open kamalmarhubi opened this issue 6 years ago • 4 comments

Right now, all the generated targets have visibility = ["//visibility:public"]. This allows a kind of strict dependency violation where a build target could depend on a maven artifact not mentioned in the yaml file.

The targets for the indrect dependencies should have visibility = ["//${thirdPartyDirectory}:__subpackages__"] so that they can only be referenced by targets under the third party directory.

Originally https://github.com/johnynek/bazel-deps/issues/98#issuecomment-349928980

kamalmarhubi avatar Dec 10 '17 02:12 kamalmarhubi

Hacked together an initial POC in #107.

We will probably have to do:

  • make a flag to disable/enable the behaviour (it can be quite disruptive for existing users)
  • also add the visibility to dirname("path/to/workspace.bzl") since it does not have to be located in the same directory as generated BUILD files
  • ~~maybe add an option to expose transitive dependencies to a custom target (writing custom rules that depend on transitive depdendencies, not sure about this one)~~

EDIT: Thinking about this again 3. is probably not useful

arjantop avatar Jan 21 '18 14:01 arjantop

adding the co-ordinates for 400 dependencies by hand is not fun. is this really really necessary ? Bazel has a query tool and there is a lock file under source control. I don't see what this layer adds.

hsyed avatar Jun 27 '18 19:06 hsyed

Using deps like spring boot and dropwizard is completely infeasible with strict deps -- I spent 90 minutes openening up deps but there is no end in sight. Like 70% of the deps in the transitive closure are needed to create a web application and they don't all neatly fit in a modules list. The standard jetty / jersey stack alone is insane (glassfish, javax.*, jetty, hk2, jersey, jackson, swagger).

Realistically I'd have to buildozer and open the visibility after the regenerating the deps.

hsyed avatar Jun 27 '18 20:06 hsyed

I'm open to a PR to add an option, but I'm in the middle of a big push on rules_scala and won't have time this week, and I'm on vacation next week.

This didn't burn us internally at stripe and actually solved several problems. If you are manually depending on these things, it seems like only O(1) work to declare them (you don't need to mention the version, just declaring the language is enough).

I am still not convinced we don't want to be strict, but I am willing to add a non-default option to be lax.

johnynek avatar Jun 27 '18 21:06 johnynek