Skip to content
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

Only update_in_place if arm64 sim slice is not present #769

Merged

Conversation

thiagohmcruz
Copy link
Contributor

@thiagohmcruz thiagohmcruz commented Sep 21, 2023

The import_middleman code path ultimately leads to the update_in_place script that adjusts deps for Apple Silicon. That feature is not necessary if the xcframework already contains a arm64 + sim slice.

@thiagohmcruz thiagohmcruz force-pushed the thiago/do-not-update-in-place-if-arm64-sim-slice-present branch from e624b7b to 33623ad Compare September 21, 2023 17:27
@thiagohmcruz thiagohmcruz marked this pull request as ready for review September 21, 2023 17:52
deps += select({
"@build_bazel_rules_ios//:arm64_simulator_use_device_deps": [name + ".import_middleman"],
"//conditions:default": vendored_deps,
"//conditions:default": vendored_deps_non_arm64_sim,
Copy link
Contributor

@jerrymarino jerrymarino Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to have to pull this down and give it an in-depth review - I wonder if this is overlapping with this code:

alias_slice = arm64_simulator_slice if arm64_simulator_slice else arm64_ios_device_slice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that even if the correct alias is picked up in import_middleman is_sim_arm64 is still True making it run update_in_place

It's not clear to me how these two checks are supposed to work together and why I don't see the expected behaviour without the changes in this PR. Some help to clarify that would be great.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok let me followup with you later on this - I don't fully understand why we need this either

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use case: Framework A depends on xcframework B and we want to use SwiftUI Previews in A.

To achieve that we necessarily need link_dynamic = True in A (SwiftUI Previews requires dynamic linking). The issue is what to do with B.

What I've tried:

  1. Set link_dynamic = True in B. This will bundle B.framework but the symbols table for B.framework/B is empty instead of containing the correctly symbols for the current slice being used (the binary created under the import_middleman path is correct but it's not the one used to create B.framework/B). This makes SwiftUI Previews fail due to missing arm64 symbols. Had I found a fix for this I'm not sure if I'd still hit the issue described in 2. next.
  2. Leave link_dynamic = False in B when I realized 1. might not be necessary since A is the module launching previews in Xcode so as long as A and it's public symbols are being linked dynamically seems like previews work fine based on my testing. The issue here is that the update_in_place logic changes the signature causing SwiftUI Previews to crash:
Exception Type:  EXC_BAD_ACCESS (SIGKILL (Code Signature Invalid))

This PR only solves this partially as it will use the arm64 + sim if it exists, bypassing update_in_place. If the xcframework contains only a non-simulator arm64 slice then update_in_place will still run and crash SwiftUI Previews.

The intent is to unblock SwiftUI Previews for scenarios where arm64 + sim slices are present and investigate 1. and 2. in a follow up.

# TODO(jmarino)Perhaps it uses a import_middleman here
if len(vendored_deps):
import_middleman(name = name + ".import_middleman", deps = vendored_deps, tags = ["manual"])
vendored_deps_non_arm64_sim = [d for d in vendored_deps if not vendored_deps_contains_arm64_sim_slice.get(d, False)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was able to handle both xcframeworks and non deps at a time - let me see if we can preserve this

Copy link
Contributor

@jerrymarino jerrymarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine for now, we can circle back to it at a later time

@thiagohmcruz thiagohmcruz merged commit f9644fb into master Sep 26, 2023
8 checks passed
@thiagohmcruz thiagohmcruz deleted the thiago/do-not-update-in-place-if-arm64-sim-slice-present branch September 26, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants