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

Add a utility to build xcframeworks #759

Merged
merged 3 commits into from Dec 7, 2019
Merged

Add a utility to build xcframeworks #759

merged 3 commits into from Dec 7, 2019

Conversation

pepicrft
Copy link
Contributor

@pepicrft pepicrft commented Dec 7, 2019

Short description 📝

In order to cache the frameworks, we need to build .xcframeworks first that contain both the device and simulator's architectures.

Solution 📦

This PR adds a new utility, XCFrameworkBuilder, that given a project or workspace, and the target representation it generates an .xcframework in a temporary directory and returns its path.

Testing strategy ✅

I've decided to use an integration test instead and runs assertions on the generated .xcframework. That way if future versions of xcodebuild break the integration of this utility with the command the tests will fail and we'll catch it.

@pepicrft pepicrft requested review from andreacipriani and a team December 7, 2019 11:54
@pepicrft pepicrft self-assigned this Dec 7, 2019
@ghost ghost requested review from kwridan and marciniwanicki and removed request for a team December 7, 2019 11:54
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2019

@pepibumur your pull request is missing a changelog!

Tests/Fixtures/Frameworks/macOS/macOS.h Outdated Show resolved Hide resolved
Tests/Fixtures/Frameworks/tvOS/tvOS.h Outdated Show resolved Hide resolved
Tests/Fixtures/Frameworks/watchOS/watchOS.h Outdated Show resolved Hide resolved
@pepicrft pepicrft force-pushed the xcframeworks branch 3 times, most recently from fea52b5 to b693ec5 Compare December 7, 2019 13:22
@RomainBoulay
Copy link
Contributor

(np: typo in PR title)

@pepicrft pepicrft changed the title Add a uility to build xcframeworks Add a utility to build xcframeworks Dec 7, 2019
Copy link
Contributor

@RomainBoulay RomainBoulay left a comment

Choose a reason for hiding this comment

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

Despacito is only the song, these animals ship stuff so rápidamente!! 🔥

return isDynamicLink ?? false
}
let isDynamicLink = try? frameworkMetadataProvider.linking(precompiled: frameworkNode) == .dynamic
return isDynamicLink ?? false
Copy link
Contributor

Choose a reason for hiding this comment

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

do you really want to swallow potential exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Ideally we should throw an error here but we can do it as part of another PR.

@@ -10,7 +10,7 @@ jobs:
steps:
- uses: actions/checkout@v1
- name: GitHub Action for SwiftLint
uses: pepibumur/action-swiftlint@0d4afd006bb24e4525b5afcefd4ab5e2537193ac
uses: norio-nomura/action-swiftlint@3c67ce2e382be797d968883944140ffa0113f737
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

productName: String,
xcframeworkPath: AbsolutePath) -> [String] {
var command = ["xcrun", "xcodebuild", "-create-xcframework"]
command.append(contentsOf: ["-framework", deviceArchivePath.appending(RelativePath("Products/Library/Frameworks/\(productName).framework")).pathString])
Copy link
Contributor

Choose a reason for hiding this comment

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

Products/Library/Frameworks fixed paths are code smell to me, I guess that would break if the Xcode project configuration may define something custom here. Probably not a problem in 99% of cases but maybe you could offer a default + optional override input flag here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Products/Library/Frameworks fixed paths are code smell to me, I guess that would break if the Xcode project configuration may define something custom here.

Yeah, some other things might break too if Apple changes something in the API of their tools or the structure of the frameworks. To catch that we have acceptance & integration tests that should fail if that happens.

Probably not a problem in 99% of cases but maybe you could offer a default + optional override input flag here?

That would not change anything, would it? Instead of having the path defined in this line, it'd be defined a few lines above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote that cause I thought you can provide the product path in the build settings. But I may be wrong and anyway as you say, this can be solved later 👍

@pepicrft pepicrft merged commit 0b8ea58 into master Dec 7, 2019
@pepicrft pepicrft deleted the xcframeworks branch December 7, 2019 16:09
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.

None yet

3 participants