Add -F flag for xcframework dependency #7998
Conversation
Kyle-Ye
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
francescomikulis,
xedin,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand and
owenv
as code owners
|
@swift-ci please test |
The detail can be found on the issue link. If you do not mind duplication, I can re-summary the issue we encounter in this PR too. (Update: Done)
Close the issue linked above.
Yes. We need to add some test case to prevent future regressions before merging it. I'm struggling with the test infra codebase here and wondering is it acceptable to add a small simple xcframework binary file into the codebase. |
|
This does need a test, but I also have no idea where it would go. I am curious, do xcframeworks need -I flags at all and should they use -F exclusively? |
|
Build planning has its own |
Yeah. I have tested it by setting working directory and arguments in Xcode Scheme Editor. 
The things I'm working on:
// Current plan: PackagePluginAPITests/PackageManagerProxyTests.swift
import PackagePlugin
import XCTest
import _InternalTestSupport
final class PackageManagerProxyTests: XCTestCase {
func testBinarySwiftTargetSymbolGraph() async throws {
struct SymbolGraphPlugin: CommandPlugin {
func performCommand(context: PackagePlugin.PluginContext, arguments: [String]) async throws {
for target in context.package.targets {
let result = try packageManager.getSymbolGraph(for: target, options: .init())
let url = result.directoryPath
...
}
}
}
try fixture(name: "BinaryTarget") { path in
// Construct a package which has a binary dependency and perform the plugin
let package = ...
let context: PluginContext = ...
let plugin = SymbolGraphPlugin()
plugin.performCommand(context: context, arguments: [])
...
}
}
}
I guess I'll end up using a pure small xcframework build in my local without package source or a remote one with checksum in the end. (Any recommendation here? eg. Apple's official xcframework example will be great. But I can't find one) |
Close #7580 Replace -I with -F is enough to fix the issue. Keeping -I for compatibility reason in case there are other cases where -I is needed.
Kyle-Ye
force-pushed
the
bugfix/kyle/xcframework_symbol_graph
branch
from
0cbb6ef
to
68c6b28
Compare
Fixtures/Miscellaneous/Plugins/DocCForBinaryDependency/Package.swift
Outdated
Show resolved
Hide resolved
|
@swift-ci please test |
Fixtures/Miscellaneous/Plugins/DocCForBinaryDependency/Package.swift
Outdated
Show resolved
Hide resolved
|
@swift-ci test |
|
@swift-ci test |
Kyle-Ye
force-pushed
the
bugfix/kyle/xcframework_symbol_graph
branch
from
6aa51d8
to
502bc8e
Compare
|
@swift-ci test |
Tried to create it via Xcode 15.0.1 / Xcode 15.1 / Xcode 15.1 Beta 2 and modifying the existing project file. They all end up with a strange crash which I can't reproduce locally. (Even with the same Xcode version - 15.1b2) error: unexpected service error: The Xcode build system has crashed. Build again to continue.There is already one xcframework zip in the repo. But I can't use it since it only contains a x86_64 one - I suggest we either bump the current Xcode 15.1b2 on CI to any release version of Xcode / just use a xcframework zip (like the existing SwiftFramework.zip) / keep it as it is but mark it as skip like the previous binary test case. @MaxDesiatov Or I'd be appreciated if you have other suggestions which may help push the PR to the next level. . |
Workaround the non-reproduciable CI xcodebuild crash
Kyle-Ye
force-pushed
the
bugfix/kyle/xcframework_symbol_graph
branch
from
478babc
to
b3aa099
Compare
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
Remove xcodeproj and use pure Swift package to generate xcframework still fails on |
Maybe -I is needed for the header search I guess. But anyway I suggest we just keep it for compatibility. @rauhul |
I think I'm missing how this has been determined, is there a message above about how the build fails without the flag? My goal here is so minimize the CLI flags because it makes debugging SwiftPM builds much easier. |
Not very familiar with such flags. So I can't confidently remove it. But I tried to remove 
|
|
I have a couple of thoughts here:
To that end, I think we should remove the code that adds and modify https://github.com/swiftlang/swift-package-manager/blob/main/Sources/Build/BuildDescription/SwiftModuleBuildDescription.swift#L522-L525 to emit
|
Got it. And huge thanks for the point out. |
d57ee08 is what I got. Correct me if I mistake something. for library in libraries {
library.headersPaths.forEach {
- swiftTarget.additionalFlags += ["-I", $0.pathString, "-Xcc", "-I", "-Xcc", $0.pathString]
+ swiftTarget.libraryBinaryPaths.insert($0)
}
swiftTarget.libraryBinaryPaths.insert(library.libraryPath)
}And it will be the following using the DemoKit.zip provided by the issue. |
|
The line that adds |
| @@ -43,7 +43,7 @@ extension BuildPlan { | |||
| let libraries = try self.parseXCFramework(for: target, triple: swiftTarget.buildParameters.triple) | |||
| for library in libraries { | |||
| library.headersPaths.forEach { | |||
| swiftTarget.additionalFlags += ["-I", $0.pathString, "-Xcc", "-I", "-Xcc", $0.pathString] | |||
| swiftTarget.libraryBinaryPaths.insert($0) | |||
There was a problem hiding this comment.
Do you mean Line 46 or Line 48 here? @xedin
Considering the po result, I guess you mean Line 46. (Personally I do not get the reason to add it here.) But if I remove the line, it would conflict with your previous comment/suggestion here.
To that end, I think we should remove the code that adds -I and -Xcc -I from BuildPlan+Swift.swift and replace it with
library.headersPaths.forEach {
swiftTarget.libraryBinaryPaths.insert($0)
}
There was a problem hiding this comment.
I mean swiftTarget.libraryBinaryPaths.insert(library.libraryPath) on line 48, libraryPath doesn't actually point to the place that -F would recognize anyway.
There was a problem hiding this comment.
This is weird. I do not know what "-F" should recognize.
But according to the testDocCPluginForBinaryDependency test case, deleting Line 46 is fine while deleting Line 48 would make the test case fail with "No such module FooKit".
Saying that, I do think it is valuable we add real xcframework test case (via a trusted local one or fix the CI to build it on the fly). The simple flags check may be doubtful here.
There is other reviewer have a +1 for the real test case.
https://github.com/swiftlang/swift-package-manager/pull/7998/files?w=1#r1822483995
| @@ -521,7 +521,10 @@ public final class SwiftModuleBuildDescription { | |||
|
|
|||
| // Only add the build path to the framework search path if there are binary frameworks to link against. | |||
| if !self.libraryBinaryPaths.isEmpty { | |||
| args += ["-F", self.buildParameters.buildPath.pathString] | |||
| args += [ | |||
There was a problem hiding this comment.
question: Does this if block and the one here always need to match?
- If so (this comment becomes blocking), can we refactor by moving the common code into its own function so that updating in one place will automatically update all usage?
- if not, from my knowledge, can you disciple the use cases where they would differ?
There was a problem hiding this comment.
I think we should. I'll refactor it to a small helper function here.
There was a problem hiding this comment.
Hi.
I provided comments using Conventional Comments.
I'm happy to have a discussion if my comments are invalid.
Close #7580
Motivation:
If a target has a dependency on binaryTarget. Then the call of
PackageManager.getSymbolGraphto that target will get a directory contains no symbol graph.It will break
PackageManager.getSymbolGraph's client such as swift-docc-plugin.The downstream user can generate doc using
swift buildand calldocc convertto generate documentation. But if they migrate to swift-docc-plugin or swift-docc-plugin based service like SwiftPackageIndex, the documentation build will fail.The underlying issue is that xcframework dependencies are being passed to swift-symbolgraph-extract with -I paths, rather than -F paths. (Thanks @QuietMisdreavus)
Modifications:
Adding -F related additionalFlag for binary swift target.