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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make watch targets runnable to fix schemes in Xcode 12 #2096

Merged
merged 1 commit into from
Dec 3, 2020

Conversation

thedavidharris
Copy link
Collaborator

Short description 馃摑

Watch schemes and watch extension schemes were not buildable as generated in Xcode 12

Solution 馃摝

This marks these targets as "runnable" which properly generates the BuildableProductRunnable instead of MacroExpansion portion of the target

I'm admittedly unsure if there are any other side effects of this. WatchExtenstion targets can't be "run" standalone still, but this seems necessary and mirrors the Xcode defaults from what I can tell.

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @thedavidharris!

@@ -111,7 +111,7 @@ public enum Product: String, CustomStringConvertible, CaseIterable, Encodable {
/// Returns true if the target can be ran.
public var runnable: Bool {
switch self {
case .app, .appClip, .commandLineTool:
case .app, .appClip, .commandLineTool, .watch2App, .watch2Extension:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it's only the watch app that is runnable, the extension is run within it - attempting to run the extension on its own will yield an error.

We have a fixture you can test this on fixtures/ios_app_with_watchapp2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is correct, but if you don't have the watch2Extension as runnable, you can't even build it targeting the watch simulator, so I think you have to mark it as runnable which is what XCode seems to do if you just create a watch app directly there, although I might need some help verifying that someone else is also seeing this. This is the one that confused me.

Copy link
Collaborator Author

@thedavidharris thedavidharris Dec 1, 2020

Choose a reason for hiding this comment

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

Yeah did check on it, if you just create a new extension and add the extension scheme, this is what you get. It's not actually runnable in the sense you could attach it, but it needs this to build. Otherwise with the MacroExpansion it shows "My Mac" as the target and totally freezes Xcode.

That said, it does get a little weird with the schemes list, and does adjust the -Project schemes (cc @fortmarek) as this will now start to clearly show -iOS and -watchOS schemes. I think this is more correct in totality.

In general it looks like there are some overall differences between the way Tuist is generating schemes and the way they are being generated if you just add the project to Xcode for watch targets, but watch targets are weird and painful so will take me a bit to go through them.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the heads-up @thedavidharris! I'll make sure to add a watchOS app to the fixture the project schemes are tested on, so we can be sure everything works correctly (and fix it if it doesn't currently)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fortmarek if you check out this PR and just look at the fixtures/ios_app_with_watchapp2 from before or after you should see the difference.

watchOS doesn't have XCTest, so overall not too big of a deal, but it does generate it "correctly" now since the scheme is parsable by XCode again

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah missed that bit, thanks for clarifying @thedavidharris

I found a solution to this, instead of marking the extensions as runnable we can update the AutogeneratedSchemeProjectMapper to provide the watch app as the runnable for the watch extension. There is already similar logic to deal with this for other extensions.

  • runnableExecutable can be updated to include a new case for .watch2Extension which would search for a host watch app to return
     private func hostWatchAppTargetReferences(for target: Target,
                                         project: Project) -> [TargetReference]
    {
        project.targets
            .filter { $0.product == .watch2App && $0.dependencies.contains(.target(name: target.name)) }
            .sorted(by: { $0.name < $1.name })
            .map { TargetReference(projectPath: project.path, name: $0.name) }
    }

There is an existing method to search for hostApps but that seems to be geared towards searching for test hosts which watch apps don't qualify for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah awesome: that seems to line up with Xcode defaults too, where it doesn't really create the xcscheme. You can add it in their GUI but then it doesn't work, gives you that error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kwridan done, thanks for the heads up!

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

@pepicrft pepicrft merged commit d6fcc68 into tuist:main Dec 3, 2020
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

4 participants