bazeltools / bazel-deps

Generate bazel dependencies for maven artifacts
MIT License
250 stars 122 forks source link

FEAT: use maven_server and maven_jar to fetch artifacts. #239

Open dgarzon opened 5 years ago

dgarzon commented 5 years ago

This PR is an extension to: https://github.com/johnynek/bazel-deps/pull/215, and aims to close https://github.com/johnynek/bazel-deps/issues/95. It replaces our custom download code, with maven_server and maven_jar. This allows users to download artifacts from authenticated servers by just having $HOME/.m2/settings.xml properly set up. Feedback welcomed!

johnynek commented 5 years ago

Thanks for the PR. I’m on vacation today and tomorrow.

One thing I’m concerned about is this is almost an exact revert of a previous PR. So this implies there are at least two different constituencies.

One question: why isn’t this a small change to your own repo: I.e. configure your own repo’s callback to use maven_jar? Why do we need to change the default (especially considering I think bazel has deprecated native maven_jar).

dgarzon commented 5 years ago

@johnynek Thanks for the review! Feel free to answer after you come back from vacation. This is my motivation for the change:

  1. According to the documentation: https://docs.bazel.build/versions/master/be/workspace.html#using-maven_server, this is the recommended way to fetch dependencies from a maven repository as it aligns better with what a Java / Scala developer would expect (define a repository in the dependencies.yaml and server definitions in settings.xml with the necessary credentials if needed)
  2. If #215 implemented all the necessary functionality to allow for credentials defined in settings.xml, a user of bazel-deps should expect it to just work out of the box end-to-end. With the current implementation this is not true, or at least I am not sure how this can be done. Reason being each dependency needs to be mapped to a maven_server, that has been defined before.
  3. Although the source files say that java_import_external is replacing maven_jar, I cannot find anywhere in the documentation where it is mentioned. In this discussion: https://github.com/bazelbuild/bazel/issues/4825, Ittai gives pros and cons of each implementation. I think the maven_jar implementation aligns better with the goals of this repo.

Let me know what you think! Enjoy the rest of your vacation 💯

johnynek commented 5 years ago

so @hsyed and @rdnetto4 have taken stabs at this problem, but we still aren't where we think it is solved.

This PR reverts the previous work of @hsyed #181 where the jar_artifact rule was made the new default callback: https://github.com/johnynek/bazel-deps/pull/181/files#diff-2af0d4e7b4b60cc36e83ad2149e68f51R50

What I'm concerned with is that it seems people have bespoke needs, they often make a PR to change the defaults to match their needs, then they move on and the next person repeats.

My question: why can't we accomplish using maven_jar using the existing callback mechanism? In that approach, you write a starlark function in your repo, and pass that function to maven_dependencies in order to control how you load the maven dependencies? That was the purpose of the callback architecture.

Your change moves us back to how things used to be, by the way, but maven_jar didn't support sha256, so others wanted to change the default.

Note, at stripe, we override this function, so I don't tend to worry too much, except that I clearly just accepting the most recent version of this and flipflopping around isn't an approach that makes sense.

What I'd like to do is outline:

  1. why using a callback with the existing code can't work. I think it can. Maybe we really simply need better documentation and not a code change?
  2. in any case can we document some use cases with authentication: using settings.xml, but do others want other mechanisms? request form user at CLI, env var, read from user defined path? etc...

I'd love for others to chime in so we can make some progress.

Thanks for bearing with me.

deadmoose commented 5 years ago

There are also issues that bazel will blindly invoke a bazillion maven_jars in parallel and bring a machine to its knees: https://github.com/bazelbuild/bazel/issues/5452