Toolchain deps infra #1072

liucijus · 2020-07-21T14:40:56Z

This is the first actual PR from Deps Toolchains Infrastructure (#1067)

Adds reusable infra to develop toolchains for deps
Adds docs
Minimizes deps required for minimal Scala setup

Opinionated decisions (welcome to discuss!):

common deps are added on existing scala toolchain (proposal had a separate deps toolchain) - this is done for simplicity
guava (moved to scrooge setup) and commons io were removed. The reusability gain was minimal, but has added more complexity for users. cc @blorente

Question: maybe we also want to migrate ScalacProvider to DepsInfo map?

N.B. This is a breaking change for users who do not use repositories macros

ittaiz · 2020-07-22T09:29:26Z

Thanks for this! I think there's a lot of sense in having a unified mechanism.
Not sure if there are cons from moving ScalacProvider itself.
WDYT about sending a commit that moves it and we can understand from it if there are cons?

ittaiz · 2020-07-22T09:34:44Z

scala/toolchains/README.md

@@ -0,0 +1,153 @@
+# Developing Toolchains for Deps


@katre is there a chance you can take a look at this toolchains pattern?
I think this might be valuable for rulesets in general and not only rules_scala.
If you think we're abusing toolchains or just misunderstanding it we'd also love to know that.
One concrete question is whether using a map here is the best solution?

I looked over the PR and the docs, and I'm afraid I don't fully understand what's being implemented. You are adding dependencies to the toolchain that will then also be provided to other, non-toolchain-aware rules, via the normal transitive dependencies? Can you give a concrete example of where this would be useful?

As for the map, I am also confused about why it is a map from a label (which seems to always be a declare_deps_provider) to a string (which is a unique provider id?). Why this direction and not a map of provider id to label?

Lastly, if you are updating your toolchains, have you seen the design for Toolchain Transitions, including the migration steps? This will allow you to declare some toolchain dependencies as being in the target config (for the target being built with the toolchain), and others in the exec config, which may be useful with these deps.

I looked over the PR and the docs, and I'm afraid I don't fully understand what's being implemented. You are adding dependencies to the toolchain that will then also be provided to other, non-toolchain-aware rules, via the normal transitive dependencies? Can you give a concrete example of where this would be useful?

Example: scalac target is java_binary which isn't aware of scala_toolchain.

As for the map, I am also confused about why it is a map from a label (which seems to always be a declare_deps_provider) to a string (which is a unique provider id?). Why this direction and not a map of provider id to label?

I haven't found a type for string to label mapping (https://docs.bazel.build/versions/master/skylark/lib/attr.html). I don't like having inverted dict as a compromise, but I failed to find a better alternative.

Lastly, if you are updating your toolchains, have you seen the design for Toolchain Transitions, including the migration steps? This will allow you to declare some toolchain dependencies as being in the target config (for the target being built with the toolchain), and others in the exec config, which may be useful with these deps.

I've looked at toolchain transitions, but I do not see how it fits into our case. Are there good examples to learn from?

If scalac is a java_binary, why doesn't it have a complete set of deps? Can it only be built as a dependency of a scala target? Or have I misunderstood the build graph?

Instead of a string_to_label dict, the typical approach is to use a label_list, and when you get the DepInfo providers (is that the right provider from this attribute?) from the attribute, each DepsInfo has the provider id. Are you ever actually using the same target as different provider ids in different toolchains?

Toolchain transitions are new, and we're still in the migration period. The basic idea is that it allows your toolchain to separate out the tool dependencies (like scalac), which need to run on the execution platform, from your library dependencies (which are actually linked into your final target), and which need to be in the target configuration.

I'll move provider id to DepsInfo. I don't like this approach, but it is semantically more correct. Mapping ids to labels feels more declarative and would be good for mapping the same targets more than once, but inverted map does not allow the same label twice anyway.

Thanks, I agree that the previous labe->id map was odd. Feel free to reach out to the Starlark API team on bazel-users and ask about this, but I suspect the answer will be that the label_list system you have is more "bazel-y".

In general, I really like this approach for solving the problem of adding transitive deps to things that consume scala targets. I think linking future toolchain authors to your docs will definitely be useful. Would it be possible for the author to also write a blog post or some other article with a high-level discussion to point other rule authors to? I realize this is even more work, but it could really help the community.

liucijus · 2020-07-23T12:35:58Z

I have migrated scalac_provider_attr to dep_providers, but come to conclusion that leaving concept of ScalacProvider is quite useful abstraction, so I left it inside phases.

ittaiz · 2020-07-24T07:09:57Z

have migrated scalac_provider_attr to dep_providers, but come to conclusion that leaving concept of ScalacProvider is quite useful abstraction, so I left it inside phases.

Can you elaborate on why?

liucijus · 2020-07-25T07:19:17Z

have migrated scalac_provider_attr to dep_providers, but come to conclusion that leaving concept of ScalacProvider is quite useful abstraction, so I left it inside phases.

Can you elaborate on why?

Mostly to have less changes in current phases. Also scala deps have "special" meaning in scala rules which are design around them. Maybe this needs to be redesigned but I would prefer to do it outside toolchains scope, which is already quite big.

ittaiz · 2020-07-25T11:42:26Z

To make sure I understand- the problem is only priorities and breaking down tasks? I'm treating scalac provider as another validation of your design and want to understand if we got negative signals

liucijus · 2020-07-25T15:57:03Z

I have refactored dep_providers to label list

liucijus · 2020-07-25T16:11:12Z

To make sure I understand- the problem is only priorities and breaking down tasks? I'm treating scalac provider as another validation of your design and want to understand if we got negative signals

The question is if we want to change current phases. Now there's single phase which brings three depsets with ScalacProvider. Maybe it makes sense to split it into three new phases per depset and get rid of ScalacProvider. I didn't do it as I have no strong opinion about it and it was very easy to hook into existing phase. In general it's no a complicated change, but will touch quite a few places in the current code.

katre

Thanks for tagging me in, this is really interesting to see.

katre · 2020-07-27T12:52:05Z

scala/providers.bzl

-        "default_repl_classpath": attr.label_list(allow_files = True),
-        "default_macro_classpath": attr.label_list(allow_files = True),
+        "deps": attr.label_list(allow_files = True),
+        "depset_id": attr.string(mandatory = True),


Unless this is actually somehow a depset, I would suggest renaming.

Thanks, I will rename it

katre · 2020-07-27T12:54:43Z

scala/toolchains/README.md

@@ -0,0 +1,153 @@
+# Developing Toolchains for Deps


Thanks, I agree that the previous labe->id map was odd. Feel free to reach out to the Starlark API team on bazel-users and ask about this, but I suspect the answer will be that the label_list system you have is more "bazel-y".

In general, I really like this approach for solving the problem of adding transitive deps to things that consume scala targets. I think linking future toolchain authors to your docs will definitely be useful. Would it be possible for the author to also write a blog post or some other article with a high-level discussion to point other rule authors to? I realize this is even more work, but it could really help the community.

simuons · 2020-07-28T09:14:04Z

scala/toolchains/README.md

+declare_deps_toolchain(
+    name = "my_toolchain_impl",
+    dep_providers = {
+        ":my_compile_deps_provider": "compile_deps",


I think this is outdated

Thanks for catching, I will update docs

ittaiz · 2020-07-31T15:40:29Z

seems as lint fails

liucijus · 2020-08-03T16:59:30Z

@ittaiz, I have fixed lint error and issues mentioned in the comments

liucijus requested a review from ittaiz as a code owner July 21, 2020 14:40

googlebot added the cla: yes label Jul 21, 2020

ittaiz reviewed Jul 22, 2020

View reviewed changes

katre reviewed Jul 27, 2020

View reviewed changes

simuons reviewed Jul 28, 2020

View reviewed changes

liucijus added 12 commits August 3, 2020 17:39

Add scala toolchain deps infra

87e2237

Add scalac_provider_attr doc

34f8ea4

Remove guava and commons_io deps

7d4185c

Extract minimal rules_scala_setup()

cae2214

Fix aspect tests

88486e2

Migrate ScalacProvider attribute to DepsInfo mmapping

dd02714

Remove ScalacProvider deps rule

11d88ec

Update docs according ScalacProvider migration to DepsInfo

ff43342

Refactor dep_provider map to DepInfo id field

4afd239

Update stale docs

dd7a7ea

Rename depset_id to deps_id in DepInfo provider

011ee99

Lint

8e6375a

liucijus force-pushed the toolchain-deps-infra branch from 07f87cf to 8e6375a Compare August 3, 2020 15:05

ittaiz approved these changes Aug 3, 2020

View reviewed changes

ittaiz merged commit eabb1d2 into bazelbuild:master Aug 3, 2020

liucijus deleted the toolchain-deps-infra branch August 4, 2020 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolchain deps infra #1072

Toolchain deps infra #1072

liucijus commented Jul 21, 2020

ittaiz commented Jul 22, 2020

ittaiz Jul 22, 2020

katre Jul 22, 2020

liucijus Jul 22, 2020

katre Jul 23, 2020

liucijus Jul 25, 2020

katre Jul 27, 2020

liucijus commented Jul 23, 2020

ittaiz commented Jul 24, 2020

liucijus commented Jul 25, 2020

ittaiz commented Jul 25, 2020

liucijus commented Jul 25, 2020

liucijus commented Jul 25, 2020

katre left a comment

katre Jul 27, 2020

liucijus Jul 31, 2020

katre Jul 27, 2020

simuons Jul 28, 2020

liucijus Jul 31, 2020

ittaiz commented Jul 31, 2020

liucijus commented Aug 3, 2020

Toolchain deps infra #1072

Toolchain deps infra #1072

Conversation

liucijus commented Jul 21, 2020

ittaiz commented Jul 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liucijus commented Jul 23, 2020

ittaiz commented Jul 24, 2020

liucijus commented Jul 25, 2020

ittaiz commented Jul 25, 2020

liucijus commented Jul 25, 2020

liucijus commented Jul 25, 2020

katre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz commented Jul 31, 2020

liucijus commented Aug 3, 2020