-
Notifications
You must be signed in to change notification settings - Fork 81
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
Multi target haskell_repl #736
Conversation
Now accepts transitive_cc_dependencies instead of build_info, so that it can be used for the multi target REPL as well.
This variable was unused.
224af51
to
648fa0d
Compare
Before the GHCi wrapper located the ghci script relative to the working directory, which was expected to be the workspace directory. This worked fine for executing the `@repl` files using `bazel run` as these were executed from the repository root and not from a dedicated exec root. In case of a dedicated repl target this will no longer work, as the target will be executed in its exec root. This change modifies the wrapper to first change into the workspace directory, and to refer to the ghci script relative to the exec root. The same is done for library dependencies.
This allows users of the ghci wrapper to pass in additional environment variables easily.
648fa0d
to
842612d
Compare
Above is a problem I ran into when using this branch. |
@SebastianKG Thanks for testing this. That commit is out-of-date with this PR. Could you retry on the current state of this PR (branch
It's surprising that the |
Loads all targets given in the deps attribute as well as all dependencies in the local workspace as sources into GHCi.
842612d
to
6ab0875
Compare
Noticed an issue with the generated |
Names such as `tmpDir` or `h` are likely to shadown names from loaded modules, which will cause errors if `-Werror=name-shadowing` is active.
Awesome, I gave it another look, and your latest code seems to load the correct modules. Great work! |
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.
Impressive piece of code!
I am unsure about the include
/exclude
parsing though. It feels like we are leaving bazel here to do our own stuff, which is going to bite us because we can never implement exactly the same target logic that bazel implements.
If I remember correctly, there is a way to call bazel query
from within bazel rules, via some builtin primitive. If its possible, we should use that at least. If not, we should open an upstream feature request describing our use-case and mark the attributes with an experimental-
prefix for now (until we know whether there is going to be an upstream feature).
Regarding old-style @repl
syntax, is this new way a superset of that? Then we could deprecate it.
There’s also haskell/private/actions/repl.bzl
. Is that file used after this change or can we remove it?
Addressing review comments https://github.com/tweag/rules_haskell/pull/736/files/cb50935fee46d40d7443bc3ace5fcfa9780d20ea..a15d14b4efc352b043a99302cc5326be22fcb2a9#r266604868 https://github.com/tweag/rules_haskell/pull/736/files/cb50935fee46d40d7443bc3ace5fcfa9780d20ea..a15d14b4efc352b043a99302cc5326be22fcb2a9#r266605247 https://github.com/tweag/rules_haskell/pull/736/files/cb50935fee46d40d7443bc3ace5fcfa9780d20ea..a15d14b4efc352b043a99302cc5326be22fcb2a9#r266605433
The RUNFILES_DIR environment variable is not meaningful on Windows, where Bazel instead creates a RUNFILES_MANIFEST_FILE which lists all runfile dependencies. This changes the REPL wrapper to automatically construct RUNFILES_DIR and RUNFILES_MANIFEST_FILE in a manner compatible with Bazel's Bash runfiles 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.
@Profpatsch Thanks for the great comments. I've addressed them all PTAL.
More for demonstrations purposes than anything else because it's superseded by #736
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.
Awesome! 👍
(I'd like some of this to be merged with #757 at some point, but that's future work).
@@ -1,5 +1,6 @@ | |||
"""GHCi REPL support""" | |||
|
|||
load(":private/context.bzl", "render_env") |
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.
Wouldn't it be possible to factor all this file out with the new haskell_repl
rule?
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 quite at the moment. haskell_repl
is its own rule and can return a DefaultInto
and thereby properly use runfiles. haskell/private/actions/repl.bzl
creates an extra output in the regular haskell_*
rules and cannot return a dedicated DefaultInfo
with runfiles. Instead it has to use the ln
trick to force it's dependencies and refer to them in bazel-out
.
I agree that there is duplication that we don't want to keep around in the long term. One possible way forward could be to drop haskell/private/actions/repl.bzl
completely and replace it by the haskell/repl.bzl
aspect, similar to haskell/haddock.bzl
. But, I'd prefer to do that in a separate PR.
Note, in some instances this adds static libraries to the link commands that were previously filtered out. A corresponding comment stated that GHCi cannot load static libraries. However, this is not true. GHCi can load static PIC libraries.
9ae5371
to
af40bb4
Compare
Great, great work |
More for demonstrations purposes than anything else because it's superseded by #736
Love it! |
Closes #380
haskell_repl
which constructs a ghci wrapper that loads multiple targets by source.Example usage
-package-id
,-package-db
flags.-package
.compiler_flags
andrepl_ghci_flags
.RUNFILES_DIR
environment variable, so that code that requires runfiles can be tested in the REPL.ghci_repl_wrapper.sh
so it can be used for the multi target repl.