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

Define an implicit REPL target per binary/library target #225

Merged
merged 4 commits into from
May 2, 2018

Conversation

mrkkrp
Copy link
Member

@mrkkrp mrkkrp commented May 2, 2018

Close #220.

This PR removes the standalone haskell_repl rule and instead adds optional REPL outputs to every haskell_binary and haskell_library targets. The REPL outputs are not built unless we explicitly ask Bazel to build them and we can do so for any target. See the updated docs for more information how to use it, but I think it should be quite straightforward.

@mrkkrp mrkkrp requested review from judah and mboes May 2, 2018 08:11
@mboes mboes requested a review from gdeest May 2, 2018 09:38
@@ -138,6 +168,9 @@ def _mk_binary_rule(**kwargs):
doc = "File containing `Main` module.",
)
),
outputs = {
"repl": "%{name}-repl",
Copy link
Member

Choose a reason for hiding this comment

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

Bazel conventions seem to pretty consistently favour snake case. I think we should stick to Bazel conventions insofar as they exist for auto-generated target names.

Copy link
Member Author

@mrkkrp mrkkrp May 2, 2018

Choose a reason for hiding this comment

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

My logic here was:

  • Names of (at least) library targets cannot contain underscores because they should be valid GHC package names (maybe now that we escape underscores it's not so important).
  • So for consistency let's use dash here.

Snake case is understandable because foo-bar is not an identifier in Python/Skylark. But target names shouldn't necesserily be valid Skylark identifiers. In fact, at least in a project I was helping with recently I've seen a lot of targets named using dashes, such as xxx-doc, etc.

Anyway, the choice of the separator seems not so important to me.

@@ -22,10 +22,14 @@ jobs:
command: bazel test //... --config=ci
- run:
name: Test REPL for libraries
command: bazel-bin/tests/haskell_repl/haskell_lib_repl -e ":quit"
command: |
bazel build //tests/repl-targets:hs-lib-repl
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 have to be a two-line sequence, or as a user, can I bazel run --direct_run //tests/repl-targets:hs-lib-repl?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have Bazel 0.12 on my machine and CI doesn't have it either, so I think we should use what works with all versions. I don't know what --direct-run sets as current working directory and that's important for this script.

Copy link
Member

Choose a reason for hiding this comment

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

Comment was not a statement, but a question. That said, Bazel had very poor support for this use case until v0.12. It's much better now, so we do need to make sure bazel run --direct_run is possible for users that have the right version, if it isn't already the case.

There's a nixpkgs PR for 0.12. I've been using it locally on my laptop.

Every `haskell_binary` target also defines an optional REPL target that is
not built by default, but can be built on request. The name of the REPL
target is the same as the name of binary with `"-repl"` added at the end.
For example, the target above also defines `main-repl`.
Copy link
Member

Choose a reason for hiding this comment

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

How to load a library or binary into a REPL should be documented (use bazel run --direct_run?). Note that in future versions of Bazel, --direct_run might become the default. See bazelbuild/bazel@1fafc6f.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added something about this.

Copy link
Contributor

@gdeest gdeest left a comment

Choose a reason for hiding this comment

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

Works for my use-case.

Copy link
Collaborator

@judah judah left a comment

Choose a reason for hiding this comment

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

LGTM. I haven't tested the --direct-run myself but it looks useful; agree we should make sure the old invocation way still works.

@mboes mboes merged commit d1ff917 into master May 2, 2018
@mboes mboes deleted the auto-repl-per-target branch May 2, 2018 20:54
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.

4 participants