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.

Sign up for GitHub

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

Jump to bottom

Fix static xcframework import identifier picking issues #1579

Merged
merged 4 commits into from
Jul 15, 2022
Merged

Fix static xcframework import identifier picking issues #1579

merged 4 commits into from
Jul 15, 2022

Conversation

keith
Copy link
Member

@keith keith commented Jul 15, 2022 •
edited
Loading

Fix xcframework incorrect library identifier picking. There are a few issues being fixed here:

  • armv7 can't be included in device tests unless we want to include it in xcframework fixtures, I just removed that since 32 bit usage should be low, and was also partially removed upstream
  • added ios_cpu to the default transition, sometimes we get this set from bazel itself, so in the case you have a dependency tree where an ios_application built for device transitively depends on a apple_static_xcframework, you ended up with conflicting actions because of the minor configuration difference. Theoretically people mostly shouldn't have this, but if you want to test a real xcframework that's produced, then you hit this case
  • fix library identification where previously it was too aggressive and over including simulator files for device

@keith keith mentioned this pull request Jul 15, 2022
Closed
@keith keith force-pushed the ks/add-static-xcframework-import-test-for-device branch from 40f7be2 to e36bba5 Compare July 15, 2022 18:32
@keith keith changed the title Add static xcframework import test for device Fix static xcframework import identifier picking issues Jul 15, 2022
@keith keith force-pushed the ks/add-static-xcframework-import-test-for-device branch from e36bba5 to 366c561 Compare July 15, 2022 18:40
@keith
Fix limiting files for correct library identifier
3c1ac7f 
`ios-arm64_x86_64-simulator` contains `ios-arm64`, so these were
matching too aggressively. Since we pull the current identifier from the
actual paths, we can be more strict since we don't need to handle
ordering differences in architectures.
@keith keith marked this pull request as ready for review July 15, 2022 18:54
@keith keith enabled auto-merge (squash) July 15, 2022 18:54
@keith keith merged commit f2e0645 into master Jul 15, 2022
@keith keith deleted the ks/add-static-xcframework-import-test-for-device branch July 15, 2022 18:55
copybara-service bot pushed a commit that referenced this pull request Aug 31, 2022
@stravinskii @swiple-rules-gardener
Fix library identifier matching for XCFrameworks import rules
717d4b4 
Current logic for figuring out an XCFramework library identifier
from file paths fails to match correctly for arm64 device builds,
choosing a simulator library due to how an XCFramework library
identifier for a device build is a substring for a simulator build:

  - Device build: `ios-arm64`
  - Simulator build: `ios-arm64-simulator`

This change updates current logic to match a library identifier from
file paths using the effective target triplet, as well as matching
XCFramework files using that library identifier from file paths.

Additionally, it adds extra handling logic to avoid selecting an
`arm64e` or `arm64_32` library when targeting `arm64` due to
substring matching.

Based on PR #1579 by keith

PiperOrigin-RevId: 471303360
keith pushed a commit that referenced this pull request Aug 14, 2023
@stravinskii @keith
Fix library identifier matching for XCFrameworks import rules
246a15e 
Current logic for figuring out an XCFramework library identifier
from file paths fails to match correctly for arm64 device builds,
choosing a simulator library due to how an XCFramework library
identifier for a device build is a substring for a simulator build:

  - Device build: `ios-arm64`
  - Simulator build: `ios-arm64-simulator`

This change updates current logic to match a library identifier from
file paths using the effective target triplet, as well as matching
XCFramework files using that library identifier from file paths.

Additionally, it adds extra handling logic to avoid selecting an
`arm64e` or `arm64_32` library when targeting `arm64` due to
substring matching.

Based on PR #1579 by keith

PiperOrigin-RevId: 471303360
(cherry picked from commit 717d4b4)
@keith keith mentioned this pull request Aug 14, 2023
Merged
keith added a commit that referenced this pull request Aug 14, 2023
@keith @stravinskii
Fix library identifier matching for XCFrameworks import rules (#2055)
27f5f67 
Current logic for figuring out an XCFramework library identifier
from file paths fails to match correctly for arm64 device builds,
choosing a simulator library due to how an XCFramework library
identifier for a device build is a substring for a simulator build:

  - Device build: `ios-arm64`
  - Simulator build: `ios-arm64-simulator`

This change updates current logic to match a library identifier from
file paths using the effective target triplet, as well as matching
XCFramework files using that library identifier from file paths.

Additionally, it adds extra handling logic to avoid selecting an
`arm64e` or `arm64_32` library when targeting `arm64` due to
substring matching.

Based on PR #1579 by keith

PiperOrigin-RevId: 471303360
(cherry picked from commit 717d4b4)

---------

Co-authored-by: Mauricio Garcia <mauriciogs@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brentleyjones brentleyjones brentleyjones approved these changes

@BalestraPatrick BalestraPatrick Awaiting requested review from BalestraPatrick BalestraPatrick is a code owner

@segiddins segiddins Awaiting requested review from segiddins

@thii thii Awaiting requested review from thii thii is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@keith @brentleyjones