-
-
Notifications
You must be signed in to change notification settings - Fork 723
Fix cdeps propagation through embed chains #4522
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
base: master
Are you sure you want to change the base?
Conversation
cgo=True (because it itself embeds a library with cgo=True), the cdeps were not being propagated. This is a regression from v0.51.0. In v0.50.1, cdeps and related cgo options were merged unconditionally: source["cdeps"] = source["cdeps"] or s.cdeps In v0.51.0 (PR bazel-contrib#4030), the logic was changed to only merge these when s.cgo is True: if s.cgo: source["cdeps"] = s.cdeps This breaks the pattern where: - Library A has cgo=True and C code - Library B embeds A, adds cdeps, but doesn't set cgo=True - Test embeds B The cdeps from B don't propagate to the test because B doesn't have cgo=True, causing undefined symbol errors during linking. This fix restores the v0.50.1 behavior: merge cgo-related attributes if the embedded library has them, regardless of the cgo flag.
fmeum
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.
This looks good. Would you be willing to add a test with the described setup?
|
Yes, I have a simple reproducer for the issue so I can turn that into a test. |
|
Thanks! Hadn't realized this was a valid scenario, adding a test would be great |
|
@fmeum I've added the test |
|
Sorry will fix it in CI - I can't build locally due to macOS SDK versions needed to be installed so hadn't actually tested the compile! |
e8938f0 to
aec9719
Compare
This test verifies that cdeps are properly propagated when a go_library with cdeps embeds another go_library with cgo=True, and a go_test embeds the library with cdeps. This pattern was broken in v0.51.0 and fixed in commit a494a68. The test reuses existing native_dep cc_library to minimize test complexity and follows the same pattern as the external reproducer.
|
@fmeum any ideas how to fix the windows build? Seems to be failing to find the msys gcc. |
|
I don't understand why this fails - @jayconrod do you? We exclude some cgo tests on Windows, but similar ones are run and pass. |
|
Sorry, I didn't have much time today to dig into this. But I was able to reproduce the error on my Windows machine. It, er, doesn't look like we print errors from failing C compiler commands. Someone should fix that at some point 😬 But with a quick hack, here's the error output. Does this help? |
When a go_library embeds another go_library that has cdeps but not cgo=True (because it itself embeds a library with cgo=True), the cdeps were not being propagated. This is a regression from v0.51.0.
In v0.50.1, cdeps and related cgo options were merged unconditionally:
source["cdeps"] = source["cdeps"] or s.cdeps
In v0.51.0 (PR #4030), the logic was changed to only merge these when s.cgo is True:
if s.cgo:
source["cdeps"] = s.cdeps
This breaks the pattern where:
The cdeps from B don't propagate to the test because B doesn't have cgo=True, causing undefined symbol errors during linking.
This fix restores the v0.50.1 behavior: merge cgo-related attributes if the embedded library has them, regardless of the cgo flag.