-
Notifications
You must be signed in to change notification settings - Fork 90
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
[Bazel 6.x.x] sim_arm64 middleman fixes #767
Conversation
rules/import_middleman.bzl
Outdated
if use_lts_5_rules_apple_api: | ||
objc_provider_fields["static_framework_file"] = depset(replaced_static_framework.inputs) | ||
else: | ||
# See comment in file above regarding this name in 6.x.x |
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.
Is this referring to L103? It might be more straightforward to repeat it here
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.
+1, since there's a few occurrences one suggestion is to move the comment in L103 to where use_lts_5_rules_apple_api
is initialized? This way readers wondering about the conditionals can simply go back to that one line for context.
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.
Yes, I have added a Notes
segment to the top of the doc WRT this: https://github.com/bazel-ios/rules_ios/pull/767/files#diff-e213489d3a65f173ffa46728826b06f786a5d63666e3fe7101493397ee12f948R2
rules/import_middleman.bzl
Outdated
replaced_frameworks = replaced_dyanmic_framework.values() + replaced_static_framework.replaced.values() | ||
all_replaced_frameworks = replaced_dyanmic_framework.values() + replaced_static_framework.replaced.values() | ||
|
||
# This slightly varies in Bazel 6.x.x |
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.
Can you expand this comment to explain what is different between the versions and needs unique handling for each?
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.
@jerrymarino correct me if I'm wrong but I believe what this is doing is handling only dynamic fmws in Bazel 6 since static fmws are handled here: #766. If that's the case a small comment with this context would be preferred IMO.
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.
Yeah you got it @thiagohmcruz - the Notes
blurb now links this: https://github.com/bazel-ios/rules_ios/pull/767/files#diff-e213489d3a65f173ffa46728826b06f786a5d63666e3fe7101493397ee12f948R2
|
||
compat_link_opt = ["-L__BAZEL_XCODE_DEVELOPER_DIR__/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/iphonesimulator", "-Wl,-weak-lswiftCompatibility51"] | ||
|
||
if len(replaced_frameworks): | ||
if len(all_replaced_frameworks): |
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.
Is it intentional that linkopt
below is still using replaced_frameworks
instead of all_replaced_frameworks
?
["\"\"\"-F" + "/".join(f.path.split("/")[:-2]) + "\"\"\"" for f in replaced_frameworks] + compat_link_opt,
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.
That is the gist of it - 6.x.x static_framework
was transitioned to imported_library
and is in the filelist
not here
In Bazel 6.x.x the `static_framework_file` was migrated onto `imported_library` this PR completes the migration and is followup to the Bazel 6 work. The related rules_apple change which updated `static_framework_file`: bazelbuild/rules_apple@8d84134
94cb5b1
to
53c0d54
Compare
53c0d54
to
d7e2f50
Compare
In Bazel 6.x.x the
static_framework_file
was migrated ontoimported_library
this PR completes the migration and is followup to the Bazel 6 work.The related rules_apple change which updated
static_framework_file
: bazelbuild/rules_apple@8d84134