Implementation the jar path -> label mapping in JavaCompileAction

[Lukács T. Berki]

Hey there,

This thread is to continue the discussion on https://github.com/bazelbuild/bazel/issues/4584 and to make it a little more discoverable.

Problem statement: Tom changed Bazel such that the label for each .jar in a Java compile action is in the .jar itself and not in a provider so that Bazel uses less RAM. Unfortunately, this breaks scala_import and exports: the former, because it doesn't use ijar and therefore is not able to annotate the .jar with its label and the second, because when using exports= attribute, the same jar can have multiple different labels.

Question is, how to fix this? The trivial solution would be to move back to providers, but that would mean regressing on memory and performance, because that's why Tom made this change in the first place.

--

Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[Tom Lundell]

+@cushon

I posted a reply before I saw this, please make sure you've read it: https://github.com/bazelbuild/bazel/issues/4584#issuecomment-374604683

I propose that even when ijar isn't used, we rewrite the compile time jar to add the desired owner to the jar manifest.

Pros:

  * Restores add_dep functionality to Scala

  * Fixes the scala_import case by reattributing ownership of the jar to the scala_import rule.

Cons:

  * Adds an additional action and artifact even when ijar is off.

  * Does not address general owner re-attribution, as in exports.

As I stated on that GitHub thread, I don't think the exports case is something we should worry about now, nor do I even think it's something that _can_ be fixed because there simply isn't a unique owner.

If that sounds palatable I'll go ahead and elaborate on two possible implementations of this. Both will work just fine, it's a matter of API taste.

[Liam Miller-Cushon]

On Tue, Mar 20, 2018 at 7:03 AM Tom Lundell <to...@google.com> wrote:

I propose that even when ijar isn't used, we rewrite the compile time jar to add the desired owner to the jar manifest.

SGTM.

Note that the tool that does that could be ijar--we could consider having a flag to disable the interface jar extraction part of ijar and use that for scala instead of disabling the ijar action wholesale. But that isn't really important, having a different tool that fixed up the manifest would be fine too.

 

I also want to note that disabling ijar for scala/kotlin is working around bugs that (especially for kotlin) we have leads on fixing:

* https://github.com/bazelbuild/bazel/issues/4549

* https://github.com/bazelbuild/bazel/issues/632

Making ijar work for those languages seems like a better long-term solution than spend time mitigating secondary problems caused by not using ijar.

As I stated on that GitHub thread, I don't think the exports case is something we should worry about now, nor do I even think it's something that _can_ be fixed because there simply isn't a unique owner.

As I mentioned in the bug, I don't think SJD is the right place to handle exports when adding deps:

https://github.com/bazelbuild/bazel/issues/4584#issuecomment-374063777

SJD doesn't have enough information to make good decisions about exports (and e.g. visibility) of the missing deps, giving it access to that information is problematic, and we'd want any solution we build for that to work for other dependency fixing workflows. I think that logic belongs in a separate dependency management tool.

[Tom Lundell]

On Tue, Mar 20, 2018 at 1:18 PM, Liam Miller-Cushon <cus...@google.com> wrote:

On Tue, Mar 20, 2018 at 7:03 AM Tom Lundell <to...@google.com> wrote:

I propose that even when ijar isn't used, we rewrite the compile time jar to add the desired owner to the jar manifest.

SGTM.

Note that the tool that does that could be ijar--we could consider having a flag to disable the interface jar extraction part of ijar and use that for scala instead of disabling the ijar action wholesale. But that isn't really important, having a different tool that fixed up the manifest would be fine too.

I essentially have two proposals.

First proposal, add flag to java_common:

java_common.create_provider(

  use_ijar = False,

  write_jar_owner = True,

  compile_time_jars = current_target_compile_jars,

  ...

)

Then under the hood, java_common can do whatever it needs to, knowing both the value of both boolean flags. The name of the flag and its default value is subject to bikeshedding.

Second proposal, expose tool to users:

owned_jar = []

for jar in compile_jars:

  owned_jar = ctx.actions.declare_file(jar.basename + ".owned.jar", sibling=jar)

  args = ctx.actions.args()

  args.add("--input")

  args.add(jar)

  args.add("--output")

  args.add(owned_jar)

  args.add("--target_label")

  args.add(ctx.label)

  ctx.actions.run(

      inputs = [jar],

      outputs = [owned_jar],

      executable = [<tool>],

 )

 owned_jars.append(owned_jar)

return java_common.create_provider(

  use_ijar = False,

  compile_time_jars = owned_jars,

  ...

)

They are both pretty much equivalent in terms of what then happens during the build. The first one is a little more convenient and magic, the second is less convenient but obvious.

[Liam Miller-Cushon]

For (1), does write_jar_owner even need to be a flag? What's the use-case for write_jar_owner=False?
I'm wondering if we can make write_jar_owner=True the default, and not expose configuration for it.

[Tom Lundell]

It does create new jars, so if there is somehow multiple ingress points you'll now have duplicates on your classpath. Though same is true when you use ijar, so maybe it can just be done unconditionally.

See here for instance: https://github.com/bazelbuild/rules_scala/blob/master/scala/scala_import.bzl#L31

This looks wrong, note how exports are treated as if they are coming from "this" rule. There can easily be multiple exports of the same target making it into the build graph. I would have thought that would lead to duplicates on the classpath. I don't think this is the semantics of "exports" on internal java rules.

[Liam Miller-Cushon]

On Tue, Mar 20, 2018 at 10:56 AM Tom Lundell <to...@google.com> wrote:

It does create new jars, so if there is somehow multiple ingress points you'll now have duplicates on your classpath. Though same is true when you use ijar, so maybe it can just be done unconditionally.

Right, it would be good to know if that's a real problem, since it will also affect ijar.

If it isn't a concern, it would be more consistent to just always create the action.

[Lukács T. Berki]

I don't think it affects ijar unless you have two java_import rules that reference the same source jar. Unfortunately, we can't say the same for exports (the difference is that exporting would give another name to a jar produced by a different rule, whereas ijar processing always happens in the same rule where the corresponding .jar is put on the classpath). If we do the "replace label in the jar" thing for exports, we can also change the order of jars on the classpath, can't we? It's also costly in terms of output size -- we add an ijar of unbounded size as a generated artifact only to convey a tiny label.

For these two reasons, I'm not really thrilled by the idea of munging .jars by exports=. I'd prefer either giving up on add_dep recommending the label of the exporting rule or administering this in a TransitiveInfoProvider like it was before. Maybe a hybrid solution is workable, i.e. exports=, but only exports= adds a .jar->label map entry, and JavaBuilder looks at the map, and if an entry is not present, uses the label in the .jar? That way, the in-memory data structure would be smaller because presumably, on any given compile action, there are much fewer .jars acquired through exports= than .jars altogether. 

--
You received this message because you are subscribed to the Google Groups "bazel-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-discus...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/CAL4Qsgvuhr_ewshT43NacArePuTP2Lds1S3REQvhYe%3D1FqLZrw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Liam Miller-Cushon]

On Wed, Mar 21, 2018 at 2:45 AM Lukács T. Berki <lbe...@google.com> wrote:

Maybe a hybrid solution is workable, i.e. exports=, but only exports= adds a .jar->label map entry, and JavaBuilder looks at the map, and if an entry is not present, uses the label in the .jar? That way, the in-memory data structure would be smaller because presumably, on any given compile action, there are much fewer .jars acquired through exports= than .jars altogether. 

The performance impact of that still seems concerning even if the constants are smaller. It is true that exports are responsible for a subset of compile-time jars, but the number they're responsible for isn't negligible and will increase over time.

I also disagree that we want to unconditionally walk exports edges backwards for SJD. There are cases where we do, e.g. private implementation targets that are exported by a public user-visible target. There are also lots of cases where we don't, when targets export other libraries for 'convenience', but other targets that only need the underlying library should should still depend directly on it.

WDYT about my suggestion earlier in the thread to leave handling of exports/visibility to a separate dependency management tool? I think +carmi has some experience in that area.

[ca...@google.com]

> WDYT about my suggestion earlier in the thread to leave handling of exports/visibility to a separate dependency management tool?

This is what we have at Google because add_dep was giving the wrong results.
There are several strategies to finding the right rule to add -
1. Find all rules in the same package that export and/or alias the new dependency
2. Look for rules in predetermined locations that export the new dependency (used for third-party jars)
3. Use a global index of export edges
After the list is expanded, it's filtered by visibility / tags / etc, and finally sorted (e.g. by distance from new dependency).

The first one can be implemented using `bazel query` in a Python script and doesn't require fancy tools.

[Lukács T. Berki]

Sounds like a plan. Sure beats rewriting .jar files when they are passed through an exports= attribute :) 

[Tom Lundell]

Can we move ahead with this? Will the proposed solution satisfy people's requirements and concerns?

Unfortunately, on the implementation side, modifying java_common.create_provider in any backwards compatible way seems really hard. People aren't required to pass ctx.actions, which I need to register actions.

# Would crash because actions aren't passed, or silently not create owned jars (confusing)

java_common.create_provider(

  use_ijar = False,

  compile_time_jars = current_target_compile_jars,

  ...

)

# If we add a write_jar_owner parameter that defaults to False, then this would work

java_common.create_provider(

  ctx.actions,

  use_ijar = False,

  write_jar_owner = True,

  compile_time_jars = current_target_compile_jars,

  ...

)

# But if we default it to False, then this is weird, because now we'd be instructing ijar to _not_ write a jar owner

java_common.create_provider(

  ctx.actions,

  use_ijar = True,

  compile_time_jars = current_target_compile_jars,

  ...

)

# And if we default it to true, then again, this would crash

java_common.create_provider(

  use_ijar = False,

  compile_time_jars = current_target_compile_jars,

  ...

)

It's a pretty weird API. A better API would be to unconditionally require the passing of ctx as the first argument to the provider method, then I wouldn't have this problem.

[ittai zeidman]

I needed to step away from the loop a bit.
Will sync tomorrow and reply.

Thanks!

[Liam Miller-Cushon]

We went through something similar when use_ijar was added. IIRC it was added to the API defaulting to false, and a future release made the breaking changing of flipping the default (which required passing ctx.actions).

Getting to a place where passing ctx.actions is mandatory SGTM.

And do we need write_jar_owner to be in the API? (Setting aside the question of how to migrate to enabling it by default.) Is there a use-case for disabling it?

[Lukács T. Berki]

On Thu, 22 Mar 2018 at 16:01, Tom Lundell <to...@google.com> wrote:

Can we move ahead with this? Will the proposed solution satisfy people's requirements and concerns?

I'm fine with the plan. I'd like Irina to okay the plan, too, though. She'll also hopefully have a plan for the java_common.create_provider issue.

[Irina Iancu]

java_common.create_provider is deprecated and will be replaced by the JavaInfo() constructor. actions is used for more actions now (surprise surprise) besides creating the ijar, e.g. creating the output source jar action, so the argument is actually mandatory. 

And do we need write_jar_owner to be in the API? (Setting aside the question of how to migrate to enabling it by default.) Is there a use-case for disabling it?

I am for doing the job silently, magically behind the API. I don't want to add yet one more flag to the constructor. One of the reasons is that it won't be present in java_common.compile and I want to keep the compile method and the constructor API consistent. Second, it puts more pressure on the users and adds complexity to the API which is already too packed with java_library inherited flags.

[Tom Lundell]

On Fri, Mar 23, 2018 at 8:09 AM Irina Iancu <elena...@google.com> wrote:

java_common.create_provider is deprecated and will be replaced by the JavaInfo() constructor. actions is used for more actions now (surprise surprise) besides creating the ijar, e.g. creating the output source jar action, so the argument is actually mandatory. 

OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.

If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False).

Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.

[ittai zeidman]

I think the open issue here is still exports.
If we decide this isn’t a different use-case labels wise then indeed having some tool encode it is ok.
Thing is I’m not convinced- what is the use-case (in the jvm world) for using exports unless you want people to relate to the exporting rule and not the exported one? What “convenience” does exports add over deps apart from SJD?

[Tom Lundell]

The point I was making earlier is that even if we wanted to solve exports using the SJD machinery, I don't think we can do so in a sane way.

Consider:

java_library(

  name = "a",

  srcs = [... some sources that illegally use indirect classes from "e" ...],

  deps = ["b"],

)

java_library(

  name = "b",

  deps = ["c", "d"],

  ...

)

java_library(

  name = "c",

  exports = ["e"],

)

java_library(

  name = "d",

  exports = ["e"],

)

java_library(

  name = "e",

  ...

)

This is perfectly legal, but there is simply no way the SJD machinery can recommend which out of "c", "d", or even "e" that should be printed to the add_deps command. The only unique choice that makes any sense is to always let the user know the exact target that produced the classes, in this case "e".

On Sat, Mar 24, 2018 at 8:29 AM ittai zeidman <itt...@gmail.com> wrote:

I think the open issue here is still exports.
If we decide this isn’t a different use-case labels wise then indeed having some tool encode it is ok.
Thing is I’m not convinced- what is the use-case (in the jvm world) for using exports unless you want people to relate to the exporting rule and not the exported one? What “convenience” does exports add over deps apart from SJD?

I personally don't think exports should be used much, if ever. It can be useful to refactor BUILD files in smaller steps, but I have found it obfuscates which targets are actually used by some code, and similarly which code actually uses a given target (eg. "Find usages" on a target in IntelliJ doesn't go through exports).

[ittai zeidman]

Thanks Tom, I understand the complexity but I was asking a different question. What is the use-case of exports other than "re-labeling"? Just convenience sounds like an anti-pattern that is exactly solved with strict-deps.

I've created the following example which demonstrates the need and problem as I see it.

In it we have a library foo which exports an internal part of it so that it can still have freedom of internal arrangement and also to ease mental load of users by exposing a single entry point for the library.

This fails today as the suggestion is to the internal, private visibility, target and not to the exporter.

Can we think of an "export-internal" semantics? Maybe this will be in a new attribute for rules that can allow themselves deprecating problematic semantics.

Lukacs,
Is there an “official” or semi-official Bazel team stand on exports?

[Carmi Grushko]

> Is there an “official” or semi-official Bazel team stand on exports?

[I admit I didn't thoroughly read everything in the thread, so I might be answering the wrong question]

AFAICT the consensus among people who deal with dependencies at Google is that

`exports` is generally more harm than good.

There's a small number of use-cases which are arguably useful, but in my opinion these should be considered the exception, not the norm.

Specifically, the internal+public use-case is not a good one in my opinion. 

It existed for C++, but was "deprecated" a couple of years ago - it's now recommended against.

My short summary is that walking dependency chains when looking for code is a waste of time, both for humans and automatic tools.

Caveat - the problems we ran into at Google might not be relevant for smaller polyrepos.

To remain on topic, let's fork this thread for further discussion.

[Tom Lundell]

Yes, if exports are no longer part of the SJD discussion, let's keep them on a different thread so we can move forward with a fix here.

[Irina Iancu]

OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.

If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False). 

SGTM. 

Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.

No, having actions instead of ctx was intentional. There is an on-going (maybe stalling?) effort from the Skylark team to split ctx in 2 parts, one of which is actions. The end goal will be to pass actions instead of ctx to restrict access to some parts of ctx, since it contains too many things and its scope is too large. 

[Tom Lundell]

On Mon, Mar 26, 2018 at 4:58 AM Irina Iancu <elena...@google.com> wrote:

OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.

If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False). 

SGTM. 

Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.

No, having actions instead of ctx was intentional. There is an on-going (maybe stalling?) effort from the Skylark team to split ctx in 2 parts, one of which is actions. The end goal will be to pass actions instead of ctx to restrict access to some parts of ctx, since it contains too many things and its scope is too large. 

I don't understand this argument. If the goal is just to split ctx into multiple sub-categories of stuff, that's cool, but that doesn't prevent APIs from taking ctx as the first argument, unconditionally always. Then they can get actions out of ctx no problems.

Are you going to completely remove actions from ctx? So rule functions take more arguments? Then sure, it might make sense. But I don't think that's what you are planning to do.

I think we'll find that without this, future evolution of the APIs will become more painful.

[Lukács T. Berki]

[Tom Lundell]

On Mon, Mar 26, 2018 at 5:34 PM Tom Lundell <to...@google.com> wrote:

On Mon, Mar 26, 2018 at 4:58 AM Irina Iancu <elena...@google.com> wrote:

OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.

If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False). 

SGTM. 

Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.

No, having actions instead of ctx was intentional. There is an on-going (maybe stalling?) effort from the Skylark team to split ctx in 2 parts, one of which is actions. The end goal will be to pass actions instead of ctx to restrict access to some parts of ctx, since it contains too many things and its scope is too large. 

