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

Add ghc version to CI matrix for bindists #1923

Merged
merged 27 commits into from
Sep 1, 2023
Merged

Add ghc version to CI matrix for bindists #1923

merged 27 commits into from
Sep 1, 2023

Conversation

avdv
Copy link
Member

@avdv avdv commented Jul 31, 2023

This PR adds a ghc dimension to the CI build matrix for the "Build & Test - bindist" job and runs jobs for GHC 9.2.5 as well as GHC 9.4.5.

Also, we have to bump the minimal supported Bazel version to 5.3.0 in this PR, since we use an API that was introduced in that version and gating it with a version check was tripping up the Stardoc documentation generation (e.g. see here).

Fixes #1933 and fixes #1900

@avdv avdv force-pushed the ci-matrix-ghc branch 5 times, most recently from 4ad86a7 to ccd7a43 Compare August 1, 2023 13:31
@avdv avdv force-pushed the ci-matrix-ghc branch 3 times, most recently from 83e19de to 87e7f3e Compare August 8, 2023 17:25
@avdv avdv force-pushed the ci-matrix-ghc branch 7 times, most recently from 6288a69 to 734ffd4 Compare August 17, 2023 07:21
@avdv avdv force-pushed the ci-matrix-ghc branch 7 times, most recently from 907d710 to eb58ed1 Compare August 23, 2023 15:01
@avdv avdv requested a review from aherrmann August 24, 2023 07:26
@avdv avdv marked this pull request as ready for review August 24, 2023 07:26
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you for figuring this out, from what I saw that was not easy!
I left a few small comments, and questions for clarification. My main concern is any potential impact on downstream users, or rather to what extent the impact is confined to our CI.

print("Using GHC version {} from env variable `GHC_VERSION`".format(ghc_version))

repository_ctx.file("BUILD")
repository_ctx.file("ghc_version.bzl", content = "GHC_VERSION = {}".format(repr(ghc_version)))
Copy link
Member

Choose a reason for hiding this comment

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

what happens if $GHC_VERSION is unset?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case the file will contain GHC_VERSION = None. This will cause the default GHC version to be used.

@@ -0,0 +1,15 @@
""" Module extension which configures the GHC version"""
Copy link
Member

Choose a reason for hiding this comment

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

IIUC it configures the default version. But, if a MODULE.bazel file provides a specific version to the bindist module extension, then it will override this env-var, correct? If so, the docs should be clearer on this.

Copy link
Member Author

@avdv avdv Sep 1, 2023

Choose a reason for hiding this comment

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

Correct, this will only effect the default GHC version used, and only for rules_haskell and rules_haskell_tests. Other modules are not effected at all. They will always just get the default GHC version if using version = None and the specified version otherwise.

I have added clearer docs to this file.

extensions/ghc_version.bzl Outdated Show resolved Hide resolved
.bazelrc.common Show resolved Hide resolved
haskell/private/ghc_ci.bzl Show resolved Hide resolved
build_file_content = """
if GHC_VERSION and GHC_VERSION.startswith("9.4."):
http_archive(
name = "Cabal",
Copy link
Member

Choose a reason for hiding this comment

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

Where is @Cabal all used? From what I can tell it's mostly used for test dependencies and for the worker. So, it shouldn't directly affect users of rules_haskell, correct?

Side note, I noticed that some test-dependencies in rules_haskell's MODULE.bazel, e.g. some stack_snapshots, are not marked as dev-dependencies. It's out of scope for this PR, but something we should fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where is @Cabal all used? From what I can tell it's mostly used for test dependencies and for the worker. So, it shouldn't directly affect users of rules_haskell, correct?

Yes.

Side note, I noticed that some test-dependencies in rules_haskell's MODULE.bazel, e.g. some stack_snapshots, are not marked as dev-dependencies. It's out of scope for this PR, but something we should fix.

Agreed, and I already tried that (setting dev_dependency = True) but that failed with some wild error later, since rules_haskell_tests depended on it.

http_archive(
name = "Cabal",
build_file_content = """
if GHC_VERSION and GHC_VERSION.startswith("9.4."):
Copy link
Member

Choose a reason for hiding this comment

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

So long as it's only for our CI, I'm fine with this approach. Generally speaking, a better route would be if we could figure out how to select based on resolved GHC version and then use select to switch to the correct versions. But, that seems quite difficult in this case.

@avdv avdv added the merge-queue merge on green CI label Sep 1, 2023
@avdv avdv mentioned this pull request Sep 1, 2023
@avdv
Copy link
Member Author

avdv commented Sep 1, 2023

@Mergifyio rebase

This makes the default GHC version publicly accessible and introduces a new
repository which uses the default GHC version if not already defined.
This wrapper adds the `bin` folder of the cc toolchain from GHC's bindist to the
`PATH` so that the protoc.exe is able to find its runtime DLLs, e.g. libc++.dll.

Note, using a `sh_binary` rule with a cmd script does not create an extra
executable contrary to using a Bash script.
These are not needed, since `executable = True` already demands a single executable file
We need to be able to use `--proto_compiler` to specify a custom protoc executable / script.

The needed "proto_compiler" configuration field has been introduced in Bazel 5.3.0.
It is always the first one.
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2023

rebase

✅ Branch has been successfully rebased

@avdv
Copy link
Member Author

avdv commented Sep 1, 2023

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2023

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 37e48d1 into master Sep 1, 2023
104 checks passed
@mergify mergify bot deleted the ci-matrix-ghc branch September 1, 2023 19:00
@mergify mergify bot removed the merge-queue merge on green CI label Sep 1, 2023
avdv pushed a commit that referenced this pull request Sep 4, 2023
Add ghc version to CI matrix for bindists
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.

Add full support for GHC 9.4 bindist Do not reverse order of plugins for GHC >= 9.4.1
2 participants