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

fix bazel7 sim_arm64 dynamic framework link error, pass dynamic framework info #915

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kuperxu
Copy link

@kuperxu kuperxu commented Sep 12, 2024

When the "apple.arm64_simulator_use_device_deps" feature is enabled, dynamic framework information is not passed to Bazel. As a result, the linker encounters errors when attempting to link dynamic frameworks.

…bazel,with feature apple.arm64_simulator_use_device_deps enable
@kuperxu kuperxu marked this pull request as draft September 12, 2024 12:35
@kuperxu kuperxu marked this pull request as ready for review September 12, 2024 12:35
@kuperxu
Copy link
Author

kuperxu commented Sep 12, 2024

hi @luispadron, Help review the code. plz

@luispadron
Copy link
Collaborator

I don't use this rule, but @thiagohmcruz @gyfelton can help here

@kuperxu
Copy link
Author

kuperxu commented Sep 14, 2024

@thiagohmcruz @gyfelton please help me with the review, I didn't add the reviewer permissions.

@gyfelton
Copy link
Contributor

kicking CI to build.
Am I right that the added @import statement was a red to green test case with your fix? 🙏

@gyfelton
Copy link
Contributor

seems that CI are failing with

# Execution platform: @local_config_platform//:host
tests/ios/in-tree-vendor-prebuilt-deps/dynamic/app/App/main.m:3:9: fatal error: module 'MobileFlow' not found
@import MobileFlow;
 ~~~~~~~^~~~~~~~~~

@kuperxu
Copy link
Author

kuperxu commented Sep 18, 2024

seems that CI are failing with

# Execution platform: @local_config_platform//:host
tests/ios/in-tree-vendor-prebuilt-deps/dynamic/app/App/main.m:3:9: fatal error: module 'MobileFlow' not found
@import MobileFlow;
 ~~~~~~~^~~~~~~~~~

The reason for this was a missing MobileFlow dependency, which I fixed in the latest commit

@gyfelton
Copy link
Contributor

@kuperxu thx sooo much for the contribution, may I ask about some basic info of the downstream repo that you are fixing for:
which bazel version?
is VFS feature also enabled? (beside arm64_simulator_use_device_deps feature)
which rules_swift/apple version you are using with?
do you have sandbox mode enabled build?
does it use latest HEAD of rules_ios?

This info is useful on planning our work and contribution to the repo.

@thiagohmcruz I don't find any usage of dynamic_framework_file in our downstream repo (otherwise we would also suffer from this issue) so I think this is a good change, WDYT?

@kuperxu
Copy link
Author

kuperxu commented Sep 19, 2024

downstream

hi @gyfelton , for your question:
I am using bazel 7.1.1
VFS feature is disable
In my project,i am using rules_swift 1.17.0 & rules_apple 3.3.0(The issue can also be reproduced in the rules_ios master branch)
sandbox mode is disabled in my project.
yeah, latest HEAD of rules_ios

@gyfelton
Copy link
Contributor

Thank you! I merged lastest master on your branch and rekicking the build so that we can land it when ready.

I am also testing my downstream repo with your change and I think it's a good change. Need some input from others though.

@kuperxu
Copy link
Author

kuperxu commented Sep 20, 2024

Thank you! I merged lastest master on your branch and rekicking the build so that we can land it when ready.

I am also testing my downstream repo with your change and I think it's a good change. Need some input from others though.

The MobileFlow framework had a dependency on MiSnapCore, which I fixed in a new commit. Please help review.

otool -L tests/ios/in-tree-vendor-prebuilt-deps/Pods/MobileFlow/Frameworks/MobileFlow.framework/MobileFlow | grep MiSnapCore                      
        @rpath/MiSnapCore.framework/MiSnapCore (compatibility version 1.0.0, current version 1.0.0)
        @rpath/MiSnapCore.framework/MiSnapCore (compatibility version 1.0.0, current version 1.0.0)

@kuperxu kuperxu requested a review from gyfelton October 8, 2024 08:05
@kuperxu
Copy link
Author

kuperxu commented Oct 8, 2024

hi @thiagohmcruz @gyfelton ,Can you help me review the code?

@gyfelton
Copy link
Contributor

gyfelton commented Oct 8, 2024

thank you for your contribution! Since you are on bazel 7 also I feel we should decide when to remove support for bazel 6 so we can save room for next bazel version bump (esp. since apple_common.objc provider is being gone in some minor version of bazel 7) @thiagohmcruz thoughts?

@kuperxu
Copy link
Author

kuperxu commented Oct 9, 2024

could you help me run the workflow to merge the code? @gyfelton
image

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.

4 participants