-
Notifications
You must be signed in to change notification settings - Fork 205
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
Update rules_haskell #1133
Update rules_haskell #1133
Conversation
f9f9bfa
to
23b5a21
Compare
3eb7b07
to
04fbb9d
Compare
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.
LGTM, thank you!
deduped = list.dedup_on(get_lib_name, ext_libs) | ||
|
||
for lib in deduped: | ||
- # This test is a hack. When a CC library has a Haskell library |
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.
Not sure if the fact that we now have patches to remove hacks in rules_haskell is a good or a bad thing 😄
@@ -1,5 +1,5 @@ | |||
diff --git a/haskell/private/actions/compile.bzl b/haskell/private/actions/compile.bzl |
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.
We should try to upstream that patch. Just haven’t had the time so far sadly 😞
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 tried to just apply the patch on rules_haskell. Sadly it breaks plugins, haddock, and doctests.
nix/ghc.nix
Outdated
@@ -24,7 +24,6 @@ let | |||
|
|||
# We need newer versions that build with GHC 8.6 | |||
language-c = super.callPackage ./overrides/language-c-0.8.2.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.
We can probably also get rid of the language-c
override if we don’t need c2hs
.
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 believe we need it for grpc related stuff.
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.
The Nix override seems to be unused. I could only find references to the Hazel package.
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 left-over from when the package was patched.
- rules_haskell now handles the global package db within Bazel tweag/rules_haskell#859 - We no longer use the Nix provided c2hs. So, we drop it. - Rename `ghcWithC2hs` to `ghcStatic` to clarify that that's where the static linking patches are applied. - Extend package-db patches to align Nix store paths with the new $out. This works around a restriction in current rules_haskell, where the paths in the package config files must have the same prefix as the path to the package config files themselves. - Don't exclude haskell libraries from extra-libraries entries.
04fbb9d
to
76ec3db
Compare
ghcWithC2hs
toghcStatic
to clarify that that's where the static linking patches are applied.$out
. This works around a restriction in current rules_haskell, where the paths in the package config files must have the same prefix as the path to the package config files themselves. (This is also related to why we need to pull in libffi headers separately on Unix)Pull Request Checklist
NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines run
totrigger the build.