Implement haskell_repl support for targets that have modules in them #1733

googleson78 · 2022-04-28T11:07:59Z

googleson78 · 2022-04-28T11:08:07Z

TODO: tests

EDIT: I added one..?

googleson78 · 2022-04-28T11:11:26Z

haskell/experimental/private/module.bzl

@@ -548,6 +569,7 @@ def build_haskell_modules(
            object files and the object files of their transitive module
            dependencies. See Note [Narrowed Dependencies].
        per_module_transitive_dyn_objects: like per_module_transitive_objects but for dyn_o files
+        repl_info: struct(source_files, boot_files, import_dirs, user_compile_flags)


Is this enough documentation? If not, how should I document the fields of the returned struct?

I would add some, to match the style of the doc for the other arguments. It looks you can indent it as it is done here.

Added some docs here.

googleson78 · 2022-04-28T11:11:52Z

haskell/experimental/defs.bzl

@@ -7,6 +7,7 @@ load(
 )
 load("//haskell:private/cc_libraries.bzl", "haskell_cc_libraries_aspect")

+# TODO[GL] should we have repl_ghci_args here?


Not sure about this one.

Probably not for now. Seems like something we can try to add later if a user wants it.

tests/haskell_module/repl/haskell_module_repl_test.go

k1nkreet

Overall looks good to me

tests/haskell_module/repl/haskell_module_repl_test.go

googleson78 · 2022-05-11T07:07:53Z

Now that #1731 is merged, I should take care of the import_dirs situation here (after rebasing).

pranaysashank · 2022-05-16T19:07:08Z

I just tested this PR on a local workspace with multiple packages, the repl target doesn't seem to build / include its dependencies.
Here is the workspace to test with https://github.com/pranaysashank/nd/tree/test-modules-repl

$ bazel run //package-b:package-b@repl
GHCi, version 8.10.7: https://www.haskell.org/ghc/  :? for help
[1 of 2] Compiling PackageB.Mod2    ( package-b/src/PackageB/Mod2.hs, interpreted )

package-b/src/PackageB/Mod2.hs:4:1: error:
    Could not find module ‘PackageA.Mod2’
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
  |
4 | import qualified PackageA.Mod2
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Failed, no modules loaded.
[1 of 2] Compiling PackageB.Mod2    ( package-b/src/PackageB/Mod2.hs, interpreted )

package-b/src/PackageB/Mod2.hs:4:1: error:
    Could not find module ‘PackageA.Mod2’
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.
  |
4 | import qualified PackageA.Mod2
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Failed, no modules loaded.

I guess this is still WIP, but this workspace could be useful for testing I suppose

googleson78 · 2022-05-16T19:13:48Z

Thanks a lot @pranaysashank. I'll look into it.

googleson78 · 2022-05-19T06:59:22Z

I just tried out your example repo and the repl starts up fine for me and loads modules, at least for package-a.

How are you running your repl?

Both bazel run //package-a:package-a@repl and defining an explicit haskell_repl target and doing bazel run //package-a:repl work for me.

googleson78 · 2022-05-19T07:00:59Z

Derp, I see. I'll look into why it doesn't work cross package.

googleson78 · 2022-05-19T07:36:34Z

Including your cross-library dependencies from package-a in the modules of the package-b haskell_library makes the repl work fine.

So this is either

an issue where we're not taking into account cross_library_deps somewhere for the repl target
a missing check to yell "things are missing from the modules attribute of your haskell_library"

Seeing as how the library builds without them included in there I assume it's the first one.

pranaysashank · 2022-05-19T07:58:15Z

For the repl target, is it better to have behaviour similar to the normal build without modules attribute i.e. we build the cross package libraries that we depend on, instead of creating a repl with all the modules across packages.

Perhaps you're missing building narrowed_deps in the repl target.

googleson78 · 2022-05-26T13:35:59Z

After a lot of struggling with the "empty libs" mechanism, I managed to get your example working in my latest commit, 346cdd7, by propagating the repl aspect along narrowed_deps too.

I don't think this is the proper solution however, since this causes the repl target to rebuild all the modules in libraries specified in narrowed_deps, regardless if they are used.

@facundominguez can you offer some help here?

googleson78 · 2022-05-26T13:36:17Z

TODO: I should also add your example as a test to the repo

EDIT: done

googleson78 · 2022-05-26T13:56:14Z

After a short discussion with @facundominguez, it turns out it might not be possible or it might be rather complicated. I think it would be better to leave it as an open question for another issue, as something working slowly is better than not working at all.

googleson78 · 2022-05-26T13:59:05Z

haskell/repl.bzl

@@ -182,6 +182,7 @@ def _create_HaskellReplCollectInfo(target, ctx):

    hs_info = target[HaskellInfo]

+    # TODO[GL]: do we need to also take into account narrowed_deps here, everywhere "deps" is checked?


I would imagine it is necessary. To rehearse that we would need to add java dependencies to dependencies of Haskell libraries using modules.

googleson78 · 2022-05-27T09:35:25Z

Re the run failures - one of them seems to be a flaky error the other one I'm not sure about - it passes locally 😿

@pranaysashank

Thanks @pranaysashank

facundominguez · 2022-06-02T19:18:15Z

The changes look good to me in intent, and given that the tests that have been added pass, the PR looks like a net improvement. I'm not versed in the aspects that get the repl working to foresee unknown issues.

The code that manages java and CC dependencies in libraries of narrowed_deps doesn't have a test yet. If you still have steam, it would be necessary to add such tests. Otherwise, just make a new issue to add them later.

googleson78 · 2022-06-03T07:15:36Z

Apart from the CC and java dependencies, it would also be nice to have tests for hie bios output. I'll try to add all three next week.

googleson78 · 2022-06-08T07:28:00Z

Actually, I can't seem to find tests for the normal repl usage with cc/java deps 😅 If they exist, can you point me to them? If not, I think we should leave this as a separate issue (add tests for both "normal" and module usage).

facundominguez · 2022-06-08T12:50:09Z

@aherrmann is this good to merge?

aherrmann

Yes, that looks great. Thank you @googleson78!

googleson78 · 2022-06-09T06:06:09Z

Opened #1756 and #1757

googleson78 mentioned this pull request Apr 28, 2022

Support hsc in haskell_module rules #1731

Merged

googleson78 commented Apr 28, 2022

View reviewed changes

googleson78 force-pushed the 1732_repl-for-haskell-modules-targets branch from 205cb61 to 87ccbdf Compare May 4, 2022 12:06

googleson78 commented May 4, 2022

View reviewed changes

tests/haskell_module/repl/haskell_module_repl_test.go Show resolved Hide resolved

k1nkreet reviewed May 5, 2022

View reviewed changes

tests/haskell_module/repl/haskell_module_repl_test.go Outdated Show resolved Hide resolved

Clarify set vs depset in some docs

f99591c

googleson78 force-pushed the 1732_repl-for-haskell-modules-targets branch from 4e5b498 to 5dedb95 Compare May 13, 2022 13:05

googleson78 added 4 commits May 13, 2022 16:09

Implement repl support for targets with modules

a7daf49

Add a test for haskell_repl with haskell_modules

3d5a976

Improve docs for repl_info

a1f27bd

Delete redundant filegroup from test

4cc50a1

googleson78 force-pushed the 1732_repl-for-haskell-modules-targets branch from 5dedb95 to 4cc50a1 Compare May 13, 2022 13:10

googleson78 commented May 26, 2022

View reviewed changes

googleson78 added 2 commits May 27, 2022 11:58

Propagate repl aspect along narrowed_deps

cc69c2e

Add unresolved question on narrowed_deps checking

02ed772

googleson78 force-pushed the 1732_repl-for-haskell-modules-targets branch from e5c2025 to 90152f1 Compare May 27, 2022 09:09

Add a repl module test for cross_library_deps

c329cb1

Thanks @pranaysashank

googleson78 added 2 commits May 30, 2022 13:08

Add a TODO to use provide and required_providers

e85b123

Refactor adding deps and narrowed_deps

3f03379

googleson78 force-pushed the 1732_repl-for-haskell-modules-targets branch from e581c48 to 3f03379 Compare May 30, 2022 10:08

googleson78 added 2 commits May 30, 2022 14:11

Also add java deps from narrowed_deps

e308a11

Also add CcInfo deps from narrowed_deps

46440f8

facundominguez mentioned this pull request Jun 3, 2022

Support fine grained module level build definitions #1552

Open

aherrmann approved these changes Jun 8, 2022

View reviewed changes

facundominguez added the merge-queue merge on green CI label Jun 8, 2022

Merge branch 'master' into 1732_repl-for-haskell-modules-targets

3997eaa

mergify bot merged commit d3caf0c into master Jun 8, 2022

mergify bot deleted the 1732_repl-for-haskell-modules-targets branch June 8, 2022 14:18

mergify bot removed the merge-queue merge on green CI label Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement haskell_repl support for targets that have modules in them #1733

Implement haskell_repl support for targets that have modules in them #1733

googleson78 commented Apr 28, 2022

googleson78 commented Apr 28, 2022 •

edited

Loading

googleson78 Apr 28, 2022

ylecornec Apr 28, 2022

googleson78 May 4, 2022

googleson78 Apr 28, 2022

googleson78 May 30, 2022

k1nkreet left a comment

googleson78 commented May 11, 2022

pranaysashank commented May 16, 2022 •

edited

Loading

googleson78 commented May 16, 2022

googleson78 commented May 19, 2022

googleson78 commented May 19, 2022

googleson78 commented May 19, 2022

pranaysashank commented May 19, 2022

googleson78 commented May 26, 2022

googleson78 commented May 26, 2022 •

edited

Loading

googleson78 commented May 26, 2022 •

edited

Loading

googleson78 May 26, 2022

facundominguez May 27, 2022

googleson78 commented May 27, 2022 •

edited

Loading

facundominguez commented Jun 2, 2022

googleson78 commented Jun 3, 2022

googleson78 commented Jun 8, 2022

facundominguez commented Jun 8, 2022

aherrmann left a comment

googleson78 commented Jun 9, 2022

		@@ -182,6 +182,7 @@ def _create_HaskellReplCollectInfo(target, ctx):

		hs_info = target[HaskellInfo]

		# TODO[GL]: do we need to also take into account narrowed_deps here, everywhere "deps" is checked?

Implement haskell_repl support for targets that have modules in them #1733

Implement haskell_repl support for targets that have modules in them #1733

Conversation

googleson78 commented Apr 28, 2022

googleson78 commented Apr 28, 2022 • edited Loading

googleson78 Apr 28, 2022

Choose a reason for hiding this comment

ylecornec Apr 28, 2022

Choose a reason for hiding this comment

googleson78 May 4, 2022

Choose a reason for hiding this comment

googleson78 Apr 28, 2022

Choose a reason for hiding this comment

googleson78 May 30, 2022

Choose a reason for hiding this comment

k1nkreet left a comment

Choose a reason for hiding this comment

googleson78 commented May 11, 2022

pranaysashank commented May 16, 2022 • edited Loading

googleson78 commented May 16, 2022

googleson78 commented May 19, 2022

googleson78 commented May 19, 2022

googleson78 commented May 19, 2022

pranaysashank commented May 19, 2022

googleson78 commented May 26, 2022

googleson78 commented May 26, 2022 • edited Loading

googleson78 commented May 26, 2022 • edited Loading

googleson78 May 26, 2022

Choose a reason for hiding this comment

facundominguez May 27, 2022

Choose a reason for hiding this comment

googleson78 commented May 27, 2022 • edited Loading

facundominguez commented Jun 2, 2022

googleson78 commented Jun 3, 2022

googleson78 commented Jun 8, 2022

facundominguez commented Jun 8, 2022

aherrmann left a comment

Choose a reason for hiding this comment

googleson78 commented Jun 9, 2022

googleson78 commented Apr 28, 2022 •

edited

Loading

pranaysashank commented May 16, 2022 •

edited

Loading

googleson78 commented May 26, 2022 •

edited

Loading

googleson78 commented May 26, 2022 •

edited

Loading

googleson78 commented May 27, 2022 •

edited

Loading