Skip to content

Emit correct header in CGo c-archive#2874

Merged
achew22 merged 1 commit intobazel-contrib:masterfrom
CodeIntelligenceTesting:fix-cgo-exports-hdrs
Oct 16, 2021
Merged

Emit correct header in CGo c-archive#2874
achew22 merged 1 commit intobazel-contrib:masterfrom
CodeIntelligenceTesting:fix-cgo-exports-hdrs

Conversation

@fmeum
Copy link
Copy Markdown
Member

@fmeum fmeum commented May 11, 2021 •
edited
Loading

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

A go_binary target with linkmode c-archive emits only a single _cgo_exports.h header instead of all such headers of all transitive dependencies.

Currently, this header is taken to be the first element in a list obtained from the depset of these transitive _cgo_exports.h files
via to_list(). While deterministic, this choice is not guaranteed to give the correct _cgo_exports.h (i.e., the one of the go_binary itself) as the depset iteration order is not specified.

This commit ensures that the _cgo_exports.h of the target itself is added as the first element of the direct list of a depset with iteration order specified as "preorder" and will thus always be the first element when the depset is converted to a list.

Which issues(s) does this PR fix?

The CcInfo hdrs exposed by a go_binary using CGo that itself has dependencies using CGo is essentially arbitrary (but deterministic) and thus may not define the exported symbols or even be empty.

@fmeum fmeum requested a review from jayconrod as a code owner May 11, 2021 14:23
@google-cla google-cla bot added the cla: yes label May 11, 2021
@fmeum fmeum force-pushed the fix-cgo-exports-hdrs branch from 044427e to 06ef45c Compare May 11, 2021 19:26
@jayconrod
Copy link
Copy Markdown
Collaborator

jayconrod commented May 27, 2021

I'd rather not merge a fix for this without an issue thoroughly explaining the bug being fixed plus tests that verify it works. cgo export headers are not well documented.

The main issue about this is #2131. go_binary should only expose one header file, named after the binary. That header should declare exported Go definitions from the main package only.

@jayconrod jayconrod requested a review from a team May 27, 2021 21:12
@achew22
Copy link
Copy Markdown
Member

achew22 commented May 27, 2021

+1, I would really only be comfortable merging this if there were a testcase that failed before but passes now

@fmeum fmeum mentioned this pull request May 28, 2021
Open
@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented May 28, 2021

My own example is not public and quite difficult to minimize (cc target depending on go target depending on cc target depending on go target), but #2878 now provides a public example.

I am unfortunately not sure how to write a reliable test for this issue. After all, the problem is that without this fix, rules_go depends on the unspecified default traversal order of a depset. This order may not be guaranteed to remain stable across Bazel versions, but any concept for a test I can think of would have to depend on it in order to fail without the current PR.

@achew22 Do you have an idea how to deal with this situation?

@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Aug 7, 2021 •
edited
Loading

@jayconrod @achew22 Do you want me to add anything before you take another look at this? With #2878, there is now an example of a build that fails without this patch.

@achew22
Copy link
Copy Markdown
Member

achew22 commented Aug 7, 2021

That's great! I think next steps here before merging would be to add a testcase into the repo that demonstrates this behavior so that the purpose of this change is effectively documented. Thanks!

@fmeum fmeum force-pushed the fix-cgo-exports-hdrs branch from 06ef45c to 21ebb95 Compare August 7, 2021 20:12
@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Aug 7, 2021

@achew22 Done, I have turned @glukasiknuro's example into a testcase that does compile and run with this PR, but fails to compile without it.

Please let me know if you need anything else.

@fmeum fmeum force-pushed the fix-cgo-exports-hdrs branch 2 times, most recently from 2f1802b to fc53cd9 Compare August 7, 2021 20:15
@achew22
Copy link
Copy Markdown
Member

achew22 commented Aug 7, 2021

Huh, looking at the test output I'm seeing:

(20:16:59) ERROR: Analysis of target '//tests/core/c_linkmodes:go_with_cgo_dep_caller' failed; build aborted: Analysis of target '//tests/core/c_linkmodes:go_with_cgo_dep_caller' failed
 ```

Got any theories?

@fmeum fmeum force-pushed the fix-cgo-exports-hdrs branch from fc53cd9 to af76cd7 Compare August 7, 2021 21:09
@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Aug 7, 2021

Huh, looking at the test output I'm seeing:

(20:16:59) ERROR: Analysis of target '//tests/core/c_linkmodes:go_with_cgo_dep_caller' failed; build aborted: Analysis of target '//tests/core/c_linkmodes:go_with_cgo_dep_caller' failed
 ```

Got any theories?

Yes, I forgot to load cc_binary from rules_cc. Should be fixed now. I also added myself to CONTRIBUTORS, in case that's required.

@fmeum fmeum force-pushed the fix-cgo-exports-hdrs branch from af76cd7 to e4e7222 Compare August 7, 2021 21:13
@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Aug 21, 2021

@achew22 Sorry, I didn't notice that the CI checks were failing. This PR is ready for review now. The race detector check is failing on Windows though, do you know whether this is related to my changes?

@steeve
Copy link
Copy Markdown
Contributor

steeve commented Aug 21, 2021

@achew22 Sorry, I didn't notice that the CI checks were failing. This PR is ready for review now. The race detector check is failing on Windows though, do you know whether this is related to my changes?

I don't think it's related to your changes. We have disabled it on master, you might want to try to rebase on it.

@fmeum fmeum force-pushed the fix-cgo-exports-hdrs branch from b2cb6d3 to af83807 Compare August 21, 2021 13:58
@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Aug 21, 2021

@steeve That worked, thanks. Should be fully ready now.

@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Sep 2, 2021

@achew22 The tests are all passing now, could you take another look?

@robfig robfig assigned robfig, achew22 and steeve and unassigned robfig Sep 23, 2021
@robfig robfig added the cgo label Sep 23, 2021
@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Sep 29, 2021

I resolved the merge conflict.

@borancar
Copy link
Copy Markdown

borancar commented Oct 1, 2021 •
edited
Loading

This would fix #2971

@fmeum
Emit correct header in CGo c-archive
72319b3 
A go_binary target with linkmode c-archive emits only a single
_cgo_exports.h header instead of all such headers of all transitive
dependencies.

Currently, this header is taken to be the first element in a list
obtained from the depset of these transitive _cgo_exports.h files
via to_list(). While deterministic, this choice is not guaranteed to
give the correct _cgo_exports.h (i.e., the one of the go_binary
itself) as the depset iteration order is not specified.

This commit ensures that the _cgo_exports.h of the target itself is
added as the first element of the `direct` list of a depset with
iteration order specified as `preorder` and will thus always be the
first element when the depset is converted to a list.

The commit adds a testcase that does not compile without it:
If a Go library that depends on a CGo library (in this case x/sys/unix)
is itself depended on by a cc_library as a c-archive, the emitted CGo
header is that of the CGo library, not the Go library itself.
@fmeum fmeum force-pushed the fix-cgo-exports-hdrs branch from f51820a to 72319b3 Compare October 4, 2021 13:28
@fmeum
Copy link
Copy Markdown
Member Author

fmeum commented Oct 16, 2021

@achew22 Do you think you could take a look? It seems that people keep hitting the cgo bug this fixes.

@achew22 achew22 merged commit f4c5bd7 into bazel-contrib:master Oct 16, 2021
@JamyDev JamyDev mentioned this pull request Jan 18, 2022
Merged
@fmeum fmeum deleted the fix-cgo-exports-hdrs branch May 14, 2022 22:20
yushan26 pushed a commit to yushan26/rules_go that referenced this pull request Jun 16, 2025
@aignas
refactor: consolidate version parsing (bazel-contrib#2874)
ee8d7d6 
This PR removes all of the custom version parsing functions where we try
to make sense about the version (e.g. extracting major/minor versions).

Whilst doing this I actually think that I made it easier to support
bazel-contrib#2837.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jayconrod jayconrod Awaiting requested review from jayconrod

Assignees

@achew22 achew22

@steeve steeve

Labels

cgo cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fmeum @jayconrod @achew22 @steeve @borancar @robfig