Skip to content
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

Building the most straightforward tests with GHC Bindist compiled with Hadrian #1842

Draft
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

GuillaumeGen
Copy link
Contributor

@GuillaumeGen GuillaumeGen commented Dec 6, 2022

This is using a locally built bindist using Hadrian.
This is currently passing 153 of the 206 tests when running bazel test //... --keep_going.

The main identified issues for failing tests are:

  • The issue with Cabal dependency calling undeclared splitFileName and minusFileName as described in splitFileName and minusFileName Variable not in scope causes builds to fail #1832
  • The Hadrian bindist does not distribute a ghci binary. As far as I understood, this should be moved to ghc --interactive to pass tests related to REPL.
  • Some header files not found like "ghcplatform.h".
  • Some type interfaces changed between GHC 8.10.7 and 9.2.5

@GuillaumeGen GuillaumeGen changed the title Building the most strightforward tests with GHC Bindist compiled with Hadrian Building the most straightforward tests with GHC Bindist compiled with Hadrian Dec 6, 2022
@GuillaumeGen
Copy link
Contributor Author

Might be worth putting the generated bindist somewhere on the internet, to avoid reference to my local file system.

WORKSPACE Outdated
url = "file:///home/guillaume/ExternalPrograms/ghc/_build/bindist/ghc-9.2.5-x86_64-unknown-linux.tar.xz",
sha256 = "e27724de38998dd6c3fb46ac5df4cf6818d779c1b749ea1afcd0b64e55b8217e",
strip_prefix = "ghc-9.2.5-x86_64-unknown-linux",
version = "9.2.5",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the issues are probably unrelated to the hadrian toolchain and instead just due to the version bump. It might make sense to tackle those in a separate PR and bump the GHC version under test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are completely right. Separating issues is always beneficial and it will help both review and understanding the cause of the failing tests.

if local_python_repo_name not in native.existing_rules():
_configure_python3_toolchain(name = local_python_repo_name)

def _configure_python3_toolchain_impl(repository_ctx):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for development, but we'll want to clean this up and deduplicate with haskell/ghc_bindist.bzl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

haskell/repl.bzl Outdated
"%{TOOL}": hs.tools.ghci.path,
"%{ARGS}": "(" + " ".join(
"%{TOOL}": hs.tools.ghc.path,
"%{ARGS}": "--interactive (" + " ".join(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work for other GHC versions as well? If so, great! If not, we'll have to gate this to not break the other GHC versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. To investigate.

Comment on lines 95 to 117
generated_bin_filegroup = define_rule(
"copy_filegroups_to_this_package",
name = "generated_bin_filegroup",
srcs = [":bin"],
)

generated_lib_filegroup = define_rule(
"copy_filegroups_to_this_package",
name = "generated_lib_filegroup",
srcs = [":lib"],
)

generated_docdir_filegroup = define_rule(
"copy_filegroups_to_this_package",
name = "generated_docdir_filegroup",
srcs = [":{}".format(docdir)],
)

generated_include_filegroup = define_rule(
"copy_filegroups_to_this_package",
name = "generated_include_filegroup",
srcs = [":{}".format(docdir)],
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can move these into haskell/toolchain.bzl next to _hadrian_bindist_settings as

copy_filegroups_to_this_package(
    name = "generated_bin_filegroup",
    srcs = [":bin"],
)
...

and the definition of copy_filegroups_to_this_package as well.

Reasons why that may be preferable:

  1. The ghc.BUILD.tpl is also used by the other bindist toolchains and needs to remain valid for those use cases until we can supersede them with the Hadrian one. But, the %{generated_*_filegroup} placeholders are only used for the Hadrian case. Invoking the copy-files rules within haskell_toolchain macro means that these placeholders are no longer needed.
  2. It's preferable to not mix repository rule and regular rule definitions in the same file, so that changes to the rule definition don't also invalidate the repository rule and cause unnecessary refetching. If the copy-file rule is moved into haskell_toolchain then the repository rules and regular rules are separated and the import of ghc_bindist_hadrian.bzl in ghc.BUILD.tpl is also no longer needed.
  3. It puts both concerns into the same place: generating the settings file and placing the bindist files next to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convinced. Will do it.

@avdv avdv force-pushed the gg/hadrian-bindist branch 2 times, most recently from 3a71f45 to fd50666 Compare May 15, 2023 08:40
@avdv avdv self-assigned this May 15, 2023
@avdv
Copy link
Member

avdv commented May 15, 2023

Made some progress:

  1. use the official GHC 9.6.1 bindist as this is built with Hadrian
  2. fixed a few build problems, so now the ./tests/run-start-script.sh --use-bindists works on CI

@avdv avdv force-pushed the gg/hadrian-bindist branch 3 times, most recently from 78ddc14 to 7d4f967 Compare May 19, 2023 14:05
@avdv avdv force-pushed the gg/hadrian-bindist branch 3 times, most recently from 49346e7 to 0f69a7f Compare November 3, 2023 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants