Check if schemes are buildable with projects, not workspaces

[mdiep]

Fixes #2369.

This broke in #2344. The check for whether a scheme should be built needs to occur against the project it's from, not against the workspace that includes it.

cc @jdhealy @allenhumphreys

[jdhealy]

Found another regression (see issue #2373) that I believe to be from #2344.

Larger, structural changes (away from what was implemented in #2344) might be necessary, so I’d vouch for holding off on this particular commit, and incorporate the ideas into whatever is needed to fix issue #2373.

[allenhumphreys]

For what it's worth. This change does fix my original problem in #2369.

[mdiep]

Updated to fix #2373 too.

[jdhealy]

Okay, I think I found the disconnect between #2344 and the code that preceded it —

Here’s a bug: if there's an shared scheme Example building a macOS app Example.app, the code:

Carthage/Source/CarthageKit/Xcode.swift

Lines 142 to 160 in 26fb61a

	return locator
	// This scheduler hop is required to avoid disallowed recursive signals.
	// See https://github.com/ReactiveCocoa/ReactiveCocoa/pull/2042.
	.start(on: QueueScheduler(qos: .default, name: "org.carthage.CarthageKit.Xcode.buildInDirectory"))
	// Pick up the first workspace which can build the scheme.
	.filter { project, schemes in
	switch project {
	case .workspace where schemes.contains(scheme):
	return true
	default:
	return false
	}
	}
	// If there is no appropriate workspace, use the project in
	// which the scheme is defined instead.
	.concat(value: (project, []))
	.take(first: 1)
	.map { project, _ in (scheme, project) }

…will eschew any actually buildable-by-carthage Example schemes in favor of passing only the scheme for the macOS app to

Carthage/Source/CarthageKit/Xcode.swift

Lines 162 to 166 in 26fb61a

	.flatMap(.concurrent(limit: 4)) { (scheme: Scheme, project: ProjectLocator) -> SignalProducer<(Scheme, ProjectLocator), CarthageError> in
	let buildArguments = BuildArguments(project: project, scheme: scheme, configuration: configuration)
	return shouldBuildScheme(buildArguments, platforms)
	.filter { $0 }
	.map { _ in (scheme, project) }

---

To a larger point, the following probably holds false-by-convention for most repos, but would cause enough failures to be non-optimal for us: scheme names don’t hold universal·meaning/non-duplication across the boundaries of different xcodeproj/xcworkspace.

We avoided this issue in the past, largely, by potentially-duplicated scheme names being treated as identifiers only for the subset of shared schemes that carthage deemed buildable.

[mdiep]

I don't see how the logic here in #2372 is different from the behavior before #2344.

In both cases:

1. We only built shared schemes that built a framework in the container that holds them
2. We build schemes from workspaces if they're contained in one

#2344 broke (1) by checking build ability against the workspace instead of the container, but this PR fixes it. So I think this fix restores the previous behavior while retaining the potential performance benefits of #2344.

Am I missing something @jdhealy?

[jdhealy]

Unlike when locator was decidedly producing deemed-buildable-by-carthage schemes:

Carthage/Source/CarthageKit/Xcode.swift

Line 779 in 19e5846

let locator = buildableSchemesInDirectory(directoryURL, withConfiguration: options.configuration, forPlatforms: options.platforms)

…the .take(first: 1) in #2372 or #2344 can grab a single deemed-unbuildable-by-carthage scheme with no recourse (where previously recourse wasn’t needed because the deemed-buildable-by-carthage determination was done beforehand by locator).

[mdiep]

I'm not seeing how that matters.

The .take(first: 1) existed before too. It's inside a .flatMap(.merge) { scheme, project in both, so it's only effect is to find one definitive place to build a given scheme that's shared from a given project. That outcome seems to same to me here and and before #2344 (but is different without this PR).

[jdhealy]

given scheme that's shared from a given project

The .take(first: 1) existed before too

Yeah, difference being: what’s given to the .take(first: 1) originated from a locator that previously (differently than #2372 or #2344) was filtering out schemes deemed-nonbuildable-by-carthage beforehand.

[mdiep]

Okay, let's try this again. 😅

@jdhealy provided a very helpful reproduction of the issue he described. 👏
WorkspaceWithDependency·ModifiedToTest«b395568».zip

[mdiep]

bump

[jdhealy]

Know you just relocated this from elsewhere, but this could be .filterMap { $0 ? project : nil }, if that sits right with you.

[jdhealy]

Less readably intuitive, but more performant (potentially ↜ untested), might be:

.lift {
	func noSharedFrameworkSchemesError(_ directoryURL: URL) -> CarthageError {
		return .noSharedFrameworkSchemes(.git(GitURL(directoryURL.path)), platforms)
	}

	return Signal.merge( // merge in error iff no values are sent
		$0,
		$0.materialize()
			.reduce(into: false) { if case .value = $1 { $0 = true } }
			.flatMap(.race) {
				$0 ? SignalProducer.empty : .init(error: noSharedFrameworkSchemesError(directoryURL))
			}
	)
}

[jdhealy]

Two comments, if you’d like to run with them @mdiep…

…larger point: the logic is sound and this is good to merge with or without those minor comments being addressed! 🎉

[mdiep]

I'd say let's just merge so we can release. ☺️

[jdhealy]