Asterius rules #1621

ylecornec · 2021-11-02T10:59:33Z

This PR contains the following asterius related rules as well as the tests.

The ahc_dist rule converts a haskell binary target (haskell_test, haskell_binary or haskell_cabal_binary) produced with the asterius haskell toolchain into an archive containing multiple javascript files.
There is a transition on its dep attribute so that this binary target is built using the asterius haskell toolchain.
The asterius_binary and asterius_test rules can then run these files using node (if the ahc_dist target is built with target="node").
The asterius_webpack rule can bundle all of these file into a unique js file (if the ahc_dist target is built with target="browser").

Tests

The tests/asterius/asterius_tests_utils.bzl file contains a macro used for the asterius tests which we use to call the above rules on existing binary targets from the tests folder.

We call this macro for almost all the binaries which do not depend on C code or use GHC specific toolchains libraries.

There are three exceptions which need more investigation:

tests/lhs needs to call the unlit binary, which should maybe be made explicit in the haskell toolchain first.
tests/haskell_cabal_package imports its main function from Lib, and it is not visible as such in the asterius generated javascript files. This example also fails when calling ahc-cabal directly.
tests/binary_with_main is a bit similar because it makes use of the main_function attribute.

Logs

I am unsure what to do about the logs of ahc-build (which always outputs some build info).
Such as :

[INFO] Writing JavaScript entry module to "/home/stan/.cache/bazel/_bazel_stan/3c7d084a53b480b3a5081e4bf80f09ce/sandbox/linux-sandbox/879/execroot/rules_haskell/bazel-out/k8-fastbuild/bin/tests/asterius/package-id-clash-binary/custom_output_name.mjs"

Should a flag be added to ahc-build in order to disable them?

aherrmann

Great work! Really exciting to see so many tests pass already.

I left a couple comments / questions.

aherrmann · 2021-11-04T11:27:41Z

haskell/asterius/defs.bzl

+]
+
+# Label of the template file to use for the webpack config.
+_TEMPLATE = "//haskell/asterius:asterius_webpack_config.js.tpl"


Suggested change

_TEMPLATE = "//haskell/asterius:asterius_webpack_config.js.tpl"

_TEMPLATE = "@rules_haskell//haskell/asterius:asterius_webpack_config.js.tpl"

I'd expect that this needs to be fully qualified for uses outside of rules_haskell itself.

aherrmann · 2021-11-04T11:30:37Z

haskell/asterius/defs.bzl

+    # ctx.file.dep was generated in the asterius platform configuration,
+    # and we want to generate js files in the current configuration.
+    # So we will copy it to the folder corresponding to the current platform.
+    file_copy_path = paths.join(output_tar_file.dirname, ctx.file.dep.basename)


Does this cause collisions if one sets --platforms @rules_haskell//haskell/asterius:asterius_platform globally? (Assuming that that's possible.) If so, this may need a check to only issue a copy action if the paths are actually different.

This should be fine now that we output files in a subfolder.
At the moment, it is not possible to set --platforms @rules_haskell//haskell/asterius:asterius_platform globally when compiling a ahc_dist target because it depends on the rules_nodejs toolchain that sets a target constraint (and maybe should not). But only the dependencies of this target need to be built for the asterius_platform.

aherrmann · 2021-11-04T11:34:29Z

haskell/asterius/BUILD.bazel

@@ -1,5 +1,11 @@
+load("@npm_rules_haskell//webpack-cli:index.bzl", webpack = "webpack_cli")


Does this mean that we impose a specific npm_install or yarn_install on the user? If so, perhaps this could use an indirection similar to what gazelle_cabal does to not impose that.

aherrmann · 2021-11-04T11:40:30Z

haskell/asterius/defs.bzl

