feat: add support for translate-c#517
Conversation
|
Once approved, I would recommend a squash and merge. |
cerisier
force-pushed
the
cerisier/translate_c
branch
from
d1d1998 to
8820c70
Compare
aherrmann
left a comment
aherrmann
left a comment
There was a problem hiding this comment.
Thank you! That looks like it was a lot of work!
This largely looks good to me. I've left a few comments and questions on some of the details.
| direct_inputs = [] | ||
| transitive_inputs = [] | ||
|
|
||
| solib_parents = [] |
There was a problem hiding this comment.
I do bazel test //... in both root and e2e/workspace and it works. I've had to fix a few things already so should be good. Unless it is tested otherwise ?
There was a problem hiding this comment.
That tests Zig docs generation for targets without C deps.
But, here's a failure to generate Zig docs I found with this changeset:
$ (cd e2e/workspace; bazel build //translate-c/transitive-cc-library-zig-binary:binary --output_groups=zig_docs)
To turn that into a proper test you can use filegroup(..., output_group = "zig_docs") and tag it as "zig-docs".
There was a problem hiding this comment.
Thanks, let me address that right now
There was a problem hiding this comment.
Fixed. There was an issue of duplicate output from zig_translate_c since we copy the rule impl almost line by line for zig_docs. I added a prefix argument for generated file paths in zig_translate_c.
However, I was thinking that maybe we should have this baked in the zig_build_impl along with a BuildSetting to conditionally -femit-docs. WDYT ?
There was a problem hiding this comment.
Yeah, I saw that. I think my choice to have separate zig_build_impl and zig_docs_impl functions as we have them today and effectively duplicate part of the implementation was not a good choice. It always seemed a little off to me, but wasn't too much of a problem, but now with the duplicate action there's clearly an issue.
I've contemplated the -femit-docs path. A downside is that the effort to compile and link would be duplicated when switching between docs enabled and disabled. Another option is to merge zig_docs_impl into zig_build_impl, re-use common actions (like the c module) and common flags, and only distinguish what's not commonly shared. This needs a bit of care as well though, as it's easy to accidentally conflate what belongs into which action.
aherrmann
changed the title
| # Only forward the linking context since compilation_context is now handled | ||
| # by Zig through the generated _c.zig. | ||
| cc_info = CcInfo( | ||
| linking_context = cc_common.merge_linking_contexts(linking_contexts = [cc_info.linking_context for cc_info in cc_infos]), | ||
| ) |
There was a problem hiding this comment.
What I mean is that above in that function we already have a call to
cc_info = cc_common.merge_cc_infos(direct_cc_infos = cc_infos)
So, instead merging the linking contexts once again here, we can just pluck the merged one out of that object.
|
@aherrmann all fixed. Thanks for pointing them out ! |
|
@cerisier Lightning fast! Thanks. Note there's also a comment about the docs #517 (comment) |
Addressed |
aherrmann
left a comment
aherrmann
left a comment
There was a problem hiding this comment.
Thank you, that looks good! And thank you for addressing all the comments!
2 participants
This PR addresses #515
It supports:
The important files to review are:
zig_module_info.bzltranslate_c.bzlzig_build.bzlSome explanation of changes:
translate-cis an additional action, I had to split args between global_args and command specific args.@cImportable)--dep c) because it is referenced as a dependency before it can be defined.