Don't hardcode platforms in toolchain anymore #597
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Our toolchain implementation was incomplete. We used to assume x86_64 as the architecture, and never set any platform constraints. We now make it possible to do so in new attributes to `haskell_toolchain`. This has important implications. It means that we no longer should define a single toolchain that states different constraints depending on the host platform. We now instead have multiple toolchains in scope, one for each operating system. Which properly declared constraints, the toolchain resolution mechanism can select the right one. There is another change in this commit, which we couldn't keep separate because it was co-dependent to the above change if we want to stay green. `ghc_bindist` now itself defines a toolchain (no need to define one separately), and also itself registers it. So `ghc_bindist` is now easier to use. A side effect of this is that the toolchain definition for Windows moves into an external dependency, where it won't bother anyone. If we define the Windows toolchain in `//tests`, then we'll be fetching and building it even on Linux whenever someone says `bazel build //tests/...`. We'll eventually also make Nixpkgs toolchains equally easy to define.
mboes
force-pushed
the
per-platform-toolchains
branch
from
75ab640 to
9255a61
Compare
guibou
approved these changes
Jan 19, 2019
WORKSPACE
Outdated
| name = "threaded-rts", | ||
| srcs = glob( | ||
| ["lib/ghc-*/rts/libHSrts_thr-ghc*." + ext for ext in [ | ||
| "so", |
There was a problem hiding this comment.
This is a copy/paste of the existing ghc.BUILD. Any improvement here would be an orthogonal change.
| version = ghc_version, | ||
| ) | ||
|
|
||
| register_toolchains( | ||
| "//tests:ghc", | ||
| "//tests:ghc_linux", | ||
| "//tests:ghc_osx", |
There was a problem hiding this comment.
No. See the commit messages as for why. Windows uses ghc_bindist and that rule self registers.
| version = ghc_version, | ||
| ) | ||
| for os, tools, locale_archive in [ | ||
| ("linux", "@hackage-ghc//:bin", "@glibc_locales//:locale-archive"), |
There was a problem hiding this comment.
Nope. See commit messages as for why.
| srcs = glob([ | ||
| "bin/*", | ||
| ]), | ||
| srcs = glob(["bin/*"]), |
There was a problem hiding this comment.
Cosmetic only, or I missed something?
WORKSPACE
Outdated
| @@ -28,7 +28,34 @@ load( | |||
| haskell_nixpkgs_package( | |||
| name = "ghc", | |||
| attribute_path = "haskellPackages.ghc", | |||
| build_file = "//haskell:ghc.BUILD", | |||
There was a problem hiding this comment.
Does this mean that //haskell:ghc.BUILD is now only used for ghc_bindist? Shouldn't the name change?
There was a problem hiding this comment.
With the latest commit in the series, we no longer need to duplicate much of the content of this file. So we're back to using //haskell:ghc.BUILD here as before.
mboes
force-pushed
the
per-platform-toolchains
branch
2 times, most recently
from
274e5ae to
a3160fb
Compare
We want the toolchain definition to be tucked away in a separate repository, that way `bazel build //...` will not match it (and e.g. build the Windows toolchain even on Linux). At the same time, we don't want the definition in the bindist repository, because then we need to download the bindist first before we can see the toolchain definition. The solution is to add the toolchain definition in its own special repository.
mboes
force-pushed
the
per-platform-toolchains
branch
from
fcf2195 to
8a1908e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our toolchain implementation was incomplete. We used to assume x86_64 as
the architecture, and never set any platform constraints. We now make it
possible to do so in new attributes to
haskell_toolchain. This hasimportant implications. It means that we no longer should define a single
toolchain that states different constraints depending on the host platform.
We now instead have multiple toolchains in scope, one for each operating
system. Which properly declared constraints, the toolchain resolution
mechanism can select the right one.
As a side effect,
ghc_bindistis now easier to use. Users don't needto write a separate toolchain declaration and toolchain registration
in addition to the declaring the bindist anymore. The next step after
this PR will be to introduce convenience functions to add external
dependencies for all known bindists, and all known GHC's in Nixpkgs.
That sounds overkill, but the toolchain resolution mechanism makes
this fine. And it's how we'll finally make it seamless for some
developers to use bindists while others on the same project can use
Nixpkgs without having to make edits to the
WORKSPACE.