+    options = " ".join(ctx.attr.options)
+    command = " && ".join([
+        "cp $2 $3",
+        "$1 --input-exe $3 {} --verbose --output-prefix {} {} {}".format(options, output_prefix, input_mjs, browser),


Suggested change

"$1 --input-exe $3 {} --verbose --output-prefix {} {} {}".format(options, output_prefix, input_mjs, browser),

"$1 --input-exe $3 {} --output-prefix {} {} {}".format(options, output_prefix, input_mjs, browser),

In other build actions we default to -v0. Is there a reason this should be --verbose or is it a debugging left over?

aherrmann · 2021-11-04T11:43:02Z

haskell/asterius/defs.bzl

+    command = " && ".join([
+        "cp $2 $3",
+        "$1 --input-exe $3 {} --verbose --output-prefix {} {} {}".format(options, output_prefix, input_mjs, browser),
+        "tar cf {output_tar} --exclude=./{output_tar_name} -C {workdir} . -h".format(


tar is not deterministic by default. In particular, timestamps and other file metadata as well as the order of files are not deterministic. See here for the necessary command-line flags to make it deterministic. See here for some more information.

Alternatively, perhaps you could use pkg_tar or the underlying tool it uses.

aherrmann · 2021-11-04T17:24:32Z

haskell/asterius/defs.bzl

+        "ahc_dist_dep": attr.label(
+            mandatory = True,
+            allow_single_file = True,
+            doc = """


Suggested change

doc = """

doc = """\

So that it's still the first line of the docstring? IIRC that has meaning in Startdoc.

aherrmann · 2021-11-04T17:25:19Z

haskell/asterius/defs.bzl

+    "testonly",
+]
+
+def asterius_common_impl(is_asterius_test, name, ahc_dist_dep, entry_point, subfolder_prefix = "asterius_untar", data = [], **kwargs):


Suggested change

def asterius_common_impl(is_asterius_test, name, ahc_dist_dep, entry_point, subfolder_prefix = "asterius_untar", data = [], **kwargs):

def _asterius_common_impl(is_asterius_test, name, ahc_dist_dep, entry_point, subfolder_prefix = "asterius_untar", data = [], **kwargs):

Looks like it should be private.

aherrmann · 2021-11-04T17:26:07Z

haskell/asterius/defs.bzl

+# Copied from https://github.com/tweag/asterius/blob/9f2574d9c2b50aa83d105741799e2f65b05e2023/asterius/test/ghc-testsuite.hs
+# The node options required to execute outputs from asterius.
+
+node_options = [


Suggested change

node_options = [

_node_options = [

Do these need to be public?

aherrmann · 2021-11-04T17:30:17Z

tests/haskell_cabal_library_sublibrary_name/package1.cabal

-        Cabal
-        , base
+        base


What's the reason for this change?

The Cabal library is not actually needed here and I thought that it was not compatible with asterius.
Because the asterius test failed with: Cabal is not a toolchain library.

But now that I looked into it deeper I think it was a mistake.
We try to use Cabal as a toolchain library because it is in this list, but this list seems wrong in the case of asterius. So maybe we could recover the Cabal package via some other way in this case ?

Oh, I see. Thanks for looking into this.
I think we can remove that list and instead use the location information provided by stack here. IIRC if there is no location field in the spec then it is a core package.
Though, I think this is orthogonal to this PR and perhaps better addressed in a separate PR.

tests/package-reexport/Final.hs

ylecornec · 2021-11-10T14:46:56Z

Thank you, I addressed the various comments.

The ahc_dist rule now creates its output in a subfolder, so the asterius_untar rule was removed.
If needed it is possible to use pkg_tar on the output but it is not asterius specific.
There is an optional subfolder_name attribute to be able to control the name of this subfolder.
There is now a default value for the name of the entry_point file.
There is now a transition to configure the ahc_dist targets (for the browser or not).
The build setting is declared in an external repository to work around the following issue (This way the same name for this build setting can be used from inside or outside the rules_haskell repository).
The webpack rule used by asterius is now declared in an external repository, which makes it possible for users to use a preexisting rules_nodejs setup to override it. Alongside asterius_dependencies_bindist and asteriuis_dependencies_nix, there is a now a third rule for this purpose:
asterius_dependencies_custom which uses the label of an custom webpack rule. For instance defined as such:
```
load("@npm//webpack-cli:index.bzl", webpack = "webpack_cli")
webpack(
    name = "webpack",
    visibility = ["//visibility:public"],
)
```
The asterius test with the Cabal library dependency is removed for now.
The asterius test with the java dependency as well.
There is now a genrule in the test macro, that tries to calls the asterius_binary target.

aherrmann

Thank you! Great work!

aherrmann · 2021-11-12T11:01:06Z

CI is failing after rebase. Perhaps due to the changes in #1623.

…/node)

ylecornec requested a review from aherrmann November 2, 2021 10:59

aherrmann reviewed Nov 4, 2021

View reviewed changes

ylecornec force-pushed the ylecornec/asterius_rules branch 5 times, most recently from 172a6e5 to 6c004d5 Compare November 10, 2021 14:28

aherrmann approved these changes Nov 12, 2021

View reviewed changes

aherrmann added the merge-queue merge on green CI label Nov 12, 2021

ylecornec force-pushed the ylecornec/asterius_rules branch from 6c004d5 to 0314425 Compare November 12, 2021 10:28

ylecornec mentioned this pull request Nov 17, 2021

Revert "remove asterius tests using haskell_modules" #1629

Open

ylecornec added 10 commits November 17, 2021 08:06

asterius rules

67dac19

Asterius tests

f2b418b

Only show errors for webpack call.

7e4d506

remove test depending on Cabal library

dced7ac

build setting and transition declaration for ahc_dist target (browser…

ac0253f

…/node)

add asterius_dependencies_custom

add0470

update rules

5445c38

update tests

cc9e2e9

remove asterius java_classpath file

628f50b

remove asterius tests using haskell_modules

01b6d5f

ylecornec force-pushed the ylecornec/asterius_rules branch from c079738 to 01b6d5f Compare November 17, 2021 08:06

mergify bot merged commit d7fec05 into master Nov 17, 2021

mergify bot deleted the ylecornec/asterius_rules branch November 17, 2021 09:13

mergify bot removed the merge-queue merge on green CI label Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asterius rules #1621

Asterius rules #1621

ylecornec commented Nov 2, 2021

aherrmann left a comment

aherrmann Nov 4, 2021

aherrmann Nov 4, 2021

ylecornec Nov 10, 2021

aherrmann Nov 4, 2021

aherrmann Nov 4, 2021

aherrmann Nov 4, 2021

aherrmann Nov 4, 2021

aherrmann Nov 4, 2021

aherrmann Nov 4, 2021

aherrmann Nov 4, 2021

ylecornec Nov 5, 2021

aherrmann Nov 5, 2021

ylecornec commented Nov 10, 2021

aherrmann left a comment

aherrmann commented Nov 12, 2021

	_TEMPLATE = "//haskell/asterius:asterius_webpack_config.js.tpl"
	_TEMPLATE = "@rules_haskell//haskell/asterius:asterius_webpack_config.js.tpl"

		@@ -1,5 +1,11 @@
		load("@npm_rules_haskell//webpack-cli:index.bzl", webpack = "webpack_cli")

	"$1 --input-exe $3 {} --verbose --output-prefix {} {} {}".format(options, output_prefix, input_mjs, browser),
	"$1 --input-exe $3 {} --output-prefix {} {} {}".format(options, output_prefix, input_mjs, browser),

	def asterius_common_impl(is_asterius_test, name, ahc_dist_dep, entry_point, subfolder_prefix = "asterius_untar", data = [], **kwargs):
	def _asterius_common_impl(is_asterius_test, name, ahc_dist_dep, entry_point, subfolder_prefix = "asterius_untar", data = [], **kwargs):

Asterius rules #1621

Asterius rules #1621

Conversation

ylecornec commented Nov 2, 2021

Tests

Logs

aherrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylecornec commented Nov 10, 2021

aherrmann left a comment

Choose a reason for hiding this comment

aherrmann commented Nov 12, 2021