Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multiscala configuration #964

Closed
wants to merge 55 commits into from
Closed

Conversation

smparkes
Copy link
Contributor

@smparkes smparkes commented Jan 27, 2020

Please look at #962 and consider earlier uncommitted PRs first. This PR is based on those PRs and will give you smaller PRs to review.

Description

This is the first PR for #962 . It creates/uses the configuration structure necessary to configure multiple scala toolchains but at this point only uses the default toolchain.

Changes to multiscala

  • create unstable/multiscala
  • combine user configuration and default configuration to create @io_bazel_rules_scala_configuration repo
  • maven_install rules_scala dependencies based on @io_bazel_rules_scala_configuration
  • create bind aliases for rules_scala dependencies when there is a default scala version
  • create scala_toolchain for configured versions
  • register scala_toolchain if there is a configured default version
  • create unstable/multiscala/private/example which can be run with bazel build :all && bazel run :all

Changes to stable code

  • add TemplateVariableInfo provider to scala_toolchain. Required when used as the value for the standard toolchains attr
  • create and use a few bindings so that they can also be configured in multiscala. Although I think long term we may want to remove all bindings, for now they're key to making multiscala opt-in.

Motivation

First step in being able to configure multiple toolchains based on a configuration with multiple scala versions.

This creates the toolchains but doesn't, at this point, use them. That will come in a subsequent PR.

@andyscott
Copy link
Contributor

I am generally very much in favor of this functionality.

That being said, I believe providers [0]-- not toolchains-- are the right long term approach for supporting this and my fear is that merging this complicates some of the existing cleanup and feature work that's already underway.

I haven't had a chance to lay out my arguments for ditching toolchains in favor of providers, so perhaps we need to finally kick off that discussion. WDYT @ittaiz?


[0] I've explored the provider approach in https://github.com/higherkindness/rules_scala. I believe this approach can be improved and simplified.

Turns out I made some mistakes on the native toolchain name vs. the implementtion name. Not for the first time.

Also had to add TemplateVariableInfo to the core scala_toolchain as required when using the standard toolchains attr.
Simply initialized it with an empty map (not really familiar with this provider ...)
@smparkes
Copy link
Contributor Author

A few thoughts:

You can think of this whole effort as a couple of big pieces. One is the tool/provider construction, execution and dependency management.

The other is the configuration and use issue, independent of how the tools are structured and managed. Things like do I have to explicitly ask for every target in every version I care about? I started with a POC where I just copied and hard-coded the versions I wanted. I moved on to explicit starlark loops for every target. That's feasible but that's when I determined that IMNSHO that would be too big a barrier: it's just too much to ask every build file to become a shim to a starlark file with a loop. I would hate that. It'd be one thing if that was the only choice but we can automate that away, so most people don't have to learn much starlark and also removes a lot of boilerplate. I'm working on a simple example of that to show soon.

The configuration and use stuff is, to a great extent, independent of how the tools are structured and managed. It has to be implemented against that stuff but that's moderately easily changeable and the API to the user doesn't need to change.

I had seen the ScalaConfiguration stuff though I hadn't really delved too deeply.

Looking at it now, it's not clear to me how different it is from a standard toolchain. If declare_scala_configuration returned a ToolchainInfo, I think it would, strictly speaking, be a toolchain(?) or at least a toolchain impl. Agree that conflicts with other work is always a concern. In my first stab, there really wasn't that much change on a lot of the core stuff, e.g., the phase stuff. Not sure what I might be overlooking. And there was still work underway (scalapb, scrooge, etc.) when I decided it would be better to approach things incrementally.

I think pretty much everything that can be done without toolchains can be done with toolchains in the areas we're talking about. IIUC, the primary difference is the extra behavior for toolchains built in to bazel, in particular toolchain resolution, e.g., ctx.toolchains. Although we can't exactly use that with CLI flags and transitions for multiple scala versions at this point (e.g., naming issues, transitions only on rules), there's support (for some definition of support) for an integrated toolchains attr in the standard API that looks useful. I'm still following up on the status of this and its behavior: looks to me like no one is using it. That said, it wouldn't be a blocker if it was dropped.

So it really comes down to factoring of dependencies and configurations, and I'm looking at that. For example, it took a bit of work to factor things so that the various tools in src/java/io/bazel/rulesscala/ could be built against multiple versions of scala. I think there's monolithicness here that we could reduce: e.g., use more than one toolchain/provider so that test flags (dependencies?) are only pulled in if you do a test, not for libraries and binaries. But all this work could be done with toolchains or ad hoc rules/providers. Just seems like toolchains is the "right" approach since that's what they are and toolchain resolution is potentially useful.

One thing I'm not particularly comfortable/familiar with is the host/execution/target stuff. AFAICT, we're somewhat isolated from that because everything is on the JVM.

Looking forward to more/others thoughts.

@smparkes
Copy link
Contributor Author

smparkes#2 is a pull request against this PR that shows early work on the scala_* macros.

This moves the scalac attr to the toolchain so it can be overriden by
different toolchain implementations.

It also adds a `ScalaInfo` provider level for scala_toolchain to follow the
pattern in the scala docs (rather than flattening into the ToolchainInfo).
@smparkes
Copy link
Contributor Author

At this point, there's not enough need/support for this (including myself) to continue forward.

@smparkes smparkes closed this May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants