-
Notifications
You must be signed in to change notification settings - Fork 277
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
Strip bitcode from imported dynamic frameworks and Swift standard libraries when building without bitcode #881
Conversation
a91b938
to
1e49361
Compare
4c76de5
to
48f3bd6
Compare
fc609f3
to
6dfcbd0
Compare
652bbd6
to
e2be06b
Compare
Please take a look. |
apple/internal/bitcode_support.bzl
Outdated
return bitcode_mode_string | ||
|
||
fail("Internal error: expected bitcode_mode to be one of: " + | ||
"['embedded', 'embedded_markers', 'none'], but got '{}'".format( |
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.
nit: might be nice to extract out the array of allowed values, so we don't have to keep it in sync in both places
@@ -131,6 +135,10 @@ def _framework_import_partial_impl(ctx, targets, targets_to_avoid): | |||
for build_arch in build_archs_found: | |||
args.add("--slice", build_arch) | |||
|
|||
# TODO: Extract `bitcode_strip` into a separate action |
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.
what's the benefit of doing it as a separate action?
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.
It gets cached separately which is more efficient. For example, if we have 2 build configugrations for 2 different archs, it will save us one bitcode_strip
command (assuming we do bitcode_strip
before lipo
).
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.
fyi - bundle & sign has historically been a single action because it actually is faster than two actions. Likewise, things like the thinning of imported frameworks/archives are thinned on import also also speed things up vs. two actions. On local builds, it doesn't really make a difference, but when you talk about remote builds. Copying everything to the machine, doing a job, copying them back, then copying things out, doing another job, and copying it back can end up being significantly slower that just doing one set of copies. Also, the more large copies, the more changes for retransmissions under the hood to the remote host can be required, slowing down the overall build.
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 for the info. I have always been naïvely thinking that the smaller actions you split the compilation into, the faster incremental build you get. That probably only makes sense for actions that take text files as inputs.
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.
Yea, the key is number of inputs/size of inputs/outputs. The costs can also apply somewhat for local builds, but there it is really the counts that matter since the action needs a setup directory to run in, so the more inputs, the more symlinks that have to be created.
libraries when building without bitcode
Gently ping for review/merge. |
Thanks! |
Fixes #327.