jvm_import_external #5068
jvm_import_external #5068
Conversation
…rt_external macro inin order to allow for other jvm languages to utilize the 'import_external' functionality
|
Google bot |
|
Ok thank u
|
|
@jart I would appreciate your review as well :) |
|
cc @simontoens about the maven_import we discussed |
|
cc bazel-discuss thread also #4876 |
There was a problem hiding this comment.
AFAICT this change would cause us to migrate our Java repository definitions to use template metaprogramming in our Java WORKSPACE definitions, e.g.:
java_import_external = jvm_import_external("java_import(name = '%{name}', ...)", extra_attrs = {})What is the use case for this?
cc: @ronshapiro from Java Core Library Teams
See also Bazel Maven Config Generator which includes a java_import_external Best Practices Guide that explains why this file was designed the way it was.
It's verbose, but I determined it needed to be that way, because so many one-off changes needed to be made to the generated configs, particularly for things like annotation processing libraries. These cannot be inferred from Maven POM metadata.
|
@jart I’m not sure I understand the problem.
The user API did not change, right?
What is it that you’d like to change?
The motivation for templating the actual rule is to have reuse of the core
logic. It was suggested by @dslomov.
…On Thu, 26 Apr 2018 at 21:35 Justine Tunney ***@***.***> wrote:
***@***.**** requested changes on this pull request.
AFAICT this change would cause us to migrate our Java repository
definitions to use template metaprogramming in our Java WORKSPACE
definitions, e.g.:
java_import_external = jvm_import_external("java_import(name = '%{name}', ...)", extra_attrs = {})
What is the use case for this?
cc: @ronshapiro <https://github.com/ronshapiro> from Java Core Library
Teams
See also Bazel Maven Config Generator
<#3946> which includes a java_import_external
Best Practices Guide
<https://github.com/jart/bazel/blob/ef0328910fd61a7e77f4c56443d87365eeac7174/tools/build_defs/repo/maven_config_generator/README.md>
that explains why this file was designed the way it was.
It's verbose, but I determined it needed to be that way, because so many
one-off changes needed to be made to the generated configs, particularly
for things like annotation processing libraries. These cannot be inferred
from Maven POM metadata.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#5068 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF22ibrFPPhzDrPIAn0POaXwvVVjjks5tshNbgaJpZM4Te1RG>
.
|
|
@dslomov I think we need your help here. |
|
This looks like it's just a drop in replacement for existing |
|
it is
…On Mon, Apr 30, 2018 at 5:51 PM Jingwen ***@***.***> wrote:
This looks like it's just a drop in replacement for existing
java_import_external calls?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#5068 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF7kZRd6sw9bJRys4PP1kInSe51JFks5ttyTpgaJpZM4Te1RG>
.
|
|
@dslomov PTAL |
I can't easily verify that is the case because this change rewrites the whole file. java.bzl is a high risk file to modify, because projects using it can't pin the version of |
|
@jart I've taken out the IIUC, you're suggesting to write a macro for java with a different name and keep the original Also the API hasn't changed at all (in terms of attributes of |
|
I generally like this idea. @jart, what is the nature of your objection? Looks like |
…ble_rule_name case
|
ping @jart |
|
I took a closer look at the code. Is there a design doc for this? The main change that seems to be happening here is it'll accept Maven coordinate triplet strings. I recommend using |
@jart I think the main change here is abstracting the *_import rule generation away from the user interface (macro). With |
|
@jingwen Exactly.
@jart It *allows* one to use it with maven coordinates while still allows
using URLs like you currently do.
Just for reference we have in rules_scala scala_import_external (uses URLs)
and scala_maven_import_external (uses coordinates).
“Yes it makes your WORKSPACE file ugly, but it's worth it.”-
1. Why are you the one making the tradeoff? Why not allow users to choose?
2. I think you’re also a bit wrong since you can pass in server_urls to the
maven macro which will append the maven coordinates URL to each server. If
the servers respect the maven coordinates url layout you’ll have many URLs.
3. Having maven as the abstraction layer enables propagating the maven
coordinates which many people want for easier bazel to maven export
…On Sat, 12 May 2018 at 6:44 Jingwen ***@***.***> wrote:
The main change that seems to be happening here is it'll accept Maven
coordinate triplet strings.
@jart <https://github.com/jart> I think the main change here is
abstracting the *_import rule generation away from the user interface
(macro). With jvm_import_external, anyone can write a custom *_import
rule and create an externalized version of it with
jvm_import_external.rule_name. For one, I can create
android_import_external easily by creating a macro that wraps
jvm_import_external with aar_import.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#5068 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF8IrZyyStwBnRcYjVmJUKDvVqJAEks5txlqEgaJpZM4Te1RG>
.
|
@jin I understand; I agree; I would ask if we've considered generalizing to all build rules, rather than just JVM? I would support Skylark automatically defining a
@ittaiz I'm very interested in hearing more of what you have to say. |
|
@jart I think I said what I have to say so I’ll repeat but if you can guide
me to what’s unclear I’d appreciate it.
I think the tradeoff is between resiliency and maintenance/readability.
Some organizations will be happy with their internal binary repository
(artifactory / nexus) and would rather have less duplication of the URLs
and have it be more readable.
…On Sat, 12 May 2018 at 10:45 Justine Tunney ***@***.***> wrote:
I think the main change here is abstracting the *_import rule generation
away from the user interface (macro).
@jin <https://github.com/jin> I understan;, I agree; I would ask if we've
considered generalizing to *all* build rules, rather than just JVM? I
would support Skylark automatically defining a foo_external for all foo.
I consider repository rules like java_import_external and
filegroup_external
<https://github.com/bazelbuild/rules_closure/blob/master/closure/filegroup_external.bzl>
(see also NodeJS
<https://github.com/tensorflow/tensorboard/blob/master/third_party/js.bzl#L25>,
DefinitelyTyped
<https://github.com/tensorflow/tensorboard/blob/master/third_party/typings.bzl#L20>)
to be simple one-off solutions. I want one to become infinity. I'm less
excited about the integers between.
1. Why are you the one making the tradeoff? Why not allow users to
choose?
@ittaiz <https://github.com/ittaiz> I'm very interested in hearing more
of what you have to say.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5068 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF5Fxq1z3hImvHtLKe67gFtMJwIk3ks5txpMygaJpZM4Te1RG>
.
|
|
It's only worthwhile to go to the trouble of defining redundant URLs explicitly when you're a public project that wants its build definitions to be as universally reliable / performant as possible. For example, any Skylark rule that uses |
|
@jart I’m sorry but I don’t understand the point you’re trying to make. Can
you please reiterate?
Btw, we want this change (and the usage of repository_ctx.download) mainly
for the external dependencies cache
…On Tue, 15 May 2018 at 4:36 Justine Tunney ***@***.***> wrote:
It's only worthwhile to go to the trouble of defining redundant URLs
explicitly when you're a public project that wants its build definitions to
be as universally reliable / performant as possible. For example, any
Skylark rule that uses repository_ctx.download (e.g. java_import_external)
will respect the http_proxy environment variable. That grants private
entities the flexibility to use their own private storage / retrieval
infrastructure when inheriting public definitions. This is also the case if
you write a macro wrapper that fills in URLs for convenience.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5068 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF_91w5mJkOTDpHLqvJGIzA3n64ABks5tyjEsgaJpZM4Te1RG>
.
|
|
By we you're referring to rules_scala, correct? Is rules_scala blocked by this change? If so, why? |
I am not sure "I want one to become infinity. I'm less excited about the integers between." is a good reason to block this PR. We should first explore the "integers between" before we generalize. |
|
In we I meant my company’s builds which need to cache the external
dependencies
…On Tue, 15 May 2018 at 9:58 Dmitry Lomov ***@***.***> wrote:
@jin <https://github.com/jin> I understand; I agree; I would ask if we've
considered generalizing to all build rules, rather than just JVM? I would
support Skylark automatically defining a foo_external for all foo. I
consider repository rules like java_import_external and filegroup_external
(see also NodeJS, DefinitelyTyped) to be simple one-off solutions. I want
one to become infinity. I'm less excited about the integers between.
I am not sure "I want one to become infinity. I'm less excited about the
integers between." is a good reason to block this PR. We should first
explore the "integers between" before we generalize.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5068 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF-JstMpQQUubzp9udFQF7G7uhbZbks5tynyFgaJpZM4Te1RG>
.
|
|
@ittaiz I sometimes use stash.sh to cache HTTP downloads, since it's easier than Squid, and cheaper than Artifactory. I was under the impression Bazel supported this, but just noticed 86a28b0 which unsets |
|
I need a systematic and easy solution for many hundreds of external
dependencies.
I didn’t deep dive into the above script but I’m not sure that is it.
Additionally,
What is the problem you’re trying to solve?
It almost sounds like you’re trying to mandate people to work with URLs.
Why?
The suggestion is equally resilient if you have the same layout of the URL
as maven artifacts have.
The suggestion just asks you for base URLs of the servers and computes the
rest.
…On Wed, 16 May 2018 at 0:50 Justine Tunney ***@***.***> wrote:
@ittaiz <https://github.com/ittaiz> I sometimes use stash.sh
<https://gist.github.com/jart/8c5288db4398b8bd7a1e20f8deec4a16> to cache
HTTP downloads, since it's easier than Squid, and cheaper than Artifactory.
I was under the impression Bazel supported this, but just noticed 86a28b0
<86a28b0>
which unsets http_proxy in blaze.cc to workaround a gRPC issue. Is this
something that'd be helpful to you and your employer? Because that support
could easily be added back via a flag.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5068 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF_zMBJXj5gXJI_iiYYHx0Is03cakks5ty02dgaJpZM4Te1RG>
.
|
|
Mandate? Please note I work TensorBoard, not Bazel. You can do anything you want. Having a caching HTTP proxy like stash.sh is useful because you'll hit a point where GitHub starts sending |
|
The current form of this PR doesn't handle android rules/aars. I imagine that we could, and probably should handle that, otherwise we raise the risk for another fork. To go back to @jart's desire for infinity rules, one thing that may make this more concrete is a rule like so: download_file(
name = "foo.jar",
urls = [
# list of url strings
],
sha256 = "abcdefgh",
licenses = [],
)That seems to be the minimum necessary to use (maybe that's the same as Once we take out the downloading, it seems that most of what is in this PR is a bit of feature creep. Inserting a load statement, extra build file content, and more. Perhaps having people write their own build file templates would actually be clearer and stop us from adding even more attributes to this in the future. |
Yes, I plan to upstream the |
|
@ronshapiro That's how we did things before 2017, when we'd define a folder in |
|
That seems like two separate issues. |
|
To @ronshapiro's point. On the other hand, parameterizing underlying build file definitions is a large part of this PR. I have trouble interpreting this statement:
This PR is about making BUILD files generated by |
|
What's the plan for Android support? Is this going to be further factored to handle that? Are there any other formats besides The parts that deal with |
|
What does android support mean other than supporting the aar packaging?
Why do you think neverlink is a feature creep? To me it feels closely
related since this concept in bazel is very related to the declaration site
…On Sun, 3 Jun 2018 at 23:25 Ron Shapiro ***@***.***> wrote:
What's the plan for Android support? Is this going to be further factored
to handle that? Are there any other formats besides .jar and .aar that
are worth supporting (ear? war? I don't know if anyone actually uses
those).
The parts that deal with neverlink seem like feature creep to me. That,
to me, seems like something that should be done separately from the
repository configuration. Visibility can help with that. It's a style
question, so people can reasonably disagree.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5068 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF4QtFcocI0okrAbtI_eHvNBuNPUfks5t5EZSgaJpZM4Te1RG>
.
|
|
@ronshapiro Here's the prototype PR: #5319 |
This PR copies "upstream" a change made to java_import_external in
rules_scala(see PR)This change was originally proposed by @dslomov here (search for 'jvm_import_external')
java_import_external was changed to
jvm_import_external'template like' rule +java_import_externalmacro in order to allow for other jvm languages (e.g. scala and kotlin) to utilize the 'import_external' functionality without copying the boiler plate again and again.This has already been used in
rules_scalawith the introduction ofscala_import_externalandscala_maven_import_externalIn addition to the
import rule name,jvm_import_externalcan also be called with custom attributes needed by the underlying import rules, as well as a custom load statement.java_import_externalis used as a macro in rules_scala to make sure it's still functioning properly after the change.jvm_maven_import_externalexposes maven artifact terminology.This will also allow to create a
maven_import_externalmacro that will delegate tojvm_maven_import_externalwith amaven_importrule which will haveMavenCoordinatesProvider as discussed here