search

bazelbuild / migration-tooling

Migration tools for Bazel
Apache License 2.0
45 stars 30 forks source link

Implements transitive maven jar repository rule. #46

Closed petroseskinder closed 7 years ago

petroseskinder commented 7 years ago

This pull request provides an initial implementation of the transitive_maven_jar rule. The repository rule self hosted and generated the generate_workspace.bzl found in this CL.

Design considerations and additional slated functionality is discussed in this design document.

Provides a working solution for the upgrade problem discussed in #20

@kchodorow

bazel-io commented 7 years ago

Can one of the admins verify this patch?

petroseskinder commented 7 years ago

@kchodorow I resolved the issue of constantly downloading the generate_workspace tool by adding the deploy jar in the transitive_maven_jar directory.

It works, but by no means is it optimal. I would rather not check a binary into the repository. However, this approach allows us to point users to an http_archive of just the transitive_maven_jar repository rule. That is, without leaking any information of how the tool resolves dependencies. The only annoyance is on our end with maintaining the jar. On that front, Alpha suggested creating a regular old rule to package that directory into a zip file.

kchodorow commented 7 years ago

I'd rather have the generate_workspace deploy jar be downloaded. Other than that, this looks good to go.

petroseskinder commented 7 years ago

I'd rather have the generate_workspace deploy jar be downloaded.

Why is that?

I don't feel strongly either way, but downloading the jar seems a little counter intuitive to me.

From a user's perspective, packaging them together is clearly preferable.

From our standpoint, we have little to gain by downloading the deploy jar. At least, how I have interpreted it. The two general approaches we could take for downloading would be to download from within the repository rule as I did before or to have the user use a http_archive.

The first is obviously bad.

The second (using http_archive) is unnecessarily clunky for users, as they have to manually write the http_archive. From our end, we would still need to host the executable jar somewhere. That is until Ulf or Damian decide it is appropriate to expose bazel under bazel-tools, we can't just use the http_archive of the migration-tooling repository (i.e. github.com/bazelbuild/migration-tooling/archive/master.zip, non-working example here).

If we are going manually host and update the jar anyways, why not package it together with the repository rule, as I have done, and make users lives easier? In addition, the repository rule is strongly dependent on the version of generate_workspace it calls. Why give users the flexibility to potentially break things?

kchodorow commented 7 years ago

I was thinking it would be better to have it be separate, so it would be easy to swap out a different version. However, I'm willing to merge it in as-is and see how it goes.

But: I was thinking about having //generate_workspace depend on transitive_maven_jar which runs generate_workspace and that seems like a bad idea. It'll be annoying to bootstrap that if any step breaks. Thus, please revert the WORKSPACE to using plain ol' generate_workspace.bzl.

petroseskinder commented 7 years ago

But: I was thinking about having //generate_workspace depend on transitive_maven_jar which runs generate_workspace and that seems like a bad idea. It'll be annoying to bootstrap that if any step breaks. Thus, please revert the WORKSPACE to using plain ol' generate_workspace.bzl.

Done.

kchodorow commented 7 years ago

Jenkins, test this please.