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

Split SwiftUI support into separate module #929

Merged
merged 5 commits into from Feb 4, 2020

Conversation

dhavalshreyas
Copy link
Contributor

While bumping up Xcode version within Square, some ObjC-only targets are bumping into some issues during linking.

Specific error we're seeing:

ld: warning: Could not find or use auto-linked library 'swiftCoreMIDI'
Undefined symbols for architecture x86_64:
  "_swift_getOpaqueTypeConformance", referenced from:
      associated type witness table accessor for Body : SwiftUI.View in WorkflowUI.WorkflowView<A, B> : SwiftUI.View in WorkflowUI in WorkflowUI(WorkflowView.o)
      associated type witness table accessor for Body : SwiftUI.View in WorkflowUI.(RootView in _03A9777E4AF05597CE6B37108BFDC731)<A> : SwiftUI.View in WorkflowUI in WorkflowUI(WorkflowView.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

While we can dig into finding out the root cause and fix it, I thought it's better to move SwiftUI support into a different module.

Copy link

@dnkoutso dnkoutso left a comment

Choose a reason for hiding this comment

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

LGTM.

Probably get one more approval from core maintainers also!

@@ -0,0 +1,366 @@
// !$*UTF8*$!
Copy link

Choose a reason for hiding this comment

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

Rather than checking in an xcodeproj, would using be pod gen work here to reduce the boilerplate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I've created a new issue to track this: #930

This one is similar to the other sample apps in the repo.

Copy link

@justinseanmartin justinseanmartin left a comment

Choose a reason for hiding this comment

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

No blockers from my perspective, just some style feedback.

@timdonnelly
Copy link
Collaborator

Is this a positive change for the library itself? Do we actually want this code to live in a separate module?

WorkflowSwiftUI.podspec Outdated Show resolved Hide resolved
@dhavalshreyas
Copy link
Contributor Author

Is this a positive change for the library itself? Do we actually want this code to live in a separate module?

@timdonnelly: Any reason it shouldn't? The way you've written it, it looks like there's no dependency on WorkflowUI at all. So we'll just need to use WorkflowSwiftUI instead of WorkflowUI for SwiftUI projects. I think this is pretty cool.

@timdonnelly
Copy link
Collaborator

Is this a positive change for the library itself? Do we actually want this code to live in a separate module?

@timdonnelly: Any reason it shouldn't? The way you've written it, it looks like there's no dependency on WorkflowUI at all. So we'll just need to use WorkflowSwiftUI instead of WorkflowUI for SwiftUI projects. I think this is pretty cool.

I don't have a strong reason that it shouldn't/couldn't at a quick glance (and the software would certainly work in either case) – but module boundaries are part of the projects's API, and I'd like to always make API decisions with the end consumer in mind. Even with UIKit+SwiftUI support, the library is pretty small – I intentionally added SwiftUI support within the same module to keep the structure simple: it's the library with the UI bindings. That could certainly change, and in that case it might make sense to list the UI architecture in each module name (WorkflowSwiftUI + WorkflowUIKit or something).

This seems to be a case where Square's internal build system is failing on a simple language feature (availability checks), and that bug is driving a design decision.

  • There is nothing clever going on in the code here. Availability checks are a common language feature, so failures here in the build system of a project that I was maintaining would make me very nervous.
  • Changing a module's structure to work around the issue is an implicit decision that some class of availability checking is now disallowed/unsupported within the codebase going forward.
  • A build system that blocks developers from using common language features seems like it will impair development on an ongoing basis (so again, I'd be pretty worried).

The actual changes look ok, and I'm totally ok merging. I just want to make sure that we do so with the compromises called out.

@dhavalshreyas
Copy link
Contributor Author

Had an offline chat with @timdonnelly.

TLDR; Even though the change itself is triggered by a build system issue, moving the SwiftUI support to a different library is still the right thing to do. The WorkflowSwiftUI system and the WorkflowUI system do not work well together. You can choose to use one or the other, but you can't mix them both. Considering this, having these two be two separate libraries makes more sense.

@dhavalshreyas dhavalshreyas merged commit ad12160 into master Feb 4, 2020
@dhavalshreyas dhavalshreyas deleted the dhaval/splitSwiftUI branch February 4, 2020 00:35
dhavalshreyas added a commit that referenced this pull request Feb 4, 2020
* release-v0.23.x:
  Finish release v0.23.1
  Correct version for WorkflowSwiftUI
  Release v0.23.1
  Split SwiftUI support into separate module (#929)
@zach-klippenstein zach-klippenstein added this to In progress in Workflow (Swift) via automation Feb 4, 2020
@kyleve
Copy link

kyleve commented Apr 15, 2020

I believe we can resolve the underlying issue here by updating the swiftUI check to be:

#if DEBUG && canImport(SwiftUI) && !arch(i386)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Workflow (Swift)
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants