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 support for GHC plugins #799

Merged
merged 2 commits into from
Apr 6, 2019
Merged

Add support for GHC plugins #799

merged 2 commits into from
Apr 6, 2019

Conversation

mboes
Copy link
Member

@mboes mboes commented Mar 31, 2019

This introduces new ghc_plugin rule type, and an extra attribute to
haskell_* rules to add plugins to the compilation process. Plugins
only affect the compile action, not any other action. We take care to
ensure that plugin dependencies do not induce extra runtime
dependencies, by using -plugin-package for plugin dependencies
instead of -package.

@mboes mboes force-pushed the plugins branch 5 times, most recently from cb217e8 to df8de05 Compare March 31, 2019 22:29
Copy link
Contributor

@guibou guibou left a comment

Choose a reason for hiding this comment

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

LGTM, please read comments.

@@ -236,7 +244,11 @@ def _haskell_binary_common_impl(ctx, is_test):

def haskell_library_impl(ctx):
hs = haskell_context(ctx)
dep_info = gather_dep_info(ctx)
dep_info = gather_dep_info(ctx, ctx.attr.deps)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a code duplication here.

@@ -66,7 +66,11 @@ def _should_inspect_coverage(ctx, hs, is_test):

def _haskell_binary_common_impl(ctx, is_test):
hs = haskell_context(ctx)
dep_info = gather_dep_info(ctx)
dep_info = gather_dep_info(ctx, ctx.attr.deps)
Copy link
Contributor

Choose a reason for hiding this comment

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

Code duplication here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@guibou What is duplicated here is one list comprehension in two places. See what our coding guidelines have to say about that (that part originally from the GHC coding style document).

haskell/private/packages.bzl Outdated Show resolved Hide resolved
@@ -17,7 +17,7 @@ load(":private/actions/package.bzl", "package")

_GHC_BINARIES = ["ghc", "ghc-pkg", "hsc2hs", "haddock", "ghci", "runghc", "hpc"]

def _run_ghc(hs, cc, inputs, outputs, mnemonic, arguments, params_file = None, env = None, progress_message = None):
def _run_ghc(hs, cc, inputs, outputs, mnemonic, arguments, params_file = None, env = None, progress_message = None, input_manifests = None):
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this?

@@ -96,6 +96,7 @@ def _run_ghc(hs, cc, inputs, outputs, mnemonic, arguments, params_file = None, e

hs.actions.run_shell(
inputs = inputs,
input_manifests = input_manifests,
Copy link
Contributor

Choose a reason for hiding this comment

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

And this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an argument to run_shell(), so it's documented in the API documentation for run_shell().

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind. I was just surprised to discover new arguments to run_shell and _run_ghc which were not used elsewhere in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you're right. They're used in a followup PR (not submitted yet). Though that extra argument does no harm at this point.

mboes added 2 commits April 5, 2019 22:51
It's an implicit dependency of the `ghc` library.
This introduces new `ghc_plugin` rule type, and an extra attribute to
`haskell_*` rules to add plugins to the compilation process. Plugins
only affect the compile action, not any other action. We take care to
ensure that plugin dependencies do not induce extra runtime
dependencies, by using `-plugin-package` for plugin dependencies
instead of `-package`.
@mboes mboes merged commit 8f09a3e into master Apr 6, 2019
@mboes mboes deleted the plugins branch April 6, 2019 05:52
@mboes mboes mentioned this pull request Aug 11, 2019
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.

2 participants