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

Properly pass the CC toolchain to cabal #1325

Merged
merged 7 commits into from
May 25, 2020

Conversation

cocreature
Copy link
Collaborator

Cabal’s with-gcc option does not apply to the cases where GHC itself
calls the C compiler which actually happens all the time. We noticed
this in daml and tested this patch there where it works successfully.

The flags here are the same as in ghc_cc_program_args. I’m not quite
sure what the best way to deduplicate this is or if this is worth
doing.

@@ -143,6 +143,18 @@ with tmpdir() as distdir:
"--with-hc-pkg=" + ghc_pkg,
"--with-ar=" + ar,
"--with-gcc=" + cc,
"--ghc-option=-pgmc=" + cc,
Copy link
Member

Choose a reason for hiding this comment

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

Could this re-use ghc_cc_program_args to avoid these flags diverging in future?
Using something like "%{ghc_cc_args}": repr(ghc_cc_program_args(cc)) here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah that looks good, I’ll give it a shot. Wasn’t quite sure how to best pass this down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I’ve tried it but just passing it on doesn’t quite work. I’m missing the find_exe logic from the python script in that case so cc points to the wrong location. I’m not really sure what to do about this. I could imagine some logic in the python script that applies find_exe to flag that don’t start with - but that seems worse than inlining the flags here. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, that's unfortunate. ghc_cc_program_args just expects a string representing cc, so we could also use a place holder and fill it in on the python side. Something like this:
bzl:

    "%{ghc_cc_args}": repr(ghc_cc_program_args("$CC"))

python:

    ] +
    [ arg.replace("$CC", cc) for arg in %{ghc_cc_args} ] +
    [

That's getting on the verge, but seems still better than duplicating the flags.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clever, I like it! I’ve made the change.

@mboes mboes requested a review from aherrmann May 10, 2020 07:49
@aherrmann
Copy link
Member

The CI failure is because the cc_wrapper assumes to be running in the execroot when producing the final artifact and optimizing the RUNPATH. But, the Cabal wrapper changes into the directory that contains the .cabal file.

@aherrmann aherrmann self-assigned this May 18, 2020
@aherrmann
Copy link
Member

This is finally working, though I've had to make a fair bit of additions, so it would be good to get someone else's review on this.

@aherrmann aherrmann requested review from thufschmitt and mboes May 25, 2020 12:29
Copy link
Contributor

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

I can’t say I have any insight in the contents, but the code looks good.

@aherrmann aherrmann added the merge-queue merge on green CI label May 25, 2020
cocreature and others added 7 commits May 25, 2020 15:03
Cabal’s with-gcc option does not apply to the cases where GHC itself
calls the C compiler which actually happens all the time. We noticed
this in daml and tested this patch there where it works successfully.

The flags here are the same as in ghc_cc_program_args. I’m not quite
sure what the best way to deduplicate this is or if this is worth
doing.
The cc_wrapper removes unneeded RUNPATHs and merges redundant RUNPATHs.
The corresponding logic requires the final output binary to be produced
at a path from which the relative RUNPATH entries to Bazel managed
dynamic libraries are valid. This change accomplishes this by building
within a temporary directory that is a sibling of the final output
directory.
MACH-O load commands can take the form `@rpath/some/dir/libfoo.dylib`.
In that case we want to take `some/dir` into account for the runpath
check.
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.

3 participants