-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Expose full compile time jars for Java targets in Skylark #3528
Comments
Thanks! |
|
Can you give a bit more details? Why do you need control over what |
@dslomov thing is that we want to use ijars in rules_scala but some (rough swag I'd say 30%) have macros which mean ijar doesn't work for them. I'd like to be able to say that for a specific @iirina the need is to have the ability to declare for a specific java_import_external) target that ijar shouldn't be created or exposed. Then I'll use that to mark external dependencies which have macros. The feature you list above sounds interesting for rules_scala's own usage for source targets but that is currently solved internally |
Is
So you would want something like the |
I am not sure what you mean by "for a specific Foo artifact ijar should be turned off". |
@iirina Yes, Foo is a maven import target. Use ijars at the target level sounds exactly what I need. I'd rather they not be created to save the time but I don't really care. @dslomov the problem in your scenario is that the using target, which is where rules scala code runs, doesn't know if the originating dependency should be consumed as an ijar or regular jar. That is something the origin declares but in your suggestion I don't see any way for me to hint from java_import_external to rules_scala about it |
Full design of this is up to @iirina, but I expect there to be two fields on |
But the information is in the declaration site not in the call site. |
I am sorry, I do not understand. Can you elaborate? Maybe an example will help. |
The information is "does the target have macros" which means "don't use my ijar since you won't compile". That information is known to the origin not to the destination. Is it clear? If not I'll try to come up with an example |
Hmm. Java does not know what macros are, so that information does not belong in Actually how Scala rules know that the source has macros? |
The user knows. My suggestion is to expose a Boolean property saying something like "turn_ijars_off". The docs will say something like: "some jvm languages cannot use ijars since it breaks compilation". The correct solution would be to fix ijar (#632) but that can take months and I don't see this being a priority in the next year or so. |
A boolean property on what? On java_import?
As a specific example, let's say maven_jar generates java_imports. Where would maven_jar get information about whether a specific jar from maven has scala macros? Basically what you are asking for is a property 'i_have_scala_macros' on 'java_*' targets. This is not a good design. Next JVM language comes along and we add 'i_have_frotlin_flux_capacitors' :) I see several solutions, all not great:
|
(and I see that in #632 this has been discussed as well; and you are actually adding |
I built a basic scala_import which utilizes maven_jar but then I had many integration issues with intellij. Add to that my need for source downloading, licences, sha256 and we'll get to me duplicating all of @jart's work in This sounds like a lot of duplication and fragmentation over ijars which obviously have their limits and I'd guess that most JVM languages (other than java) will have caveats with it since they try to piggyback on the bytecode. If the composition story of rules and workspace rules would have been more advanced then maybe I could have built on top of these rules and just propagated my own boolean property but this just isn't doable in the current setting. wdyt about exposing |
I think you want to cut your cake and eat it too. Let's talk about
|
Sorry for the delay, am on vacation and with limited access. |
I wrote a little pseudocode to describe what you said, is this what you were thinking about? java_import(name = "real_import",...)
java_import_wrapper(name = "wrapper_import", deps = [":real_import"], ...) def _java_import_wrapper_impl(ctx):
deps_providers = [dep[java_common.provider] for dep in ctx.attr.deps]
deps_providers_without_ijars = [remove_ijar(dep_provider) for dep_provider in deps_providers]
return java_common.merge(deps_providers_without_ijars)
def remove_ijar(provider):
return java_common.create_provider(compile_time_jars=provider.compile_time_full_jars, ....) |
Not really since this strips out all ijars. I want only for specific deps.
And then for dependency |
Yes, only you cannot use def _scala_import_impl(ctx):
native_import = ctx.attr.native_import
if (ctx.attr.use_ijars):
provider = native_import[java_common.provider]
else:
provider = remove_ijar(native_import[java_common.provider])
deps_providers = [dep[java_common.provider] for dep in ctx.attr.deps] + [provider]
return java_common.merge(deps_providers)
scala_import = rule(
implementation = _scala_import_impl,
attrs = {
"use_ijars": attr.bool(default = True),
"native_import": attr.label(),
"deps": attr.label_list()
}
) And the BUILD file would look like this: java_import(
name = "whatever_import",
jars = ["a.jar"]
)
scala_import(
name = "whatever",
native_import = ":whatever_import",
use_ijars = 1
) |
Thanks!
I can make scala_import a macro which will generate both of them and have
the java_import with private visibility, right?
If so this sounds great
…On Tue, 22 Aug 2017 at 10:51 Irina Iancu ***@***.***> wrote:
Yes, only you cannot use native.java_import in a rule implementation (in
analysis phase). You can only use it in macros (loading phase).
Unfortunately the only solution I see here is to declare both a
java_importand a scala_import, where the latter depends somehow on the
former and retrieves whatever it needs from it:
def _scala_import_impl(ctx):
native_import = ctx.attr.native_import
if (ctx.attr.use_ijars):
provider = native_import[java_common.provider]
else:
provider = remove_ijar(native_import[java_common.provider])
deps_providers = [dep[java_common.provider] for dep in ctx.attr.deps] + [provider]
return java_common.merge(deps_providers)
scala_import = rule(
implementation = _scala_import_impl,
attrs = {
"use_ijars": attr.bool(default = True),
"native_import": attr.label(),
"deps": attr.label_list()
}
)
And the BUILD file would look like this:
java_import(
name = "whatever_import",
jars = ["a.jar"]
)
scala_import(
name = "whatever",
native_import = ":whatever_import",
use_ijars = 1
)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF9AE25RySPfzWrdjEoiNvhKtloWZks5saoiJgaJpZM4Ox7uS>
.
|
Yup, that sounds like a plan! |
Great.
Any idea when you'll be able to expose it?
…On Tue, 22 Aug 2017 at 15:30 Irina Iancu ***@***.***> wrote:
Yup, that sounds like a plan!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIFx0wnSSDHIP8iQB1CAwnodiUTqhBks5sasnfgaJpZM4Ox7uS>
.
|
To expose the full jars? |
If I understood you correctly |
Sounds good. Thanks!
…On Wed, 6 Sep 2017 at 19:36 Irina Iancu ***@***.***> wrote:
I'll start tomorrow laying the grounds for this and I'll let you know the
estimate. Now I appreciate a few days, but maybe tomorrow I'll find more
intricacies.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF3PqqWkDUbzqzz30xKnhpo_4nA6dks5sfsnvgaJpZM4Ox7uS>
.
|
Next week should be at HEAD. |
awesome, thanks! Any chance you know when is 0.6 targeted for? |
No, probably when we get Bazel CI green again :) You can watch the release issue and ping the assignee for more details. |
Will do. Thanks!
…On Fri, 8 Sep 2017 at 9:39 Irina Iancu ***@***.***> wrote:
No, probably when we get Bazel CI green again :) You can watch the
release issue <#3286> and ping
the assignee for more details.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF5KkSbTf6VMiKbxYbgFlWnOOYNHjks5sgOEugaJpZM4Ox7uS>
.
|
I'm having a bit of a problem deciding how to create the API and its meaning. Exposing full compile time jars is easy, straight-forward: But for this to be hermetic, the user also should have the possibility of specifying full compile time jars when creating a provider. Here I have a problem. Let's say we pass the compile time jars like this: @ittaiz Does the following documentation make sense? Does it meet your needs? Do you thing it's easy to understand? Do you have any better proposal?
One drawback I see is that it puts more burden on the user to also add the full time jars as What is your user opinion about this? Thanks! |
I agree it's a tricky one.
WDYT? BTW It's important IMO to notice that the need to expose the full compile time jars is not only the aspect I mentioned above but also for IDEs. Intellij currently has an agreement with each of the jvm rules about which attribute it should expose this from but using JavaInfo should be standard |
Yes, I've also thought of a builder, but I don't think it aligns with the Skylark way. At the same time I want this object to be sealed in as fast as possible, and just be used for passing information. And keep everything simple.
This make sense, but I think it's very Java-like. Besides, the main problem I see here is that you need another data structure to encapsulate what a compile time jar is (with or without full jar/ijar), which brings up more problems and complexity.
For this extent we already exposed ijar on This is what I came up with after some thinking: java_common.create_provider(
compile_jars = [...],
runtime_jars = [...],
source_jars = [...],
deps = [...], # this should be a list/set of JavaInfo, to encapsulate the transitive information
declare_full_jars = True/False, # by default False
full_compile_jars = [...], # only set if declare_full_jars is True
) This presents 2 benefits:
dep_provider = java_common.create_provider(runtime_jars = [C.jar])
provider = java_common.create_provider(
compile_jars = [A.jar],
runtime_jars = [B.jar],
deps = [dep_provider]
) This is more readable IMO, and I think it would reduce the burden of understanding transitive jars.
Thanks for bringing up the importance of full jars for IDEs. I'm not familiar with how they work with jvm rules. What kind of agreement do they have? In terms of who else needs the compile time full jars, I am aware of Kotlin also needing them as depoendencies. |
sounds like a good idea to avoid repeating each field for transitive jars as well.
It's still unclear to me (and so maybe for other users of the api), what happens if the user sets this field ( Just to be sure I understand, e.g. when depending on Am I correct? If usage of |
First I'd like to say I like the move to
The problem with the API as I see it is that it's not clear to the user why should they declare full jars? |
After a meeting with @ittaiz and @lberki yesterday, we agreed on the following API: java_common.create_provider(
compile_jars = [...],
use_ijar = True/False, # defaults to False
runtime_jars = [...],
source_jars = [...],
deps = [...] # A list of JavaInfo to encapsulate the transitive information
) which is great. We now distinguish 2 cases:
The only inconvenience is that to use ijar, the method also needs access to the rule context (to register the ijar actions) and to the java toolchain (to retrieve the ijar tool). We end up with this API: java_common.create_provider(
compile_jars = [...],
use_ijar = True/False, # defaults to False
ctx = ctx, # only required when use_ijar is True
java_toolchain = ctx.attr._java_toolchain, # only required when use_ijar is True
runtime_jars = [...],
source_jars = [...],
deps = [...] # A list of JavaInfo to encapsulate the transitive information
) I am happy with this approach. It's consistent with |
Can you replace |
@dslomov Yes, I managed to replace |
Sounds good!
Two small questions/suggestions:
1. Maybe useIjar should default to True? That is the normal use case.
2. Can we have full_compile_jars always non empty? When I use the target I
don't know if the original target used ijar or not
…On Wed, 13 Sep 2017 at 15:34 Irina Iancu ***@***.***> wrote:
@dslomov <https://github.com/dslomov> Yes, I managed to replace ctx with
ctx.actions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF8M6V9ORtVYShZIxSuQzYfCTh9fRks5sh8vWgaJpZM4Ox7uS>
.
|
Couple of use cases:
|
Thanks Chaoren!
Irina,
2 more questions:
1. If I pass a dep with neverlink this should be fine right? If I
understand it correctly it should contribute to the compile deps
transitively (strict deps story) and won't contribute to the runtime deps
2. Is the reference to ctx.actions mean that I'll need to pass ctx.actions
in? I think the symmetry with java_common.compile is valuable and I
wouldn't want to lose it.
…On Wed, 13 Sep 2017 at 19:13 Chaoren Lin ***@***.***> wrote:
IDEs- @chaoren <https://github.com/chaoren> does intellij (as an example)
have use-cases where it needs the full jars and not only the ijars? I think
so but not sure what are they.
Couple of use cases:
1. If the sources are not available, attaching the decompiled full jar
would be more helpful than the ijar.
2. For Android, we sometimes want the resource ID values from the
R.class.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF7KS2XU4FeGBoTkCnR6cA9W6TZw7ks5sh_8_gaJpZM4Ox7uS>
.
|
(sorry I forgot to press comment when I first wrote this -_-) Thanks @chaoren for the use cases. @ittaiz my answers are below:
I chose the default value as
OK. Let's assume that compile_jars are full jars when
For native rules yes. Unfortunately java_common.compile doesn't consider neverlink right now :(
Dmitry is actually working on making it possible to pass |
Ok, aren't you concerned this will push people to not use ijars? this is exactly what @lberki was afraid of
Awesome, thanks.
ok, I think we'll need that fixes. Shall I open a new issue for this?
sounds great |
Yeah, that's also true. I'll consider this.
Yes, thanks! Thanks everyone for participating, it was very helpful! |
Hi Irina,
Do you think this will make it into 0.6.0? It needs to due this being a
breaking change but rc2 already rolled out
…On Thu, 14 Sep 2017 at 14:45 Irina Iancu ***@***.***> wrote:
Ok, aren't you concerned this will push people to not use ijars? this is
exactly what @lberki <https://github.com/lberki> was afraid of
Yeah, that's also true. I'll consider this.
ok, I think we'll need that fixes. Shall I open a new issue for this?
Yes, thanks!
Thanks everyone for participating, it was very helpful!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF-ywuA1DW-pd_GCrugrVRsiQkJyNks5siRHLgaJpZM4Ox7uS>
.
|
I'll ask tomorrow if we can cherry pick this. I'm waiting for the change to be submitted internally, should be on GH tomorrow. |
Exciting! Thanks!
…On Mon, 18 Sep 2017 at 19:45 Irina Iancu ***@***.***> wrote:
I'll ask tomorrow if we can cherry pick this. I'm waiting for the change
to be submitted internally, should be on GH tomorrow.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3528 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF7QqdWn7X-ZmRi0oEEjJklYtcMknks5sjp4ngaJpZM4Ox7uS>
.
|
This PR copies "upstream" a change made to java_import_external in`rules_scala` (see [PR](bazelbuild/rules_scala#473)) This change was originally proposed by @dslomov [here](#3528) (search for 'jvm_import_external') java_import_external was changed to `jvm_import_external` 'template like' rule + `java_import_external` macro 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_scala` with the introduction of `scala_import_external` and `scala_maven_import_external` In addition to the `import rule name`, `jvm_import_external` can also be called with custom attributes needed by the underlying import rules, as well as a custom load statement. `java_import_external` is used as a macro in rules_scala to make sure it's still functioning properly after the change. `jvm_maven_import_external` exposes maven artifact terminology. This will also allow to create a `maven_import_external` macro that will delegate to `jvm_maven_import_external` with a `maven_import` rule which will have `MavenCoordinates` Provider as discussed [here](#4654) Closes #5068. PiperOrigin-RevId: 198398621
Right now there is no way to retrieve the full jars (only ijars/hjars) used at compile time for a Java target in Skylark. The best way would be to expose it on
java_common.provider
(JavaInfo
).The text was updated successfully, but these errors were encountered: