Add support for profiling #332
Conversation
|
OK, 7 tests fail with profiling. I'll look into this. |
|
My |
|
This is ready. |
| @@ -175,7 +174,36 @@ def _process_chs_file(hs, cc, ghc_defs_dump, chs_file, chi_files=[]): | |||
|
|
|||
| return hs_out, chi_out, idir | |||
|
|
|||
| def _compilation_defaults(hs, cc, java, dep_info, srcs, extra_srcs, cpp_defines, compiler_flags, main_file = None, my_pkg_id = None): | |||
| def _output_file_ext(base, dynamic, profiling_enabled): | |||
There was a problem hiding this comment.
I find this function doesn't pay its own way and could be more harmful than good. The cheap option is to construct the file name extensions manually every time: .p_o_dyn etc. It should prove more readable. Also your line breaking below won't need to be as ugly, because the declare_compiled stuff will fit on one line.
There was a problem hiding this comment.
Yes and no... I think it reduces the possibility of typos somewhat, but the main reason is the with_profiling argument. If we "inline" _output_file_ext (with some beta-reduction) there will be a lot of conditionals of the form:
"." + ("_p" if with_profiling else "") + "dyn_o"
Etc. which is not much better and definitely not shorter. I'd leave the function "as is".
(Well another option would be something like this: ".p_dyn_o" if with_profiling else ".dyn_o" which is typo-prone I think and not really shorter either.)
| java, | ||
| dep_info, | ||
| srcs = ctx.files.srcs, | ||
| # NOTE We must make the object files compiled without profiling |
There was a problem hiding this comment.
Or use -fext-external-interpreter, which I believe should resolve this kind of problem. See https://ghc.haskell.org/trac/ghc/wiki/RemoteGHCi. I'm surprised it's still not the default - we should pass that flag systematically if not.
There was a problem hiding this comment.
You meant -fexternal-interpreter I think?
This looks good, although I'm not sure it makes sense to spend more time switching to that as I've already spent more hours than I expected to spend with this. Also, right now we're doing what Cabal is doing here. Of course Cabal works that way because it needs to support older GHC, but maybe there are other undiscovered issues with -fexternal-interpreter? CC @facundominguez, he mentioned something like "there may be issues with that".
Let's open an issue for the future and leave this "as is" so I can spend more time on client's stuff instead of re-working the working solution that replicates what Cabal does?
There was a problem hiding this comment.
Subtleties like this in the codebase make me uncomfortable, because they'll trip us up at some point in the future. If it works, then we can keep it for now. But at a minimum, file a ticket and link to it from the code, so that people know to look out.
-fexternal-interpreter was introduced specifically to solve this problem. We don't have to, and shouldn't, replicate anything in Cabal that is just a historical workaround. If this flag introduces other issues, we should make our decisions based on specific references, not hypotheticals (e.g. https://ghc.haskell.org/trac/ghc/ticket/14335).
| @@ -212,8 +250,11 @@ def _compilation_defaults(hs, cc, java, dep_info, srcs, extra_srcs, cpp_defines, | |||
| args.add(compiler_flags) | |||
| haddock_args.add(compiler_flags, before_each="--optghc") | |||
|
|
|||
| # Output static and dynamic object files. | |||
| args.add(["-static", "-dynamic-too"]) | |||
There was a problem hiding this comment.
If I understand correctly this hack is a workaround to an upstream issue in GHC. It would be good to call that out explicitly, and to look to the relevant upstream issue.
There was a problem hiding this comment.
I don't know if there is a ticket for this, actually. CC @hvr
There was a problem hiding this comment.
After some searching, opened https://ghc.haskell.org/trac/ghc/ticket/15394 for now. Even if it's a dupe, we'll know that soon.
There was a problem hiding this comment.
I added a note in the source too.
This PR adds support for building things with profiling.