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

Factor toolchains #978

Closed
wants to merge 11 commits into from
Closed

Conversation

smparkes
Copy link
Contributor

@smparkes smparkes commented Jan 30, 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 step towards factoring scala configurations and tools into toolchains and providers as described in the bazel docs.

There are multiple toolchain-like objects in our current code, one of which is a toolchain and others of which are named after providers. Among other things, this PR makes the core objects that are toolchains into toolchains. This change makes the selection of the toolchains happen via standard toolchain resolution, removing the hard-coding based on labels.

This PR doesn't cleanup everything: I tried to scope it to the minimal changes necessary to use toolchain resolution. Although it creates multiple toolchains to handle the various tools, the attrs on those toolchains haven't all been cleaned up. I hope multiple PRs will be easier to review than one omnibus PR. (Progress over perfection.) I'm actually working on those subsequent PRs immediately so things should continue to progress.

Change details

  • Create separate toolchain types and default toolchains for bootstrap, compile and test phases. The bootstrap toolchain contains what has in the past been in the ScalacProvider. The bootstrap toolchain is used to build the scalac driver since it can't depend on itself and subsumes declare_scalac_provider. The scala and test toolchains have not been factored yet. The compile toolchains is pretty much what we had before. The test toolchain will be used to move some of the test stuff out of the compile toolchain.
  • Register the standard toolchains in WORKSPACE: since default toolchain resolution now replaces hard-coding, the tests need the toolchains registered.
  • Move ScalacProvider to the bootstrap toolchain and rename it BootstrapInfo to follow the pattern in bazel docs. Simplify the names.
  • Adjust all places where the toolchain providers are used to match above. Note that there's still more factoring to be done on this code, e.g., scalac_provider and scalac_provider_attr should be renamed or go away. In particular, I think phase_scalac_provider becomes phase_bootstrap_info. I almost made this change now because it seems like it makes things clearer but decided not to to keep the PR small.
  • Add the bootstrap toolchain to rules where needed.
  • I tweaked theskylib load in twitter_scrooge: the warning's been bugging me and it's fatal in newer versions of skylib.
  • I added a simple "hello, world" example in. Not sure if we have this somewhere? This is for my testing and I'm happy to keep it or ditch it.

Motivation

Beyond cleanup/tech debt reduction, this is required by the multiscala work in #962.

Outstanding

We could do this a couple of different ways. See looking for advice on linked toolchains vs. parallel toolchains. I took the approach that I prefer and works best with the current factoring in our phases where we share a lot of code in ways that would be difficult with linked toolchains. It's relatively easy to swap if there's a consensus for the other approach.

@smparkes
Copy link
Contributor Author

smparkes commented Jan 31, 2020

Responding to @andyscott in #984 (comment)_

can we rename bootstrap_toolchain to scala_toolchain?

There are three toolchains at this point: bootstrap_toolchain, toolchain and test_toolchain.

I don't have too strong of an opinion on this but I will say that they're annoyingly easy to confuse so naming matters more than it might otherwise.

I don't think it would be obvious what scala_toolchain means compared to toolchain. That might just mean changing toolchain to compiler_toolchain. What is now called toolchain used to be called scala_toolchain and maybe that's better.

Options (pick one of each ...)
For the bootstrap: bootstrap_toolchain, library_toolchain, scala_toolchain
For the compiler and tools: toolchain, scala_toolchain, compiler_toolchain, scalac_toolchain
For tests: test_toolchain. scalatest_toolchain, scala_test_toolchain

"scala_toolchain" feels very ambiguous
"compiler" might be misleading because it might include other tools (haven't gotten back to that yet ...)

These are all fairly easy to change.

@liucijus
Copy link
Collaborator

liucijus commented Mar 6, 2020

@smparkes please rebase this PR on top of the master. I would like to test it with our code base to see if it breaks us.

@smparkes
Copy link
Contributor Author

smparkes commented Mar 6, 2020

@liucijus: plan to. Appreciate the feedback. Plan to respond but need a couple of days to get through another task.
Thanks!!!

@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