I don't understand this argument. If the goal is just to split ctx into multiple sub-categories of stuff, that's cool, but that doesn't prevent APIs from taking ctx as the first argument, unconditionally always. Then they can get actions out of ctx no problems.

Are you going to completely remove actions from ctx? So rule functions take more arguments? Then sure, it might make sense. But I don't think that's what you are planning to do.

I think we'll find that without this, future evolution of the APIs will become more painful.

I didn't have to go very far to find an example of this :(

To do this change I need the rule's target label so I can tell ijar about it. This requires either passing in the label, or getting it from ctx. But I don't have ctx. Either way it's a migration.

I resubmit that these kinds of methods should all just take ctx as the first arg, and have it be mandatory. I think our methods will survive their evolution much better.

It's fine for ctx to just be a container of other things, if the original concern was the size of the ctx API.

[Tom Lundell]

On Sat, Mar 31, 2018 at 12:54 PM Tom Lundell <to...@google.com> wrote:

On Mon, Mar 26, 2018 at 5:34 PM Tom Lundell <to...@google.com> wrote:

On Mon, Mar 26, 2018 at 4:58 AM Irina Iancu <elena...@google.com> wrote:

OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.

If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False). 

SGTM. 

Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.

No, having actions instead of ctx was intentional. There is an on-going (maybe stalling?) effort from the Skylark team to split ctx in 2 parts, one of which is actions. The end goal will be to pass actions instead of ctx to restrict access to some parts of ctx, since it contains too many things and its scope is too large. 

I don't understand this argument. If the goal is just to split ctx into multiple sub-categories of stuff, that's cool, but that doesn't prevent APIs from taking ctx as the first argument, unconditionally always. Then they can get actions out of ctx no problems.

Are you going to completely remove actions from ctx? So rule functions take more arguments? Then sure, it might make sense. But I don't think that's what you are planning to do.

I think we'll find that without this, future evolution of the APIs will become more painful.

I didn't have to go very far to find an example of this :(

To do this change I need the rule's target label so I can tell ijar about it. This requires either passing in the label, or getting it from ctx. But I don't have ctx. Either way it's a migration.

I resubmit that these kinds of methods should all just take ctx as the first arg, and have it be mandatory. I think our methods will survive their evolution much better.

It's fine for ctx to just be a container of other things, if the original concern was the size of the ctx API.

Just FYI this change will be stalled until there's a resolution here. I can't proceed until we figure out what to do.

My opinion is to require ctx as the first arg to JavaInfo, but that's a migration so I'm not going to go do it unless people agree.

[Dmitry Lomov]

On Tuesday, April 3, 2018 at 4:53:35 PM UTC+2, Tom Lundell wrote:

On Sat, Mar 31, 2018 at 12:54 PM Tom Lundell <to...@google.com> wrote:

On Mon, Mar 26, 2018 at 5:34 PM Tom Lundell <to...@google.com> wrote:

On Mon, Mar 26, 2018 at 4:58 AM Irina Iancu <elena...@google.com> wrote:

OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.

If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False). 

SGTM. 

Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.

No, having actions instead of ctx was intentional. There is an on-going (maybe stalling?) effort from the Skylark team to split ctx in 2 parts, one of which is actions. The end goal will be to pass actions instead of ctx to restrict access to some parts of ctx, since it contains too many things and its scope is too large. 

I don't understand this argument. If the goal is just to split ctx into multiple sub-categories of stuff, that's cool, but that doesn't prevent APIs from taking ctx as the first argument, unconditionally always. Then they can get actions out of ctx no problems.

Are you going to completely remove actions from ctx? So rule functions take more arguments? Then sure, it might make sense. But I don't think that's what you are planning to do.

I think we'll find that without this, future evolution of the APIs will become more painful.

I didn't have to go very far to find an example of this :(

To do this change I need the rule's target label so I can tell ijar about it. This requires either passing in the label, or getting it from ctx. But I don't have ctx. Either way it's a migration.

I resubmit that these kinds of methods should all just take ctx as the first arg, and have it be mandatory. I think our methods will survive their evolution much better.

It's fine for ctx to just be a container of other things, if the original concern was the size of the ctx API.

Just FYI this change will be stalled until there's a resolution here. I can't proceed until we figure out what to do.

My opinion is to require ctx as the first arg to JavaInfo, but that's a migration so I'm not going to go do it unless people agree.

We are not passing `ctx` as an argument to JavaInfo, because that couples JavaInfo semantics too much to the rule that creates that javaInfo. It is inflexible and most of the time produces bad results

In the particular case you are talking about here, by embedding whatever comes from `ctx.label` into ijars, you are in fact saying "the primary output of this target is this jar; to if you happen to use classes from this jar, add a direct dependency on this specific target".

This is most likely the wrong thing. What if the rule only build the jar as an intermediate step, and it should actually never be exposed or dependent upon? What if the actual target you need to take to directly depend on a jar is different from the target denoted by `ctx.label`?

To make this more concrete, consider as an example Java proto aspects. JavaInfos for proto libraries are created inside an aspect, and the ctx.label there will refer to the proto_library rule. If you embed a label for proto_library rule into a jar, add_deps will try to add a direct dependency on a proto_library instead of suggesting an appropriate  java_proto_library.

In general, no Skylark APIs should ever take `ctx`. Take appropriate parts of ctx that you actually depend on, not the whole thing.

Dmitry

 

 

 

 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | <fon

[to...@google.com]

On Wednesday, April 4, 2018 at 5:23:13 AM UTC-4, Dmitry Lomov wrote:
> On Tuesday, April 3, 2018 at 4:53:35 PM UTC+2, Tom Lundell wrote:
>
>
>
>
> On Sat, Mar 31, 2018 at 12:54 PM Tom Lundell <to...@google.com> wrote:
>
>
>
>
>
> On Mon, Mar 26, 2018 at 5:34 PM Tom Lundell <to...@google.com> wrote:
>
>
>
>
>
> On Mon, Mar 26, 2018 at 4:58 AM Irina Iancu <elena...@google.com> wrote:
>
>
>

> OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False). 

>
>
> SGTM. 
>
> Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.
>
>
> No, having actions instead of ctx was intentional. There is an on-going (maybe stalling?) effort from the Skylark team to split ctx in 2 parts, one of which is actions. The end goal will be to pass actions instead of ctx to restrict access to some parts of ctx, since it contains too many things and its scope is too large. 
>
>
> I don't understand this argument. If the goal is just to split ctx into multiple sub-categories of stuff, that's cool, but that doesn't prevent APIs from taking ctx as the first argument, unconditionally always. Then they can get actions out of ctx no problems.
>
>
> Are you going to completely remove actions from ctx? So rule functions take more arguments? Then sure, it might make sense. But I don't think that's what you are planning to do.
>
>
> I think we'll find that without this, future evolution of the APIs will become more painful.
>
>
> I didn't have to go very far to find an example of this :(
>
>
> To do this change I need the rule's target label so I can tell ijar about it. This requires either passing in the label, or getting it from ctx. But I don't have ctx. Either way it's a migration.
>
>
> I resubmit that these kinds of methods should all just take ctx as the first arg, and have it be mandatory. I think our methods will survive their evolution much better.
>
>
> It's fine for ctx to just be a container of other things, if the original concern was the size of the ctx API.
>
>
> Just FYI this change will be stalled until there's a resolution here. I can't proceed until we figure out what to do.
>
>
> My opinion is to require ctx as the first arg to JavaInfo, but that's a migration so I'm not going to go do it unless people agree.
>

I completely missed this; For some reason the thread response didn't make its way to my email inbox.

>
> We are not passing `ctx` as an argument to JavaInfo, because that couples JavaInfo semantics too much to the rule that creates that javaInfo. It is inflexible and most of the time produces bad results
>
>
> In the particular case you are talking about here, by embedding whatever comes from `ctx.label` into ijars, you are in fact saying "the primary output of this target is this jar; to if you happen to use classes from this jar, add a direct dependency on this specific target".
>
>
> This is most likely the wrong thing. What if the rule only build the jar as an intermediate step, and it should actually never be exposed or dependent upon? What if the actual target you need to take to directly depend on a jar is different from the target denoted by `ctx.label`?

Then we'll have to take the label as an argument. This precludes it from being a seamless or behind-the-scenes migration, users will have to know about this aspect of the machinery, and explicitly pass the target label.

I'm OK with that, but previous opinion on this thread was that users shouldn't care, so that's what I was operating under.

>
>
> To make this more concrete, consider as an example Java proto aspects. JavaInfos for proto libraries are created inside an aspect, and the ctx.label there will refer to the proto_library rule. If you embed a label for proto_library rule into a jar, add_deps will try to add a direct dependency on a proto_library instead of suggesting an appropriate  java_proto_library.

That could be supported by passing another argument, "injecting_rule_kind".

As an aside, this case is pretty poorly supported anyway, because its support is hard coded. add_deps takes a label and the injecting rule kind and tries to guess the name of the corresponding java_proto_library rule. Eg. "Oh I see it's the target 'foo_proto' but created by a 'java_proto_library', hopefully there exists a rule called 'foo_java_proto' then". It will not extend graciously to user-defined aspects, and it will not work for users that do not adhere to our internal naming conventions.

I don't think it really changes your general argument.

>
>
> In general, no Skylark APIs should ever take `ctx`. Take appropriate parts of ctx that you actually depend on, not the whole thing.

I don't know that I agree with this as a blanket statement. Accepting ctx.actions seems to indicate that you are operating in the context of a rule implementation anyway, I don't quite see how 'actions' is dramatically more narrow for this reason, and my point about migration stands. But you people have thought about this more and I'm sure you have good reasons, I'm happy to move ahead with the explicit label argument in JavaInfo.

> Google Germany GmbH | <a href="https://maps.google.com/?q=Erika-Mann-Str.+33++%C2%A0%7C+80636+M%C3%BCnchen+%7C+Germany&entry=gmail&source=g" target="_blank" rel="nofollow" onmousedown="this.href='https://maps.google.com/?q\x3dErika-Mann-Str.+33++%C2%A0%7C+80636+M%C3%BCnchen+%7C+Germany\x26entry\x3dgmail\x26source\x3dg';return true;

[Liam Miller-Cushon]

On Monday, April 9, 2018 at 10:20:53 AM UTC-7, to...@google.com wrote:

Then we'll have to take the label as an argument. This precludes it from being a seamless or behind-the-scenes migration, users will have to know about this aspect of the machinery, and explicitly pass the target label.

It also allows rule implementations to pass a label other than the one that corresponds to the current action, which isn't ideal.

 

That could be supported by passing another argument, "injecting_rule_kind".

As an aside, this case is pretty poorly supported anyway, because its support is hard coded. add_deps takes a label and the injecting rule kind and tries to guess the name of the corresponding java_proto_library rule. Eg. "Oh I see it's the target 'foo_proto' but created by a 'java_proto_library', hopefully there exists a rule called 'foo_java_proto' then". It will not extend graciously to user-defined aspects, and it will not work for users that do not adhere to our internal naming conventions.

The current approach isn't supported at all in Bazel. The suggested fixes in Bazel use buildozer, and completely ignore the injecting rule kind. I filed https://github.com/bazelbuild/bazel/issues/4990 for this.

[Lukács T. Berki]

On Wed, 4 Apr 2018 at 11:23, 'Dmitry Lomov' via bazel-discuss <bazel-...@googlegroups.com> wrote:

On Tuesday, April 3, 2018 at 4:53:35 PM UTC+2, Tom Lundell wrote:

On Sat, Mar 31, 2018 at 12:54 PM Tom Lundell <to...@google.com> wrote:

On Mon, Mar 26, 2018 at 5:34 PM Tom Lundell <to...@google.com> wrote:

On Mon, Mar 26, 2018 at 4:58 AM Irina Iancu <elena...@google.com> wrote:

OK in that case I propose to do just do it always. I'll probably just make ijar do the job unless somebody thinks it's cleaner and worth it to add another standalone tool.

If actions isn't passed to create_provider then it'll silently not do anything for backwards compatibility. This does come with some risk of breakage, in case people make assumptions on the exact jars that get passed upwards (when use_ijar=False). 

SGTM. 

Tangentially, would it make sense to pass ctx always instead of actions in the API? Having ctx as the first argument is pretty common in various APIs. It feels a bit odd to have to pass piecemeal sub-components of it, and if we need more components in the future we end up having to add more arguments as part of migration, checking for null, etc.

No, having actions instead of ctx was intentional. There is an on-going (maybe stalling?) effort from the Skylark team to split ctx in 2 parts, one of which is actions. The end goal will be to pass actions instead of ctx to restrict access to some parts of ctx, since it contains too many things and its scope is too large. 

I don't understand this argument. If the goal is just to split ctx into multiple sub-categories of stuff, that's cool, but that doesn't prevent APIs from taking ctx as the first argument, unconditionally always. Then they can get actions out of ctx no problems.

Are you going to completely remove actions from ctx? So rule functions take more arguments? Then sure, it might make sense. But I don't think that's what you are planning to do.

I think we'll find that without this, future evolution of the APIs will become more painful.

I didn't have to go very far to find an example of this :(

To do this change I need the rule's target label so I can tell ijar about it. This requires either passing in the label, or getting it from ctx. But I don't have ctx. Either way it's a migration.

I resubmit that these kinds of methods should all just take ctx as the first arg, and have it be mandatory. I think our methods will survive their evolution much better.

It's fine for ctx to just be a container of other things, if the original concern was the size of the ctx API.

Just FYI this change will be stalled until there's a resolution here. I can't proceed until we figure out what to do.

My opinion is to require ctx as the first arg to JavaInfo, but that's a migration so I'm not going to go do it unless people agree.

We are not passing `ctx` as an argument to JavaInfo, because that couples JavaInfo semantics too much to the rule that creates that javaInfo. It is inflexible and most of the time produces bad results

It also constrains the implementation of said method.

This came up with the methods in cc_common that return various things about the compiler -- currently, they either look at the configuration, but they will soon look at the CcToolchainProvider which they can get either from toolchain resolution or a dependency. And if we only pass in e.g. ctx.fragments, we'll have to change the Skylark interface in order to change the plumbing between various Java classes. That seemed somewhat unsatisfactory.

--

You received this message because you are subscribed to the Google Groups "bazel-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-discus...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/f8db7c40-1764-412e-9245-0b4c0bb93953%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Liam Miller-Cushon]

On Tue, Apr 10, 2018 at 1:02 AM 'Lukács T. Berki' via bazel-discuss <bazel-...@googlegroups.com> wrote:

We are not passing `ctx` as an argument to JavaInfo, because that couples JavaInfo semantics too much to the rule that creates that javaInfo. It is inflexible and most of the time produces bad results

It also constrains the implementation of said method.

This came up with the methods in cc_common that return various things about the compiler -- currently, they either look at the configuration, but they will soon look at the CcToolchainProvider which they can get either from toolchain resolution or a dependency. And if we only pass in e.g. ctx.fragments, we'll have to change the Skylark interface in order to change the plumbing between various Java classes. That seemed somewhat unsatisfactory.

Is the concern about making JavaInfo inflexible that passing ctx allows JavaInfo to depend on attributes of the current rule? The alternative of passing actions/labels/fragments as separate arguments will make using JavaInfo more verbose and error prone, and make breaking changes to the API more frequent. This seems like a bad trade-off.

[to...@google.com]

Just a reminder that I'm blocked on resolution of this issue, and I do not have decision making power over this bit of code. I don't know how important the issue is, it seems after the SJD problem was fixed this has been downgraded in priority?

[ittai zeidman]

The exports usecase is still important for us and I’d like us to try and understand what we can do with it.
Main motivation is to have a layer of abstraction for a library provider over the bits of their libraries so they can refactor in peace without breaking their clients.
How do you resolve that in google?

[Tom Lundell]

On Thu, Apr 12, 2018 at 12:23 AM ittai zeidman <itt...@gmail.com> wrote:

The exports usecase is still important for us and I’d like us to try and understand what we can do with it.

That's a separate discussion, is it not? It's off-topic for SJD at any rate, for reasons discussed earlier.

I'd prefer to keep this thread focussed on restoring the functionality you did have. It's a one hour job to fix the code at this stage (modulo migration). I just need somebody that owns JavaInfo to tell me how they want me to get the label there.

Maybe continue that chat on the original issue https://github.com/bazelbuild/bazel/issues/4584?

 

--
You received this message because you are subscribed to a topic in the Google Groups "bazel-discuss" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bazel-discuss/mt2llfwzmac/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bazel-discus...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/47dcc1fb-500c-4555-83a9-e466d741609d%40googlegroups.com.

[Dmitry Lomov]

The problem with passing ctx is that the users of the API that accepts ctx _have no information whatsoever_ about which information in ctx is relevant and which is not - and ctx gives you access to *all* information the rule implementation can possibly have. The API that takes a ctx is not an API because it has _no contract_. It is not an API, it as best a functional decomposition.

Not passing ctx as an argument does not "make breaking changes to the API more frequent". It makes breaking changes to the API more apparent.

If you method takes the entire ctx, then every time you depend on one more thing from the ctx in method implementation, you are making a breaking change, and you are doing it _silently_. This is the worst trade-off you can make.

Please make breaking changes explicit.

Responding to various tangential points raised:

> Accepting ctx.actions seems to indicate that you are operating in the context of a rule implementation anyway, I don't quite see how 'actions' is dramatically more narrow for this reason,

ctx.actions is specially designed to be an _action factory_. It gives your API a capability of creating actions without leaking information about attributes, labels, fragments or any other bits about the calling rule. It tells you: this API creates actions on behalf of a rule/aspect - there are no information sidechannels.

> This came up with the methods in cc_common that return various things about the compiler [...]

As this is unrelated to Java, we discussed this offline with Lukacs. The resolution there is that for those specific set of methods, the desirable end state is the methods that do not take the ctx anyway. Some helper functions in pure Skylark can be written to facilitate the transition, but unclear at this stage if they are even needed.

> [java_proto_library] could be supported by passing another argument, "injecting_rule_kind" [...]

> this case is pretty poorly supported anyway, because its support is hard coded. add_deps takes a label and the injecting rule kind and tries to guess the name of the corresponding java_proto_library rule. Eg. "Oh I see it's the target 'foo_proto' but created by a 'java_proto_library', hopefully there exists a rule called 'foo_java_proto' then"

injecting_rule_kind is not really sustainable as it does not generalize to custom rules in general. Any Skylark rule people add will need to be added to strict_deps/add_deps which is impossible to maintain.

We need a different solution if we want to make strict_deps/add_deps diagnostics extensible.

I see two:

- instead of passing label and injecting_rule_kind and whatnot other information, make the rule pass "the instructions to add dependency on this jar" as a string and store those in the ijar manifest. The diagnostics just displays this verbatim (replacing a placeholder with offending dependency target label)

- completely change how a jar to add is discovered. Instead of getting this information from a jar manifest (or, previously, command line), strict_deps will use an aspect to analyze dependencies of offending target. JVM rules will provide a provider that describes, for a target that produces jars, how to add dependencies on those jars. The aspect will discover the closest topologically closest producing target to the offending target and display that information as an "add_deps" suggestion. This will solve proto_library aspect/java_proto_library, exports, and is extensible. It is a large reinvention of how strict deps report errors though.

Maybe there are other ideas here?

As a side note, I am rather uncomfortable with the fact that JavaInfo constructor (which should really be just constructing a data structure) actually creates actions. This API choice seems questionable to me - are we sure that every API usage need to create these specific actions? I understand that actual action creation is controlled by use_ijar, and if we all pinky swear we will never ever create other actions that are controlled by other parameters, it is ok, and not much harm done anyhow if we actually do, but it still is aesthetically unpleasant.

Dmitry

--

Google Germany GmbH

Erika-Mann-Straße 33, 80636 München, Germany

[Tom Lundell]

On Thu, Apr 12, 2018 at 2:56 AM 'Dmitry Lomov' via bazel-discuss <bazel-...@googlegroups.com> wrote:

The problem with passing ctx is that the users of the API that accepts ctx _have no information whatsoever_ about which information in ctx is relevant and which is not - and ctx gives you access to *all* information the rule implementation can possibly have. The API that takes a ctx is not an API because it has _no contract_. It is not an API, it as best a functional decomposition.

Not passing ctx as an argument does not "make breaking changes to the API more frequent". It makes breaking changes to the API more apparent.

If you method takes the entire ctx, then every time you depend on one more thing from the ctx in method implementation, you are making a breaking change, and you are doing it _silently_. This is the worst trade-off you can make.

Please make breaking changes explicit.

Point taken, but there are a class of changes that aren't "breaking", but rather are a reorg/refactor/internal change. Those get harder to do, i.e. we're making non-breaking changes explicit too.

In case it isn't clear I am more than happy to defer this decision to your taste. The main point of this thread from my point of view is to resolve the immediate issue at hand. I'll do whatever it is you want me to do.

 

Responding to various tangential points raised:

> Accepting ctx.actions seems to indicate that you are operating in the context of a rule implementation anyway, I don't quite see how 'actions' is dramatically more narrow for this reason,

ctx.actions is specially designed to be an _action factory_. It gives your API a capability of creating actions without leaking information about attributes, labels, fragments or any other bits about the calling rule. It tells you: this API creates actions on behalf of a rule/aspect - there are no information sidechannels.

> This came up with the methods in cc_common that return various things about the compiler [...]

As this is unrelated to Java, we discussed this offline with Lukacs. The resolution there is that for those specific set of methods, the desirable end state is the methods that do not take the ctx anyway. Some helper functions in pure Skylark can be written to facilitate the transition, but unclear at this stage if they are even needed.

> [java_proto_library] could be supported by passing another argument, "injecting_rule_kind" [...]

> this case is pretty poorly supported anyway, because its support is hard coded. add_deps takes a label and the injecting rule kind and tries to guess the name of the corresponding java_proto_library rule. Eg. "Oh I see it's the target 'foo_proto' but created by a 'java_proto_library', hopefully there exists a rule called 'foo_java_proto' then"

injecting_rule_kind is not really sustainable as it does not generalize to custom rules in general. Any Skylark rule people add will need to be added to strict_deps/add_deps which is impossible to maintain.

Agreed, injecting_rule_kind is totally unprincipled and a hack. Target label isn't unprincipled (there really _is_ exactly one unique creator of a jar), but as we've found it ultimately only does what people want in 80% of cases.

 

We need a different solution if we want to make strict_deps/add_deps diagnostics extensible.

This is all true. Do we want some solution _right now_ though?

If we're not adding ctx, then the only remaining trivial solution seems to be to require the user to pass the target label themselves if they want the jar stamped for add_deps support. This should restore functionality to exactly what it used to be.

 

I see two:

- instead of passing label and injecting_rule_kind and whatnot other information, make the rule pass "the instructions to add dependency on this jar" as a string and store those in the ijar manifest. The diagnostics just displays this verbatim (replacing a placeholder with offending dependency target label)

- completely change how a jar to add is discovered. Instead of getting this information from a jar manifest (or, previously, command line), strict_deps will use an aspect to analyze dependencies of offending target. JVM rules will provide a provider that describes, for a target that produces jars, how to add dependencies on those jars. The aspect will discover the closest topologically closest producing target to the offending target and display that information as an "add_deps" suggestion. This will solve proto_library aspect/java_proto_library, exports, and is extensible. It is a large reinvention of how strict deps report errors though.

Maybe there are other ideas here?

The information available from SJD is at the essence "I'm compiling target T, and there is a SJD violation involving jar X". We could build a contract that says "I will invoke a tool with these two things". Current add_deps could work with this contract, it would simply crack open the jar and find the stamped target label.

And yes, you could very possibly build some tool that traverses the build graph and tries to find the best target to add given all paths from T -> jar X. This could support any heuristic, including exports, respect visibility, and so on, in a way that the SJD machinery isn't positioned to do.

 

As a side note, I am rather uncomfortable with the fact that JavaInfo constructor (which should really be just constructing a data structure) actually creates actions. This API choice seems questionable to me - are we sure that every API usage need to create these specific actions? I understand that actual action creation is controlled by use_ijar, and if we all pinky swear we will never ever create other actions that are controlled by other parameters, it is ok, and not much harm done anyhow if we actually do, but it still is aesthetically unpleasant.

Dmitry

On Tue, Apr 10, 2018 at 11:14 PM Liam Miller-Cushon <cus...@google.com> wrote:

On Tue, Apr 10, 2018 at 1:02 AM 'Lukács T. Berki' via bazel-discuss <bazel-...@googlegroups.com> wrote:

We are not passing `ctx` as an argument to JavaInfo, because that couples JavaInfo semantics too much to the rule that creates that javaInfo. It is inflexible and most of the time produces bad results

It also constrains the implementation of said method.

This came up with the methods in cc_common that return various things about the compiler -- currently, they either look at the configuration, but they will soon look at the CcToolchainProvider which they can get either from toolchain resolution or a dependency. And if we only pass in e.g. ctx.fragments, we'll have to change the Skylark interface in order to change the plumbing between various Java classes. That seemed somewhat unsatisfactory.

Is the concern about making JavaInfo inflexible that passing ctx allows JavaInfo to depend on attributes of the current rule? The alternative of passing actions/labels/fragments as separate arguments will make using JavaInfo more verbose and error prone, and make breaking changes to the API more frequent. This seems like a bad trade-off.

--

Google Germany GmbH

Erika-Mann-Straße 33, 80636 München, Germany

--
You received this message because you are subscribed to a topic in the Google Groups "bazel-discuss" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bazel-discuss/mt2llfwzmac/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bazel-discus...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/CAObu7DE%3DYwyjSRL-6q1Uw%3DNSxD%2B_VsA5_ubLcONXapop4xfwzg%40mail.gmail.com.

[Marcel Hlopko]

+Pedro LF 

You received this message because you are subscribed to the Google Groups "bazel-discuss" group.

To unsubscribe from this group and stop receiving emails from it, send an email to bazel-discus...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/CAPvN2rUxmH7G9g9y2jy7d009YQrFViJ1vc2z6p8yPTtmUm2UJA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Marcel Hlopko | Software Engineer | hlo...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

[Dmitry Lomov]

On Thu, Apr 12, 2018 at 3:33 PM Tom Lundell <to...@google.com> wrote:

On Thu, Apr 12, 2018 at 2:56 AM 'Dmitry Lomov' via bazel-discuss <bazel-...@googlegroups.com> wrote:

The problem with passing ctx is that the users of the API that accepts ctx _have no information whatsoever_ about which information in ctx is relevant and which is not - and ctx gives you access to *all* information the rule implementation can possibly have. The API that takes a ctx is not an API because it has _no contract_. It is not an API, it as best a functional decomposition.

Not passing ctx as an argument does not "make breaking changes to the API more frequent". It makes breaking changes to the API more apparent.

If you method takes the entire ctx, then every time you depend on one more thing from the ctx in method implementation, you are making a breaking change, and you are doing it _silently_. This is the worst trade-off you can make.

Please make breaking changes explicit.

Point taken, but there are a class of changes that aren't "breaking", but rather are a reorg/refactor/internal change. Those get harder to do, i.e. we're making non-breaking changes explicit too.

In case it isn't clear I am more than happy to defer this decision to your taste. The main point of this thread from my point of view is to resolve the immediate issue at hand. I'll do whatever it is you want me to do.

 

Responding to various tangential points raised:

> Accepting ctx.actions seems to indicate that you are operating in the context of a rule implementation anyway, I don't quite see how 'actions' is dramatically more narrow for this reason,

ctx.actions is specially designed to be an _action factory_. It gives your API a capability of creating actions without leaking information about attributes, labels, fragments or any other bits about the calling rule. It tells you: this API creates actions on behalf of a rule/aspect - there are no information sidechannels.

> This came up with the methods in cc_common that return various things about the compiler [...]

As this is unrelated to Java, we discussed this offline with Lukacs. The resolution there is that for those specific set of methods, the desirable end state is the methods that do not take the ctx anyway. Some helper functions in pure Skylark can be written to facilitate the transition, but unclear at this stage if they are even needed.

> [java_proto_library] could be supported by passing another argument, "injecting_rule_kind" [...]

> this case is pretty poorly supported anyway, because its support is hard coded. add_deps takes a label and the injecting rule kind and tries to guess the name of the corresponding java_proto_library rule. Eg. "Oh I see it's the target 'foo_proto' but created by a 'java_proto_library', hopefully there exists a rule called 'foo_java_proto' then"

injecting_rule_kind is not really sustainable as it does not generalize to custom rules in general. Any Skylark rule people add will need to be added to strict_deps/add_deps which is impossible to maintain.

Agreed, injecting_rule_kind is totally unprincipled and a hack. Target label isn't unprincipled (there really _is_ exactly one unique creator of a jar), but as we've found it ultimately only does what people want in 80% of cases.

 

We need a different solution if we want to make strict_deps/add_deps diagnostics extensible.

This is all true. Do we want some solution _right now_ though?

If we're not adding ctx, then the only remaining trivial solution seems to be to require the user to pass the target label themselves if they want the jar stamped for add_deps support. This should restore functionality to exactly what it used to be.

Why not go one further with my 1st solution below and instead of passing in a label, pass in "a message for add_deps"? This is trivial and helps solve protobuf. It does not help solve exports though.

[Tom Lundell]

On Thu, Apr 12, 2018 at 2:50 PM Dmitry Lomov <dsl...@google.com> wrote:

On Thu, Apr 12, 2018 at 3:33 PM Tom Lundell <to...@google.com> wrote:

On Thu, Apr 12, 2018 at 2:56 AM 'Dmitry Lomov' via bazel-discuss <bazel-...@googlegroups.com> wrote:

The problem with passing ctx is that the users of the API that accepts ctx _have no information whatsoever_ about which information in ctx is relevant and which is not - and ctx gives you access to *all* information the rule implementation can possibly have. The API that takes a ctx is not an API because it has _no contract_. It is not an API, it as best a functional decomposition.

Not passing ctx as an argument does not "make breaking changes to the API more frequent". It makes breaking changes to the API more apparent.

If you method takes the entire ctx, then every time you depend on one more thing from the ctx in method implementation, you are making a breaking change, and you are doing it _silently_. This is the worst trade-off you can make.

Please make breaking changes explicit.

Point taken, but there are a class of changes that aren't "breaking", but rather are a reorg/refactor/internal change. Those get harder to do, i.e. we're making non-breaking changes explicit too.

In case it isn't clear I am more than happy to defer this decision to your taste. The main point of this thread from my point of view is to resolve the immediate issue at hand. I'll do whatever it is you want me to do.

 

Responding to various tangential points raised:

> Accepting ctx.actions seems to indicate that you are operating in the context of a rule implementation anyway, I don't quite see how 'actions' is dramatically more narrow for this reason,

ctx.actions is specially designed to be an _action factory_. It gives your API a capability of creating actions without leaking information about attributes, labels, fragments or any other bits about the calling rule. It tells you: this API creates actions on behalf of a rule/aspect - there are no information sidechannels.

> This came up with the methods in cc_common that return various things about the compiler [...]

As this is unrelated to Java, we discussed this offline with Lukacs. The resolution there is that for those specific set of methods, the desirable end state is the methods that do not take the ctx anyway. Some helper functions in pure Skylark can be written to facilitate the transition, but unclear at this stage if they are even needed.

> [java_proto_library] could be supported by passing another argument, "injecting_rule_kind" [...]

> this case is pretty poorly supported anyway, because its support is hard coded. add_deps takes a label and the injecting rule kind and tries to guess the name of the corresponding java_proto_library rule. Eg. "Oh I see it's the target 'foo_proto' but created by a 'java_proto_library', hopefully there exists a rule called 'foo_java_proto' then"

injecting_rule_kind is not really sustainable as it does not generalize to custom rules in general. Any Skylark rule people add will need to be added to strict_deps/add_deps which is impossible to maintain.

Agreed, injecting_rule_kind is totally unprincipled and a hack. Target label isn't unprincipled (there really _is_ exactly one unique creator of a jar), but as we've found it ultimately only does what people want in 80% of cases.

 

We need a different solution if we want to make strict_deps/add_deps diagnostics extensible.

This is all true. Do we want some solution _right now_ though?

If we're not adding ctx, then the only remaining trivial solution seems to be to require the user to pass the target label themselves if they want the jar stamped for add_deps support. This should restore functionality to exactly what it used to be.

Why not go one further with my 1st solution below and instead of passing in a label, pass in "a message for add_deps"? This is trivial and helps solve protobuf. It does not help solve exports though. 

I don't see how stamping an add_deps message is any better than just passing in the label that add_deps should try to add a dep to. If you are able to construct an add_dep command you necessarily know the correct label, let's just pass that?

If you truly do prefer an add_deps message, it will take about a month to swing it around because of various latencies.

[Dmitry Lomov]

On Thu, Apr 12, 2018 at 9:18 PM Tom Lundell <to...@google.com> wrote:

On Thu, Apr 12, 2018 at 2:50 PM Dmitry Lomov <dsl...@google.com> wrote:

On Thu, Apr 12, 2018 at 3:33 PM Tom Lundell <to...@google.com> wrote:

On Thu, Apr 12, 2018 at 2:56 AM 'Dmitry Lomov' via bazel-discuss <bazel-...@googlegroups.com> wrote:

The problem with passing ctx is that the users of the API that accepts ctx _have no information whatsoever_ about which information in ctx is relevant and which is not - and ctx gives you access to *all* information the rule implementation can possibly have. The API that takes a ctx is not an API because it has _no contract_. It is not an API, it as best a functional decomposition.

Not passing ctx as an argument does not "make breaking changes to the API more frequent". It makes breaking changes to the API more apparent.

If you method takes the entire ctx, then every time you depend on one more thing from the ctx in method implementation, you are making a breaking change, and you are doing it _silently_. This is the worst trade-off you can make.

Please make breaking changes explicit.

Point taken, but there are a class of changes that aren't "breaking", but rather are a reorg/refactor/internal change. Those get harder to do, i.e. we're making non-breaking changes explicit too.

In case it isn't clear I am more than happy to defer this decision to your taste. The main point of this thread from my point of view is to resolve the immediate issue at hand. I'll do whatever it is you want me to do.

 

Responding to various tangential points raised:

> Accepting ctx.actions seems to indicate that you are operating in the context of a rule implementation anyway, I don't quite see how 'actions' is dramatically more narrow for this reason,

ctx.actions is specially designed to be an _action factory_. It gives your API a capability of creating actions without leaking information about attributes, labels, fragments or any other bits about the calling rule. It tells you: this API creates actions on behalf of a rule/aspect - there are no information sidechannels.

> This came up with the methods in cc_common that return various things about the compiler [...]

As this is unrelated to Java, we discussed this offline with Lukacs. The resolution there is that for those specific set of methods, the desirable end state is the methods that do not take the ctx anyway. Some helper functions in pure Skylark can be written to facilitate the transition, but unclear at this stage if they are even needed.

> [java_proto_library] could be supported by passing another argument, "injecting_rule_kind" [...]

> this case is pretty poorly supported anyway, because its support is hard coded. add_deps takes a label and the injecting rule kind and tries to guess the name of the corresponding java_proto_library rule. Eg. "Oh I see it's the target 'foo_proto' but created by a 'java_proto_library', hopefully there exists a rule called 'foo_java_proto' then"

injecting_rule_kind is not really sustainable as it does not generalize to custom rules in general. Any Skylark rule people add will need to be added to strict_deps/add_deps which is impossible to maintain.

Agreed, injecting_rule_kind is totally unprincipled and a hack. Target label isn't unprincipled (there really _is_ exactly one unique creator of a jar), but as we've found it ultimately only does what people want in 80% of cases.

 

We need a different solution if we want to make strict_deps/add_deps diagnostics extensible.

This is all true. Do we want some solution _right now_ though?

If we're not adding ctx, then the only remaining trivial solution seems to be to require the user to pass the target label themselves if they want the jar stamped for add_deps support. This should restore functionality to exactly what it used to be.

Why not go one further with my 1st solution below and instead of passing in a label, pass in "a message for add_deps"? This is trivial and helps solve protobuf. It does not help solve exports though. 

I don't see how stamping an add_deps message is any better than just passing in the label that add_deps should try to add a dep to. If you are able to construct an add_dep command you necessarily know the correct label, let's just pass that?

Well, with a custom message one could construct a better diagnostics for protobuf case.

But a label also works, as long as it is clear what is its intended use.

I am quite sure that the principled solution for add_deps need to involve post-factum analysis of the build graph, like I outlinedю

[Tom Lundell]

So can we get a green light to pass a label into JavaInfo? If present, the jar is stamped with this label, and add_deps works. Going first, going second...?

 

I am quite sure that the principled solution for add_deps need to involve post-factum analysis of the build graph, like I outlinedю

Yes, I think we agree here. SJD should provide a hook to invoke some binary with the target and the jar. I feel like there is already something like that actually. Liam?

[Liam Miller-Cushon]

On Thu, Apr 12, 2018 at 12:31 PM Tom Lundell <to...@google.com> wrote:

So can we get a green light to pass a label into JavaInfo? If present, the jar is stamped with this label, and add_deps works. Going first, going second...?

Sold!

(Assuming the Java rule owners have bought in to the decision about `JavaInfo(ctx.actions, ctx.label, ...)` vs. `JavaInfo(ctx, ...)`? )

 

I am quite sure that the principled solution for add_deps need to involve post-factum analysis of the build graph, like I outlinedю

Yes, I think we agree here. SJD should provide a hook to invoke some binary with the target and the jar. I feel like there is already something like that actually. Liam?

Right, SJD is factored so we have the suggested fix use other tools instead of buildozer. What is the "target and the jar" part? Isn't having the two targets (the dependency to add, and the target to add it to) necessary and sufficient?

Once we have those targets, I agree that some kind of post-build analysis in the tool that adds the dependency is the way to go to handle visibility/exports/aspects. I've brought that up earlier in the thread, so maybe there's still some disagreement that I'm not understanding?

I cc'd Carmi earlier because jadep already does most of this, and is now open source: https://github.com/bazelbuild/bazel/issues/4584#issuecomment-380814286

 

[Irina Iancu]

On Fri, Apr 13, 2018 at 12:48 AM Liam Miller-Cushon <cus...@google.com> wrote:

On Thu, Apr 12, 2018 at 12:31 PM Tom Lundell <to...@google.com> wrote:

So can we get a green light to pass a label into JavaInfo? If present, the jar is stamped with this label, and add_deps works. Going first, going second...?

Sold!

Having yet another flag on JavaInfo is not ideal since it puts more load on the user.  It looks like passing redundant information. IMO Skylark should have a mechanism such as the native implementation of java_common would know all basic information like labels, action factory, toolchain, javabase. Another idea would be to create a new provider to wrap all the repetitive information and pass that to JavaInfo. That would also require a migration. But adding a label to the constructor would also require a migration, since I suppose it will be a mandatory argument.

[Dmitry Lomov]

On Fri, Apr 13, 2018 at 2:29 PM Irina Iancu <elena...@google.com> wrote:

On Fri, Apr 13, 2018 at 12:48 AM Liam Miller-Cushon <cus...@google.com> wrote:

On Thu, Apr 12, 2018 at 12:31 PM Tom Lundell <to...@google.com> wrote:

So can we get a green light to pass a label into JavaInfo? If present, the jar is stamped with this label, and add_deps works. Going first, going second...?

Sold!

Having yet another flag on JavaInfo is not ideal since it puts more load on the user.  It looks like passing redundant information.

 

IMO Skylark should have a mechanism such as the native implementation of java_common would know all basic information like labels, action factory, toolchain, javabase.

My objections to "just passing ctx" apply here as well. Let's not rehash these.

 

Another idea would be to create a new provider to wrap all the repetitive information and pass that to JavaInfo.

You mean some well documented "parameter object" that packs all information related to java_common into one single data structure?

Certainly not opposed to this.

[Irina Iancu]

You mean some well documented "parameter object" that packs all information related to java_common into one single data structure?

Well documented, of course. What do you mean by "parameter object"? How is that different from a provider? Packs all information related to java_common - yes. 

[Dmitry Lomov]

On Fri, Apr 13, 2018 at 3:14 PM Irina Iancu <elena...@google.com> wrote:

You mean some well documented "parameter object" that packs all information related to java_common into one single data structure?

Well documented, of course. What do you mean by "parameter object"? How is that different from a provider? Packs all information related to java_common - yes. 

I meant in a sense of https://refactoring.com/catalog/introduceParameterObject.html.

 

On Fri, Apr 13, 2018 at 2:43 PM Dmitry Lomov <dsl...@google.com> wrote:

On Fri, Apr 13, 2018 at 2:29 PM Irina Iancu <elena...@google.com> wrote:

On Fri, Apr 13, 2018 at 12:48 AM Liam Miller-Cushon <cus...@google.com> wrote:

On Thu, Apr 12, 2018 at 12:31 PM Tom Lundell <to...@google.com> wrote:

So can we get a green light to pass a label into JavaInfo? If present, the jar is stamped with this label, and add_deps works. Going first, going second...?

Sold!

Having yet another flag on JavaInfo is not ideal since it puts more load on the user.  It looks like passing redundant information.

 

IMO Skylark should have a mechanism such as the native implementation of java_common would know all basic information like labels, action factory, toolchain, javabase.

My objections to "just passing ctx" apply here as well. Let's not rehash these.

 

Another idea would be to create a new provider to wrap all the repetitive information and pass that to JavaInfo.

You mean some well documented "parameter object" that packs all information related to java_common into one single data structure?

Certainly not opposed to this.

--

Google Germany GmbH

Erika-Mann-Straße 33, 80636 München, Germany

[Irina Iancu]

Yes

[Liam Miller-Cushon]

On Wed, Apr 11, 2018 at 11:56 PM Dmitry Lomov <dsl...@google.com> wrote:

The problem with passing ctx is that the users of the API that accepts ctx _have no information whatsoever_ about which information in ctx is relevant and which is not - and ctx gives you access to *all* information the rule implementation can possibly have.

If the implementation of JavaInfo restricts itself to information that is present in all rules (like actions and labels) what issues does this cause in practice?

Is the main problem that it gives us the ability to make bad implementation decisions in JavaInfo? I understand that, for example, having JavaInfo use the context to access particular attributes is problematic.

 

The API that takes a ctx is not an API because it has _no contract_. It is not an API, it as best a functional decomposition.

(Can you expand on this? I'm not sure what distinction you're making. This might just be a terminology question.)

 

Not passing ctx as an argument does not "make breaking changes to the API more frequent". It makes breaking changes to the API more apparent.

If you method takes the entire ctx, then every time you depend on one more thing from the ctx in method implementation, you are making a breaking change, and you are doing it _silently_. This is the worst trade-off you can make.

The specific example in this thread is that JavaInfo currently needs actions, and in the future we would like it to have access to the label of the current rule. If ctx was already being passed in, accessing ctx.label in addition to ctx.actions seems like it would not be a breaking change? On the other hand, adding a new parameter to JavaInfo for the label *is* a breaking change.

[Dmitry Lomov]

On Tue, Apr 17, 2018 at 10:16 PM Liam Miller-Cushon <cus...@google.com> wrote:

On Wed, Apr 11, 2018 at 11:56 PM Dmitry Lomov <dsl...@google.com> wrote:

The problem with passing ctx is that the users of the API that accepts ctx _have no information whatsoever_ about which information in ctx is relevant and which is not - and ctx gives you access to *all* information the rule implementation can possibly have.

If the implementation of JavaInfo restricts itself to information that is present in all rules (like actions and labels) what issues does this cause in practice?

Actions (ctx.actions) is not "information": it is a factory specially designed for being passed around without leaking information.

The labels are present in all rules, but they might not _mean_ the same thing w.r.t JavaInfo for all rules.

For java_library, the resulting jar is the primary output of the target denoted by the label, so can be used in add_deps directly.

For aspect applied to the proto_library, that is not the case, some extra information needs to be used (no, injecting rule kind is a very bad design for this, as we discussed on this thread; please do not go that route; have a proper solution instead that does not require to maintain a central registry of all Skylark rules).

For some other not-yet-existing usage of JavaInfo, the jar produced should never be dependent upon, maybe, so that label needs to be nil.

The fact that information "is present in all rules" does not mean that your API can use it directly. 

API needs to be adaptable to the variety of usage, and by passing ctx you tightly couple the API to one specific usage you have in mind. It seems like an API, but in fact the only caller that it ever caters for is a java_library rule.

 

Is the main problem that it gives us the ability to make bad implementation decisions in JavaInfo? I understand that, for example, having JavaInfo use the context to access particular attributes is problematic.

 

The API that takes a ctx is not an API because it has _no contract_. It is not an API, it as best a functional decomposition.

(Can you expand on this? I'm not sure what distinction you're making. This might just be a terminology question.)

[see below, expansion of the things you intend to get from ctx] 

 

Not passing ctx as an argument does not "make breaking changes to the API more frequent". It makes breaking changes to the API more apparent.

If you method takes the entire ctx, then every time you depend on one more thing from the ctx in method implementation, you are making a breaking change, and you are doing it _silently_. This is the worst trade-off you can make.

The specific example in this thread is that JavaInfo currently needs actions, and in the future we would like it to have access to the label of the current rule. If ctx was already being passed in, accessing ctx.label in addition to ctx.actions seems like it would not be a breaking change? On the other hand, adding a new parameter to JavaInfo for the label *is* a breaking change.

Generally, If you know that you will only ever need access to ctx.actions and ctx.label,  just pass those two parameters (unconditionally passing label is problematic for this use case as well, as I said above, but speaking generally). Cleaner API clearly outweights the cost of migration.

What people on this thread are saying though is, we will pass ctx, and we will take a label from the ctx today, *and we will take other things we don't know about yet from ctx tomorrow*. This latter intent is extremely problematic, because you are pushing for an API with undefined contract.

HTH,

Dmitry

[Tom Lundell]

A document with a solution proposal for add_dep and JavaInfo is being discussed here: https://docs.google.com/document/d/1ubah6phuvWnugShtVgSQnaopQ1BtKtNxQASVwGZA7k0/edit?usp=sharing

--
You received this message because you are subscribed to a topic in the Google Groups "bazel-discuss" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/bazel-discuss/mt2llfwzmac/unsubscribe.
To unsubscribe from this group and all its topics, send an email to bazel-discus...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/CAObu7DGkzLgFo-SKYYON4iEd%2BP%2B-3-YRznHBFs6-hoL_uvZvOg%40mail.gmail.com.

[Paul Draper]

I am for doing the job silently, magically behind the API. I don't want to add yet one more flag to the constructor.

IDK what "magic" this is, but it's magic that would be unavailable to a hypothetical Skylark reimplementation of JavaInfo, that would be a poor direction.

[Paul Draper]

I'm actually a little confused on the case for labels of transitive items. (Simple repro?) I'd think that labels on direct items are useful. But less so transitive ones.

FYI ittai, you might look at is aspects. You can use them to layer on nearly any analysis you can think of, without requiring participation from other rules or providers. Leaving aside the important question of "should", aspects "can" solve just about anything mentioned in this thread.

[ittai zeidman]

The case is strict-deps

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/6a2f27fb-f127-43ff-8f4c-9d17c62f78f6%40googlegroups.com.

[Paul Draper]

I don't what I was thinking. For some reason I was thinking "unused deps" (for which you only need the immediate labels).

Here's an example I'm working on using aspects: https://github.com/andyscott/rules_scala_annex/pull/51

I'm still testing it, but I think it works, for strict and unused deps, for deps and exports.

When selecting which label to recommend adding, it chooses the nearest label (because it uses a preporder depset).

The aspect portions:

load("//rules/jvm:private/label.bzl", _labeled_jars_implementation = "labeled_jars_implementation")

labeled_jars = aspect(
    implementation = _labeled_jars_implementation,
    attr_aspects = ["deps"],  # assumption
)

---

load("@rules_scala_annex//rules/jvm:provider.bzl", "LabeledJars")

def labeled_jars_implementation(target, ctx):
    if JavaInfo not in target:
        return []

    manifest = ctx.actions.declare_file("{}/jar_exports.txt".format(ctx.label.name))
    args = ctx.actions.args()
    if hasattr(args, "add_all"):  # Bazel 0.13.0+
        args.add_all(target[JavaInfo].compile_jars)
    else:
        args.add(target[JavaInfo].compile_jars)
    args.set_param_file_format("multiline")
    ctx.actions.write(manifest, args)

    deps_labeled_jars = [dep[LabeledJars] for dep in ctx.rule.attr.deps if LabeledJars in dep]
    return [
        LabeledJars(
            labeled_manifests = depset(
                [struct(label = ctx.label, manifest = manifest)],

                order = "preorder",
                transitive = [labeled_jars.labeled_manifests for labeled_jars in deps_labeled_jars],
            ),
            manifests = depset(
                [manifest],
                transitive = [labeled_jars.manifests for labeled_jars in deps_labeled_jars],
            ),
        ),
    ]

I make file manifests of the jars provided by every JavaInfo rule, following the "deps" attribute.

Pathological behavior is possible, though it done lazily, at exec time.

The laziness could have also been done using just actions.args().

[Tom Lundell]

The design has been implemented and the new JavaInfo constructor should be available at bazel head if anyone wants to play around with it. I have some more work to do with deprecation flags and regression tests, but if anyone wants to preflight add_dep support it's there.

For no-ijar rules like Scala it should be a matter of pulling the output jar through java_common.stamp_jar and feeding it to JavaInfo via compile_jar. Once you use a new-style arg you'll be instructed to remove the rest of the legacy args, like use_ijar.

[ittai zeidman]

Thanks for the update.
By “if anyone wants to preflight add_dep support” you mean to add *better* support which will answer the missing parts we discussed here (like exports), right?
The existing somewhat broken add_dep support will still work but obviously we want to have the full functionality.

To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-discuss/CAPvN2rXpij1sN%3DnXFVTZ04aEEwcmt%2BTsk0KP6%3DAmEKQk7OJ5EQ%40mail.gmail.com.