-
-
Notifications
You must be signed in to change notification settings - Fork 22
Link with cc_common.link
#135
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
Link with cc_common.link
#135
Conversation
|
Ok, windows fails since I didn't get the library name right... Is it I don't yet understand why Linux fails though... Works for me just fine. |
I think you are using a llvm C toolchain on your machine, but github runners have gcc installed. |
I've tried with I will try using a container with exact versions GHA is using later. |
|
I don't think that action_env will help here. The detected local cc_toolchain will still be llvm and thus it will use the llvm linker (lld). |
This is required to use `cc_common.link`
With 0.2.0 I'm getting the error about the missing bzl file.
To properly integrate PIC support with rules_cc, we need to create both PIC/non-PIC versions of every object, like rules_cc does, and pass both to cc_common.create_library_to_link/create_compilation_outputs
This is the main change. Instead of linking with D compiler, call `cc_common.link` for better portability. This requires some changes, mainly we don't collect libraries anymore, instead we build `LinkingContext` for rules_cc. This simplifies handling of C dependencies, but add a bit of wrapping around D libraries.
Also we are passing the flags directly to CC now, so we need to drop -L.
cf9c28e to
7e99dde
Compare
Yes, you are right! Even though setting CC=gcc via action_env does make it link with gcc as a driver, it still passes It turned out to be about the order of libs to link, apparently lld is more permissive here. Now it still fails on windows, but now it can find libc functions, like |
dcarp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It looks good! I added two comments.
And move environment handling to callers.
87d8228 to
be400ae
Compare
I'm still not fully convinced this is the right approach, but it simplifies some things and hopefully provides better compatibility with different cc toolchains.
I've tested it on Mac/ARM64 and Linux/AMD64.
Looking forward to feedback.