haskell_toolchains extension #1914

ylecornec · 2023-06-27T17:24:44Z

Depends on #1912

This PR adds the haskell_toolchains module extension which is used to configure and register the haskell bindists, via the bindist and bindists tags.

The bindists tag corresponds to haskell_register_ghc_bindists and registers toolchains for all platforms.
The bindist tag corresponds to ghc_bindist and is used to register toolchains one at a time. Inside the same module, the toolchains declared with bindist take precedence over the bindists ones.

The rules_haskell module calls the bindists tag itself to register default toolchains that can be used without any configuration. Alternatively, we can register customized toolchains, as done by the rules_haskell_test module (the priority of toolchains respects bazel's iteration order over modules where the root module comes first).

We cannot use load in the `MODULE.bazel` files at the moment

The configuration variables such as test_repl_ghci_args are duplicated inside the MODULE.bazel file because it is not possible to load them at the moment (see bazelbuild/bazel#17880). For the same reason it is not possible to rely on is_windows to configure the cabalops variable so the windows toolchains are declared separately in this file.

Documentation

The documentation is not generated because stardoc does not support it yet (bazelbuild/stardoc#123).

Toolchain declaration

It is not possible to register aliases to toolchain with bzlmod yet (bazelbuild/bazel#16298), so ghc_bindist.bzl was refactored a bit to make ghc_bindist_toolchain_declaration public. This way the haskell_toolchains module extension can call it to build its own toolchain declarations.

aherrmann

Good work, that looks great!

Only a couple small comments.

Btw, I really appreciate how easy this PR and #1912 are to review commit by commit. Thank you!

aherrmann · 2023-06-29T08:31:50Z

rules_haskell_tests/MODULE.bazel

+test_ghc_version = "9.2.5"
+
+test_ghcopts = [
+    "-XStandaloneDeriving",  # Flag used at compile time
+    "-threaded",  # Flag used at link time
+    # Used by `tests/repl-flags`
+    "-DTESTS_TOOLCHAIN_COMPILER_FLAGS",
+    # this is the default, so it does not harm other tests
+    "-XNoOverloadedStrings",
+]
+
+test_haddock_flags = ["-U"]
+
+test_repl_ghci_args = [
+    # The repl test will need this flag, but set by the local
+    # `repl_ghci_args`.
+    "-UTESTS_TOOLCHAIN_REPL_FLAGS",
+    # The repl test will need OverloadedString
+    "-XOverloadedStrings",
+]
+
+test_cabalopts = [
+    # Used by `tests/cabal-toolchain-flags`
+    "--ghc-option=-DTESTS_TOOLCHAIN_CABALOPTS",
+    "--haddock-option=--optghc=-DTESTS_TOOLCHAIN_CABALOPTS",
+]
+
+cabalopts_windows = test_cabalopts + [
+    # To avoid ghcide linking errors with heapsize on Windows of the form
+    #
+    #   unknown symbol `heap_view_closurePtrs'
+    #
+    # See https://github.com/haskell/ghcide/pull/954
+    "--disable-library-for-ghci",
+]


The configuration variables such as test_repl_ghci_args are duplicated inside the MODULE.bazel file because it is not possible to load them at the moment (see bazelbuild/bazel#17880). For the same reason it is not possible to rely on is_windows to configure the cabalops variable so the windows toolchains are declared separately in this file.

Perhaps another approach could be for the module extension/tags to accept a label to a JSON file holding this configuration? (Either way, out of scope for this PR)

MODULE.bazel

aherrmann · 2023-06-29T08:41:02Z

extensions/haskell_toolchains.bzl

+        ),
+    },
+    doc = """ Used to generate the `all_bindist_toolchains` external repository.
+    We can then invoque `register_toolchains("@all_bindist_toolchains//:all")` in the MODULE.bazel file.


typo invoque --> invoke.

aherrmann · 2023-06-29T08:44:38Z

extensions/haskell_toolchains.bzl

+    attrs = {
+        "name": attr.string(
+            mandatory = True,
+            doc = "A unique name for the repository",


I think this needs a bit more explanation. As I understand it, it's only a unique name for the tag within the calling module. I.e. it doesn't have to be a globally unique name, correct?

aherrmann · 2023-06-29T08:48:05Z

extensions/haskell_toolchains.bzl

+    found_bindists = False
+    for module in mctx.modules:
+        for bindist_tag in module.tags.bindist:
+            name = "bindist_{}_{}_{}".format(module.name, module.version, bindist_tag.name)


Is bindist_tag.name actually needed, or could it be replaced by an autogenerated unique suffix (e.g. a counter, or perhaps a hash plus collision counter to reduce invalidation)? Asking in the spirit of this discussion. Since the name is scoped to within the calling MODULE and not global it doesn't seem too onerous, but, maybe it can be avoided altogether.

Ah indeed I think we can do without the name attribute (and without auto-generated suffixes).

I updated the code to rely on the target attribute instead.

Before, if two bindist tags shared the same target attribute, the toolchains would have the same constraints so the second one would be effectively ignored. So now, a module is not allowed to use two bindist tags with the same target, and (module.name, module.version, bindist_tag.target) can be used as an identifier.

aherrmann · 2023-06-29T08:54:27Z

extensions/haskell_toolchains.bzl

+        for bindists_tag in module.tags.bindists:
+            # We only consider the first `bindists` tag, because subsequent
+            # ones would have the same constraints and lower priority.
+            if not found_bindists:


Could this be simplified to something along the lines of the following?

if module.tags.bindists: bindists_tag = module.tags.bindists[0] ...

I.e. drop the found_bindists boolean.

dpulls · 2023-06-29T09:51:55Z

🎉 All dependencies have been resolved !

These are not needed anymore in bzlmod because of the haskell_toolchains module extension.

This helps the haskell_toolchains module extensions to declare all of its toolchains in one BUILD file.

This extension is used to register the haskell bindists with bzlmod.

ylecornec force-pushed the ylecornec/bindist_extension branch from 9cf5ed4 to 26926db Compare June 28, 2023 07:34

ylecornec marked this pull request as ready for review June 28, 2023 08:10

ylecornec requested a review from avdv as a code owner June 28, 2023 08:10

ylecornec mentioned this pull request Jun 14, 2023

Add a module extension for bindist toolchains #1897

Closed

3 tasks

ylecornec force-pushed the ylecornec/activate_bzlmod branch 5 times, most recently from 8d445fa to 8877ccc Compare June 28, 2023 20:26

ylecornec force-pushed the ylecornec/bindist_extension branch from 26926db to ff84233 Compare June 28, 2023 20:41

aherrmann approved these changes Jun 29, 2023

View reviewed changes

Base automatically changed from ylecornec/activate_bzlmod to master June 29, 2023 09:51

ylecornec added 2 commits June 29, 2023 11:52

Move calls to haskell_register_ghc_bindists to WORKSPACE files

9bc6fce

These are not needed anymore in bzlmod because of the haskell_toolchains module extension.

Refactor ghc_bindist.bzl for use with haskell_toolchains extension

47a843f

This helps the haskell_toolchains module extensions to declare all of its toolchains in one BUILD file.

ylecornec force-pushed the ylecornec/bindist_extension branch from ff84233 to b0659bf Compare June 29, 2023 12:22

ylecornec added 2 commits June 29, 2023 15:23

Add haskell_toolchains module extension

ea16eab

This extension is used to register the haskell bindists with bzlmod.

On non nix platforms with bzlmod: use posix toolchain from rules_sh

eefde79

ylecornec force-pushed the ylecornec/bindist_extension branch from b0659bf to eefde79 Compare June 29, 2023 13:23

ylecornec added the merge-queue merge on green CI label Jun 29, 2023

mergify bot merged commit 7600541 into master Jun 29, 2023
31 checks passed

mergify bot deleted the ylecornec/bindist_extension branch June 29, 2023 20:28

mergify bot removed the merge-queue merge on green CI label Jun 29, 2023

avdv mentioned this pull request Oct 16, 2023

Remove duplicate configuration in MODULE.bazel #1976

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

haskell_toolchains extension #1914

haskell_toolchains extension #1914

ylecornec commented Jun 27, 2023 •

edited

aherrmann left a comment

aherrmann Jun 29, 2023

aherrmann Jun 29, 2023

aherrmann Jun 29, 2023

aherrmann Jun 29, 2023

ylecornec Jun 29, 2023

aherrmann Jun 29, 2023

dpulls bot commented Jun 29, 2023

haskell_toolchains extension #1914

haskell_toolchains extension #1914

Conversation

ylecornec commented Jun 27, 2023 • edited

We cannot use load in the MODULE.bazel files at the moment

Documentation

Toolchain declaration

aherrmann left a comment

Choose a reason for hiding this comment

aherrmann Jun 29, 2023

Choose a reason for hiding this comment

aherrmann Jun 29, 2023

Choose a reason for hiding this comment

aherrmann Jun 29, 2023

Choose a reason for hiding this comment

aherrmann Jun 29, 2023

Choose a reason for hiding this comment

ylecornec Jun 29, 2023

Choose a reason for hiding this comment

aherrmann Jun 29, 2023

Choose a reason for hiding this comment

dpulls bot commented Jun 29, 2023

ylecornec commented Jun 27, 2023 •

edited

We cannot use load in the `MODULE.bazel` files at the moment