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

Make ‘cc_haskell_import’ work with ‘haskell_binary’ targets #191

Merged
merged 1 commit into from
Mar 27, 2018

Conversation

mrkkrp
Copy link
Member

@mrkkrp mrkkrp commented Mar 19, 2018

Close #179.

This PR allows to generate .so files from object files of executables, not just executables themselves. This way it's possible to use them with cc_haskell_import. Generation of .so version in conditional though, and disabled by default. The reasons:

  • Some existing sh_tests run outputs of targets, and so if executable target produces more than one file they get confused. This could be avoided, if they just used executable from DefaultInfo, which always be a single file, but somehow this is not what sh_test does. We use our own copy of it, so we could patch it and submit the changes upstream.

  • Usually such .so versions of binaries are not needed, so why do extra work.

The both reasons are not very strong IMO, so I'm open to alternative decisions.

@mrkkrp mrkkrp requested a review from mboes March 19, 2018 11:47
@mrkkrp mrkkrp force-pushed the cc_haskell_import-with-haskell_binary branch from c5ef9c8 to 905f30a Compare March 21, 2018 10:35
@mrkkrp mrkkrp mentioned this pull request Mar 23, 2018
Copy link
Member

@mboes mboes left a comment

Choose a reason for hiding this comment

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

As to whether we should create an extra output always or have a flag for that - let's do the former, not the latter.

Bazel only generates the outputs that are needed by dependencies. Listing the output in DefaultInfo only makes it a default output. So if the output is never requested downstream then we'll never do the work to create it, provided the rule actions are structured correctly (independent actions for independent outputs).

But I don't understand what this PR is doing. A Haskell binary is a binary, not a library. #179 did not call for sometimes making a Haskell binary a library, whether through an explicit flag or as an extra (non-default) output. Even when a binary is compiled with -pie, it's still a bona fide executable, named exactly as the user specified.

All I was expecting from this PR was that a provider be returned by haskell_binary, and that this provider be common to haskell_library as well (probably by decomposing the monolithic HaskellPackageInfo into smaller pieces as mentioned in the ticket), so that cc_haskell_import can uniformly deal with either, provided the binary was compiled as PIE (which incidentally, should probably be the default, or perhaps even always the case, given the security benefits).

@mrkkrp
Copy link
Member Author

mrkkrp commented Mar 25, 2018

I tried what you describe (only generate binary) but I could not make C program link against such a binary exposed via cc_haskell_import. I was getting unresolved symbol errors.

Bazel only generates the outputs that are needed by dependencies. Listing the output in DefaultInfo only makes it a default output. So if the output is never requested downstream then we'll never do the work to create it, provided the rule actions are structured correctly (independent actions for independent outputs).

The main issue is not this, but that label of executable target no longer designates single file, but several and so it cannot be used in sh_test for example.

I also have the impression that only outputs listed in DefaultInfo force compilation, and that if you don't specify something is DefaultInfo at all, it won't be ever created. But I may be mistaken.

@mboes
Copy link
Member

mboes commented Mar 25, 2018

The main issue is not this, but that label of executable target no longer designates single file, but several and so it cannot be used in sh_test for example.

Yes. You mentioned in the PR description that an alternative would be to fix sh_test and then contribute upstream. I'm all for that. But I want to first understand why a .so is a necessity in the first place. It's possible that cc_* rules do something special based on the name of the output. In that case the only "work" necessary should be creating a symlink called libfoo.so to binary foo.

@mrkkrp
Copy link
Member Author

mrkkrp commented Mar 25, 2018

I don't think this is going to work because while I was experimenting with this, I tried generating an executable and naming it with lib prefix and .so extension and it was not accepted by the linker. It started working only when I added the -shared flag, which means that we need two outputs: binary and so library if we want this to be visible to C code via cc_haskell_import.

@mrkkrp
Copy link
Member Author

mrkkrp commented Mar 25, 2018

Can try once again tomorrow if you like.

@mrkkrp mrkkrp force-pushed the cc_haskell_import-with-haskell_binary branch 2 times, most recently from 1b2e0d8 to 1f1ba7f Compare March 26, 2018 08:57
@mrkkrp
Copy link
Member Author

mrkkrp commented Mar 26, 2018

Amended the PR.

  • The refactoring moves generation of dummy archive in its own function.

  • We need to use dynamic object files if we want to compile with -dynamic.

  • Handling of dynamic object files moved to _compilation_defaults so we have everything in one place (handling of dynamic and static object files). This will also allow to drop _hs_srcs and we'll be able to build list of source files (including source pre-processed with hsc2hs)/object files, etc. in one place only, in _compilation_defaults, and avoid re-building it. I did this in a later PR.

  • Setting allow_single_file = True for dep attribute of cc_haskell_import won't work because rules like haskell_library generate multiple files (in its DefaultInfo), so Bazel will complain.

  • According to the docs of cc_haskell_import we should make available

    the shared object files produced as well as the object files it depends on directly and transitively

    I preserved this logic, instead of making available only file from direct target (your branch). (I guess there is a reason it's done this way.)

So far I can't build clodl with Bazel so I can't test if it works with the changes.

@mrkkrp mrkkrp force-pushed the cc_haskell_import-with-haskell_binary branch from 1f1ba7f to 9ece46c Compare March 26, 2018 13:45
@mboes mboes merged commit 211ba0b into master Mar 27, 2018
@mboes mboes deleted the cc_haskell_import-with-haskell_binary branch March 27, 2018 21:38
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.

2 participants