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

Basic support for profiling, PIC and hsc2hs #9

Closed
wants to merge 10 commits into from
Closed

Conversation

Fuuzetsu
Copy link
Collaborator

See commentary on individual commits for additional info (if any).

Mateusz Kowalczyk added 5 commits November 28, 2017 11:56
These do not propagate into rules because aspects are awful. There is,
as far as I can figure out, no way to have a uniform rule
`haskell_library` such that a configuration option in it is propagated
into its dependencies. At first glance aspects look like they could
work but problem comes with the "uniform" requirement. Consider

```
haskell_library(
  name = 'foo',
  deps = ['bar'],
  compilerFlags = "-O2"
)

haskell_library(
  name = 'bar',
  deps = ['baz'],
  compilerFlags = ""
)

haskell_library(
  name = 'baz',
  deps = [],
  compilerFlags = ""
)
```

Suppose we have some aspect `A` that's included in `haskell_library`
that propagates `compilerFlags`. When we request `foo`, we would like
to get the following:

`foo -O2` <- `bar -O2` <- `baz -O2`.

In other words `foo <- foo(bar) <- foo(baz)`. In reality we get
something like this:

`foo -O2` <- `bar -O2` <- `baz -O2`
                    \- <- `baz`

Why? Simply because `bar` is also an aspected rule. Notably `foo(bar)`
results in `foo(bar(baz))` as well as `foo(baz)` and because
`compilerFlags` is empty in `bar`, we end up having two different rule
invocations: one with -O2 and one without; `bar(baz) = baz ""` but
`foo(baz) = baz "-O2"`. Not only would this cause 2 builds, Bazel
actually catches this earlier in our rules as two invocations of `baz`
declare the same output file and Bazel complains:

```
Owner information: []#@io_tweag_rules_haskell//haskell:haskell.bzl%haskell_aspect 230ec99d878b1ad3a4b02084390f466b 230ec99d878b1ad3a4b02084390f466b {ghcFlags=[]}, []#@io_tweag_rules_haskell//haskell:haskell.bzl%haskell_aspect 230ec99d878b1ad3a4b02084390f466b 230ec99d878b1ad3a4b02084390f466b {ghcFlags=[-O2]}
```

We could disambiguate output files based on input config but it does
not fix spurious build.

`-O2` was just an example and there really is configuration that we
want to propagate downwards. Consider profiling builds: we have to
ensure that every dependency is built with profiling. Ideally we would
just say `profiling = True` in any `haskell_library` or
`haskell_binary` and get everything down the chain built
appropriately: basically think about `stack build
--enable-library-profiling`.

There is non-solution that we might be tempted with and that is to
give up uniform rule interface: make aspected rules explicit and make
everything else use vanilla rules. In the example above, this would
mean that `foo` stops being `haskell_library` and becomes
`haskell_library_aspected`. This sadly breaks down the moment we have
another `haskell_library_aspected` in the chain with exactly the same
issue that was demonstrated above. It is also unclear how to deal with
the case where only `bar` is `haskell_library_aspected`: if `foo` can
check that `bar` has a provider with information from the aspect
instead of from vanilla build, it may work.

Another non-solution is inverting responsibility for configuration: we
_can_ propagate configuration *upwards* in Skylark. We can ask "was
this dependency built with profiling?". Sadly this buys us nothing
meaningful: a dependency built with profiling or `-O2` does not
indicate anything to the current library about developer intent. We
could of course have a "buildEverythingWithProfiling" flag but the
developer still has to then manually find all the Haskell leaves of
the dependency graph and add it there (because what should we do if
one dependency has the flag set and another doesn't?). It's awful.

Last problem with aspects is that they are "add-ons" on top of
existing builds. They are useful for example if you want to walk your
rules and for every source file generate TAGS file. In order to take
advantage of aspect-configured parameters, you have to move in all
your build logic into aspect implementation however we just seemed to
agree that we could want both aspects and vanilla rules. This means a
large complexity increase in build rules. Even with this it may not be
a viable approach: we have to switch on whether the rule is running
aspected (indicated through some configuration attribute) and if yes
produce no outputs except indicating to upstream that it was
aspected (so things depending on it know how to query it), run the
whole build in aspect implementation, provide the right thing. Next
rule up then has to do The Right Thing™ as well &c. It gets pretty
complicated when you have to collect and unify depsets at the end. I
think diamond dependencies would be insane.

Having mentioned the problems, I think there is only one sane way
forward and that is to shift the burden of configuration onto the
users. What this means is that user defines wrappers (macros?) around
our rules that read their configuration. The user defines in a single
place a configuration struct and a macro that generates binary/library
rule out of the struct then all their BUILD files call out to the
macro. If you want profiling everywhere then you got it, change the
configuration. This also allows developers some granularity: if they
have a group of packages that are often worked on together, they can
have a separate config for that. The buy-in seems large but I think
it's OK: if your project is large with many targets, the cost of
setting up will be relatively small to all the effort you have to put
into setting all the BUILD files &c. anyway. The overhead is just
defining then calling your macro vs calling `haskell_library`
directly. If your project is small, you likely don't need this level
of configuration at all and can get away with just using vanilla rules
+ manually adjusting flags everywhere when/if you need it.
Profiling fails at link time. I think we might need to link against
RTS but I had no such luck. I tried to figure out how to do profiling
build in Buck to see what they do but I failed to find a way. I hacked
Buck to have profiling enabled out of the box and it turns out they
have exactly the same problem.

```
ERROR: /home/shana/.cache/bazel/_bazel_shana/5c33788dab2bc46ef34646c3807ca729/external/examples/hello-world/BUILD:8:1: Linking hello-world failed (Exit 1).
bazel-out/local-fastbuild/bin/external/examples/hello-world/objects/Main.p_o(.data+0x108): error: undefined reference to 'CCS_DONT_CARE'
bazel-out/local-fastbuild/bin/external/examples/hello-world/objects/Main.p_o(.data+0x128): error: undefined reference to 'CCS_DONT_CARE'
bazel-out/local-fastbuild/bin/external/examples/hello-world/objects/Main.p_o(.data+0x148): error: undefined reference to 'CCS_DONT_CARE'
bazel-out/local-fastbuild/bin/external/examples/hello-world/objects/Main.p_o:ghc_3.c:function prof_init_Main: error: undefined reference to 'CCS_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-world/objects/Main.p_o:ghc_3.c:function prof_init_Main: error: undefined reference to 'CCS_ID'
bazel-out/local-fastbuild/bin/external/examples/hello-world/objects/Main.p_o:ghc_3.c:function prof_init_Main: error: undefined reference to 'CC_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-world/objects/Main.p_o:ghc_3.c:function prof_init_Main: error: undefined reference to 'CC_ID'
bazel-out/local-fastbuild/bin/external/examples/hello-lib/lib/libhello-lib.a(Lib.p_o)(.data+0x108): error: undefined reference to 'CCS_DONT_CARE'
bazel-out/local-fastbuild/bin/external/examples/hello-lib/lib/libhello-lib.a(Lib.p_o):ghc_12.c:function prof_init_hellozmlibzm1zi0zi0_Lib: error: undefined reference to 'CCS_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-lib/lib/libhello-lib.a(Lib.p_o):ghc_12.c:function prof_init_hellozmlibzm1zi0zi0_Lib: error: undefined reference to 'CCS_ID'
bazel-out/local-fastbuild/bin/external/examples/hello-lib/lib/libhello-lib.a(Lib.p_o):ghc_12.c:function prof_init_hellozmlibzm1zi0zi0_Lib: error: undefined reference to 'CC_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-lib/lib/libhello-lib.a(Lib.p_o):ghc_12.c:function prof_init_hellozmlibzm1zi0zi0_Lib: error: undefined reference to 'CC_ID'
bazel-out/local-fastbuild/bin/external/examples/hello-sublib/lib/libhello-sublib.a(MessageSuffix.p_o):ghc_3.c:function prof_init_hellozmsublibzm1zi0zi0_MessageSuffix: error: undefined reference to 'CCS_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-sublib/lib/libhello-sublib.a(MessageSuffix.p_o):ghc_3.c:function prof_init_hellozmsublibzm1zi0zi0_MessageSuffix: error: undefined reference to 'CCS_ID'
bazel-out/local-fastbuild/bin/external/examples/hello-sublib/lib/libhello-sublib.a(MessageSuffix.p_o):ghc_3.c:function prof_init_hellozmsublibzm1zi0zi0_MessageSuffix: error: undefined reference to 'CC_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-sublib/lib/libhello-sublib.a(MessageSuffix.p_o):ghc_3.c:function prof_init_hellozmsublibzm1zi0zi0_MessageSuffix: error: undefined reference to 'CC_ID'
bazel-out/local-fastbuild/bin/external/examples/hello-subsublib/lib/libhello-subsublib.a(MessageEnd.p_o):ghc_3.c:function prof_init_hellozmsubsublibzm1zi0zi0_MessageEnd: error: undefined reference to 'CCS_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-subsublib/lib/libhello-subsublib.a(MessageEnd.p_o):ghc_3.c:function prof_init_hellozmsubsublibzm1zi0zi0_MessageEnd: error: undefined reference to 'CCS_ID'
bazel-out/local-fastbuild/bin/external/examples/hello-subsublib/lib/libhello-subsublib.a(MessageEnd.p_o):ghc_3.c:function prof_init_hellozmsubsublibzm1zi0zi0_MessageEnd: error: undefined reference to 'CC_LIST'
bazel-out/local-fastbuild/bin/external/examples/hello-subsublib/lib/libhello-subsublib.a(MessageEnd.p_o):ghc_3.c:function prof_init_hellozmsubsublibzm1zi0zi0_MessageEnd: error: undefined reference to 'CC_ID'
collect2: error: ld returned 1 exit status
```
This links in a profiled RTS and we can link again!
@Fuuzetsu Fuuzetsu requested a review from mboes November 29, 2017 16:15
We suffix all the build directories and files that we make with a
package name and version. As package name is a target name, there can
only be a unique one in single project and we preserve some sanity in
build environment, at least for the directories we declare and pass
around. This allows multiple `haskell_library` and `haskell_binary`
targets in a single package.

Closes #7.
Mateusz Kowalczyk added 4 commits November 30, 2017 13:30
`-package-db` is enough and if we do pass -package, it tries to find
shared library for whatever reason! Worse, it seems to fail even if
the library is present, somehow.
All these have shared libraries that we can find. The additional
benefit of -package is that it _unhides_ packages which are hidden.
For example if we have a dependency on `ghc` package, this is hidden
by default and we unhide it with `-package`.
@Fuuzetsu
Copy link
Collaborator Author

Fuuzetsu commented Dec 7, 2017

I'll reopen at point I have inline-java tests running as I'm working on this branch.

@Fuuzetsu Fuuzetsu closed this Dec 7, 2017
Profpatsch pushed a commit that referenced this pull request Mar 7, 2019
Make it more clear which parts are about setting up a new project, versus
writing rules that use the autogenerated repositories.
Profpatsch pushed a commit that referenced this pull request Mar 7, 2019
Don't hard-code the GHC toolchain
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.

1 participant