Cleanup some cgo handling #4466
Conversation
|
I like where this is going, but CI is red |
dzbarsky
force-pushed
the
zbarsky/cleanup
branch
from
b173eb0 to
258db29
Compare
Got a bit too impatient with local tests. Should be green now |
| cgo_deps = depset(transitive = [cgo_deps] + [a.cgo_deps for a in direct]), | ||
| cgo_exports = cgo_exports, | ||
| cgo_deps = depset(cgo_deps, transitive = [a.cgo_deps for a in direct]), | ||
| cgo_export = out_cgo_export_h, |
There was a problem hiding this comment.
I guess this is technically a breaking change since GoArchive is public API? Could we keep this backwards compatible by making this a one-element depset?
There was a problem hiding this comment.
I'm not a huge fan of 1-element depsets and code comments that say "this is small so flattening is ok, trust me", they tend to get stale :) I suppose we could add an additional field for it just for backwards-compat, although it feels like anyone consuming it should be able to adjust prety trivially
Really the out_cgo_export_h could have been a plain return from the function, it's just a bit annoying to wire up because the functions are invoked via go.binary calling go.archive.
There was a problem hiding this comment.
@jayconrod Do you think this breakage is minor enough that we can pull it off?
I would want to fix the regressions introduced by the latest minor release before we merge a change that's intentionally breaking though.
There was a problem hiding this comment.
I'd lean toward preserving backward compatibility.
I'm not sure I really understand this change though. cgo_exports has a single direct element, but it also contains files from transitive dependencies. You need all of those to include the top-level file. Why drop those? I'm sure it's possible to reassemble the same set, but it seems harder.
3 participants
What type of PR is this?
Code cleanup
What does this PR do? Why is it needed?
Improve cgo_deps depset construction and simplifies cgo_export handling
Which issues(s) does this PR fix?
Fixes #
Other notes for review