-
Notifications
You must be signed in to change notification settings - Fork 282
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
Factor toolchains #978
Conversation
Responding to @andyscott in #984 (comment)_
There are three toolchains at this point: 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 Options (pick one of each ...) "scala_toolchain" feels very ambiguous These are all fairly easy to change. |
@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. |
@liucijus: plan to. Appreciate the feedback. Plan to respond but need a couple of days to get through another task. |
At this point, there's not enough need/support for this (including myself) to continue forward. |
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
ScalacProvider
. The bootstrap toolchain is used to build thescalac
driver since it can't depend on itself and subsumesdeclare_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.WORKSPACE
: since default toolchain resolution now replaces hard-coding, the tests need the toolchains registered.ScalacProvider
to the bootstrap toolchain and rename itBootstrapInfo
to follow the pattern in bazel docs. Simplify the names.scalac_provider
andscalac_provider_attr
should be renamed or go away. In particular, I thinkphase_scalac_provider
becomesphase_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.skylib
load intwitter_scrooge
: the warning's been bugging me and it's fatal in newer versions ofskylib
.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.