-
Notifications
You must be signed in to change notification settings - Fork 81
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
Asterius toolchain declarations #1619
Conversation
🎉 All dependencies have been resolved ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
A couple comments.
Asterius is not able to implement a full haskell toolchain by itself and needs to rely on a regular one (in particular we need runghc for cabal support).
Just to be sure I understand correctly. This is not something that the Asterius toolchain needs to track itself explicitly. Instead this is taken care of by the changes introduced in #1510. Correct?
haskell/asterius/BUILD.bazel
Outdated
# Toolchain type for asterius specific tools such as ahc-dist, | ||
# which are not part of the regular haskell toolchain. | ||
toolchain_type( | ||
name = "asterius-toolchain", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name = "asterius-toolchain", | |
name = "toolchain_type", |
The documented convention is to use the name toolchain_type
for toolchain_type
targets. (I know the Haskell toolchain doesn't follow it either, but that's only for historical reasons)
Is there a good reason not to follow this convention with this new toolchain?
def default_exec_constraints(repository_ctx): | ||
(_, exec_constraints) = default_constraints(repository_ctx) | ||
return [Label(c) for c in exec_constraints] | ||
|
||
def labels_from_bundle_name(bundle_repo_name, asterius_version): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything that doesn't start with _
is publically exposed. These look more like local helpers, correct? If so, better to prefix _
and not expose them.
@@ -0,0 +1,323 @@ | |||
load("@bazel_skylib//lib:paths.bzl", "paths") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can follow the rule author guidelines here as well. This file should then be named repositories.bzl
.
sha256 = sha256, | ||
) | ||
|
||
asterius_bundle = repository_rule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this one meant to be public? Otherwise, also prefix with _
.
doc = "Defines the haskell toolchain using asterius, as well as the asterius toolchain which contains asterius specific tools.", | ||
) | ||
|
||
def ahc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps rules_haskell_asterius_toolchains
would be a better name for this. Following the pattern recommended here and also the naming here.
), | ||
] | ||
|
||
asterius_toolchain = rule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a regular rule, so probably better placed into haskell/asterius/defs.bzl
. Then the ghc.BUILD.tpl
also doesn't have to import asterius_workspace_rules.bzl
(or repositories.bzl
) which would be a strange import for a BUILD
file.
cabalopts = None, | ||
locale = None, | ||
nix = False): | ||
""" Define and registers asterius related toolchains. """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the user facing function, then all the attributes should be documented in the docstring here.
"nix": attr.bool( | ||
mandatory = True, | ||
doc = "Whether this toolchain depends on nix", | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand the following comment
For both nix and bindists, we download the same bundle made using the proverbial daml script.
the Asterius bindist is a self contained bundle. So, under which circumstances would the nix
attribute need to be True
?
haskell/nixpkgs.bzl
Outdated
repl_ghci_args, | ||
cabalopts, | ||
locale, | ||
nix = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. The asterius toolchain is attached to a Haskell toolchain (either bindist or nixpkgs) and the nix
parameter is used to select the correct one out of those two.
Yes, that's a valid approach.
If I understand correctly the Asterius toolchain does not itself directly depend on the regular GHC toolchain. It only depends on it indirectly through the runghc
attribute on the Cabal rules.
Would it be possible to define only one Asterius toolchain in the WORKSPACE
by directly calling a user exposed rules_haskell_asterius_toolchains
and leave it to Bazel's toolchain resolution to find the correct regular GHC toolchain for the Cabal rules? That would drop the need for the nix
parameter and would seem a bit more modular.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this would be better.
I was under the impression that this was not possible, because the asterius haskell toolchain would depend on another toolchain of the same type (but targeting another platform) so it looked a bit like a circular dependency.
But I just tried it and it seems to work 👍
60ac6d6
to
17ef182
Compare
Yes it seems so. I was not aware of this so I assumed the runghc from the toolchain was used. So at the moment tools from the regular haskell toolchain are used to complete the asterius toolchain. For later, I think that ReadthedocThe build fail looks quite similar to This issue. I will try to look into it. About the diff
|
5447533
to
17ef182
Compare
Ah, yes, that looks tricky. Good call to skip for now.
Good catch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Great work!
17ef182
to
b64079d
Compare
Depends on #1618
This PR contains the declarations of the asterius toolchains.
Declarations alongside regular toolchains.
Asterius is not able to implement a full haskell toolchain by itself and needs to rely on a regular one (in particular we need runghc for cabal support).
In order to declare asterius and regular haskell toolchains together there is a new optional
asterius_version
attribute tohaskell_register_ghc_nixpkgs
andhaskell_register_ghc_bindists
: if it is present, we try to also declare asterius versions of the variours haskell toolchains.At the moment only linux is supported.
The bundle.
For both nix and bindists, we download the same bundle made using the proverbial daml script.
This bundle contains asterius, Tweag's fork of wasi-sdk (the cc toolchain targetting WebAssembly) and the needed dynamic libraries.
It is temporariy uploded to this repository, the plan is to recover it later from the asterius repository once it can build in its ci.
Asterius toolchain type.
We declare a new toolchain type for asterius binaries that are not part of the regular haskell toolchain. It only contains the
ahc-dist
binary which generates.js
files from a haskell "binary" built with the asterius based haskell toolchain.