Binary dependencies of C++ module break build planning

[furby-tm]

Fix a regression where the build planning phase will early terminate if SwiftPM packages contain binary targets that C++ products refer to, #8055.

Motivation:

When running swift build on SwiftPM packages with binary targets that C++ products refer to, they will early terminate during the build planning phase with the following error:

error: InternalError(description: "Internal error. Please file a bug at https://github.com/swiftlang/swift-package-manager/issues with this info. unknown module: <ResolvedModule: Python, binary>")

Modifications:

Remove the guard, so we do not fail if the module dependency does not yet exist in the build plan's target map, which I think is normal on binary targets that C++ products refer to, since they don't have build descriptions.

Result:

When running swift build on SwiftPM packages with binary targets that C++ products refer to, the build planning phase will proceed successfully without error.

[MaxDesiatov]

What does "large packages" mean here (what exactly would make a package large in this case?) and how that and binary artifacts are related to changed lines of code?

But first of all, this needs a self-contained test case added to the test suite, so that we can verify that the change actually fixes the issue and doesn't regress with future changes.

[furby-tm]

@MaxDesiatov Sorry, I inferred from the issue linked above #8055 should give you more insight.

What does "large packages" mean here (what exactly would make a package large in this case?)

A cross-platform package manifest of roughly ~2000 lines of code and ~40 targets.

This is all you have to do to replicate binary targets not resolving in the main branch of swiftpm:

git clone git@github.com:wabiverse/MetaverseKit.git
cd MetaverseKit

swift build

error: InternalError(description: "Internal error. Please file a bug at https://github.com/swiftlang/swift-package-manager/issues with this info. unknown module: <ResolvedModule: Python, binary>")

If you try to build it again with this revision, it no longer fails here, resolving what appears to be a needless early-out at the guard, as far as I can tell.

and how that and binary artifacts are related to changed lines of code?

The guard early exits swiftpm, because the binary target: Python in this case was not present in the BuildPlan's targetMap property, which is a IdentifiableSet<ModuleBuildDescription>, failing to find the binary target at that invocation causes swiftpm to terminate at the guard before the build commences, I believe it should be harmless to not early-out here, but you would know better than I do if that appears to be problematic.

But first of all, this needs a self-contained test case added to the test suite, so that we can verify that the change actually fixes the issue and doesn't regress with future changes.

I can attempt to re-create a pretty large dependency cycle as seen in the MetaverseKit SwiftPM package, if that's something suitable to be added to the test suite, this is the only way I was able to reproduce this issue.

[furby-tm]

My underlying assumption here is that it is considered normal for some modules to not be present in BuildPlan's targetMap: Set<ModuleBuildDescription> at the time of invocation.

Also for these reasons, this is why I decided to take the approach I did when creating this PR:

1. This PR retains the previous (no guard) behavior of SwiftPM before this revision (added a guard) on Aug 15th.

2. Also of note, this PR now matches the same (no guard) logic in L.485 LLBuildManifestBuilder+Swift.swift.

3. Given the optional parameter in addStaticTargetInputs(_ description: ModuleBuildDescription?).

[furby-tm]

@MaxDesiatov good news, I was able to minimally reproduce this, it isn't related to a large dependency cycle.

This issue is not related to Swift targets, you can only reproduce this regression in C++ targets, the underlying issue is that binary target dependencies specified within C++ targets do not resolve without this PR.

git clone git@github.com:wabiverse/SwiftPMBinaryTargetRegression.git
cd SwiftPMBinaryTargetRegression

swift build

If you comment out #1 and #2, swift build...and toggle them back in...swift build, you can reproduce this issue. I've tested against this PR, and it appears it fixes binary targets to be resolved from C++ targets.

[furby-tm]

@MaxDesiatov

I have added a reproducible test to the existing XCFramework fixture, one final thing to note, I realized that this regression only affects binary target dependencies brought in from external package dependencies, within C++ targets.

first run is this PR, second run is SwiftPM main.

[MaxDesiatov]

With a new test fixture added this needs a new test case function exercising it, which would fail on main, but pass with this PR.

[furby-tm]

Sorry, I'm still quite new to the swift-package-manager codebase and its respective test cases.

I see the XCFramework test fixture utilized in these 4 places:

• swift-package-manager/Tests/CommandsTests/PackageCommandTests.swift

  Line 115 in 4095b90

  try await fixture(name: "DependencyResolution/External/XCFramework") { fixturePath in

• swift-package-manager/Tests/CommandsTests/PackageCommandTests.swift

  Line 132 in 4095b90

  try await fixture(name: "DependencyResolution/External/XCFramework") { fixturePath in

• swift-package-manager/Tests/CommandsTests/SwiftCommandStateTests.swift

  Line 168 in 4095b90

  try fixture(name: "DependencyResolution/External/XCFramework") { fixturePath in

• swift-package-manager/Tests/CommandsTests/SwiftCommandStateTests.swift

  Line 203 in 4095b90

  try fixture(name: "DependencyResolution/External/XCFramework") { fixturePath in

For which of the existing tests do you think is a good place to add a new test case function to exercise XCFrameworkExternal?

[MaxDesiatov]

It depends on what you need to have access to and what exactly the fix covers. SwiftCommandStateTests is almost exclusively for testing the SwiftCommandState API. PackageCommandTests is for testing swift package CLI invocations. I don't think your fixture belongs to either, but then the nature of your fix is unclear to me in general, in your "a needless early-out at the guard" explanation, what exactly makes you think it's needless?

The title of the PR says "fix ... not resolving", but then your change is in the build planning phase and not in the modules resolution phase. You'd have to pinpoint the exact logic you're changing and test that bit specifically.

[furby-tm]

The title of the PR says "fix ... not resolving", but then your change is in the build planning phase and not in the modules resolution phase. You'd have to pinpoint the exact logic you're changing and test that bit specifically

@MaxDesiatov I have updated the title to reflect the purpose of this PR.

I don't think your fixture belongs to either, but then the nature of your fix is unclear to me in general, in your "a needless early-out at the guard" explanation, what exactly makes you think it's needless?

For these reasons I mentioned previously:

1. This PR retains the previous (no guard) behavior of SwiftPM before this revision (added a guard) on Aug 15th.

2. Also of note, this PR now matches the same (no guard) logic in L.485 LLBuildManifestBuilder+Swift.swift.

3. Given the optional parameter in addStaticTargetInputs(_ description: ModuleBuildDescription?).

---

The removal of the guard is not new behavior (the opposite is true - the addition of the guard is a recent revision which led to this error), it is to match the current behavior in point 2, and maintain the previous behavior of SwiftPM in point 1, when binary artifact dependencies in C++ targets did not break (early terminate) the build planning phase.

[MaxDesiatov]

Do we need a fixture at all then? Would something that replicates tests in https://github.com/swiftlang/swift-package-manager/blob/main/Tests/BuildTests/ClangTargetBuildDescriptionTests.swift make more sense? Bear in mind that fixtures are quite heavy in terms of amount of time they add to test runs. We should avoid creating fixtures unless they're the only way to replicate buggy behavior.

[furby-tm]

That's more than fine with me, whatever is the preferred approach here that still allows the bug to be minimally reproducible.

Just to confirm, would it be preferable to add a new test case to replicate this bug in https://github.com/swiftlang/swift-package-manager/blob/main/Tests/BuildTests/ClangTargetBuildDescriptionTests.swift, or would it be better to setup a whole new file for this test under BuildTests?

[MaxDesiatov]

That file is not too big, so adding a new test function to it is fine by me. Thanks!

[furby-tm]

As an aside, forgive my confusion here a bit but after running into a:

failed: caught error: "The package does not contain a buildable target.
Add at least one `.target` or `.executableTarget` to your `Package.swift`."

error while attempting to re-create a SwiftPM package in this completely different approach to the current SwiftPM Package Manifest API (using internal APIs and things that don't reflect a "consistent with the available API" approach of how any package manifests are authored), I find it odd that we are creating tests in this way.

I'm sure I will fix this error and get a test case that eventually works, I'm not too worried about that. Most of these tests try to replicate some tiny fragment of the API, like a ClangModule with a source file that doesn't exist, and then shoe-horning that into a method intended to create a valid build plan, seems very odd at least on the outset.

Nearly any swift engineer can create Swift Package Manifests all day, writing these tests however, it's quite a bit more prickly.

[xedin]

Just came here to say that the fix is correct, there shouldn't have been guard in there in the first place because binary targets that C++ products refer to don't have build descriptions and addStaticTargetInputs is expected to handle only swift library targets. Might be worth it to rename addStaticTargetInputs into something like addStaticSwiftLibraryInputs or something similar.

[xedin]

@furby-tm and @MaxDesiatov I think all we need to test this issue is to form a BuildPlan, build and analyze the compile command, we don't actually need fixtures here.

[furby-tm]

Just came here to say that the fix is correct, there shouldn't have been guard in there in the first place because binary targets that C++ products refer to don't have build descriptions and addStaticTargetInputs is expected to handle only swift library targets. Might be worth it to rename addStaticTargetInputs into something like addStaticSwiftLibraryInputs or something similar.

Thanks for the clarification! That makes much more sense than my earlier attempt to explain:

“Remove the guard, so we do not fail if the module dependency does not yet exist in the build plan’s target map, which I think is normal.”

In retrospect, that doesn’t make sense at all. Please disregard my earlier comments; I’m still learning the nuances of SwiftPM and familiarizing myself with the codebase, feel free to write me off as the crazy new guy lol. 😅

@MaxDesiatov, I will remove the new fixture I added and include the test case we discussed. I apologize for my chaotic explanation, which understandably caused some confusion. I appreciate your willingness to help me with this PR, even if you probably felt like throwing your MacBook at me in the process! 😭🙏

[furby-tm]

I offered some general feedback on SwiftPM's current approach of creating many of these tests here, if helpful.