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

Add -F flag for xcframework dependency #7998

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// swift-tools-version: 5.8

import PackageDescription

let package = Package(
name: "FooKit",
products: [
.library(name: "FooKit", type: .dynamic, targets: ["FooKit"]),
],
targets: [
.target(name: "FooKit"),
]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
#!/bin/bash
##===----------------------------------------------------------------------===##
##
## This source file is part of the Swift open source project
##
## Copyright (c) 2024 Apple Inc. and the Swift project authors
## Licensed under Apache License v2.0 with Runtime Library Exception
##
## See http://swift.org/LICENSE.txt for license information
## See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
##
##===----------------------------------------------------------------------===##

set -e

SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd -P)"
PROJECT_ROOT="$(dirname "$SCRIPT_DIR")"

cd $PROJECT_ROOT

PROJECT_BUILD_DIR="${PROJECT_BUILD_DIR:-"${PROJECT_ROOT}/.build"}"
XCODEBUILD_BUILD_DIR="$PROJECT_BUILD_DIR/xcodebuild"
XCODEBUILD_DERIVED_DATA_PATH="$XCODEBUILD_BUILD_DIR/DerivedData"

PACKAGE_NAME=$1
if [ -z "$PACKAGE_NAME" ]; then
echo "No package name provided. Using the first scheme found in the Package.swift."
PACKAGE_NAME=$(xcodebuild -list | awk 'schemes && NF>0 { print $1; exit } /Schemes:$/ { schemes = 1 }')
echo "Using: $PACKAGE_NAME"
fi

build_framework() {
local sdk="$1"
local destination="$2"
local scheme="$3"

local XCODEBUILD_ARCHIVE_PATH="$PROJECT_BUILD_DIR/$scheme-$sdk.xcarchive"

rm -rf "$XCODEBUILD_ARCHIVE_PATH"

PROTOBUFKIT_LIBRARY_TYPE=dynamic xcodebuild archive \
-scheme $scheme \
-archivePath $XCODEBUILD_ARCHIVE_PATH \
-derivedDataPath "$XCODEBUILD_DERIVED_DATA_PATH" \
-sdk "$sdk" \
-destination "$destination" \
BUILD_LIBRARY_FOR_DISTRIBUTION=YES \
INSTALL_PATH='Library/Frameworks' \
OTHER_SWIFT_FLAGS=-no-verify-emitted-module-interface

if [ "$sdk" = "macosx" ]; then
FRAMEWORK_MODULES_PATH="$XCODEBUILD_ARCHIVE_PATH/Products/Library/Frameworks/$scheme.framework/Versions/Current/Modules"
mkdir -p "$FRAMEWORK_MODULES_PATH"
cp -r \
"$XCODEBUILD_DERIVED_DATA_PATH/Build/Intermediates.noindex/ArchiveIntermediates/$scheme/BuildProductsPath/Release/$scheme.swiftmodule" \
"$FRAMEWORK_MODULES_PATH/$scheme.swiftmodule"
rm -rf "$XCODEBUILD_ARCHIVE_PATH/Products/Library/Frameworks/$scheme.framework/Modules"
ln -s Versions/Current/Modules "$XCODEBUILD_ARCHIVE_PATH/Products/Library/Frameworks/$scheme.framework/Modules"
else
FRAMEWORK_MODULES_PATH="$XCODEBUILD_ARCHIVE_PATH/Products/Library/Frameworks/$scheme.framework/Modules"
mkdir -p "$FRAMEWORK_MODULES_PATH"
cp -r \
"$XCODEBUILD_DERIVED_DATA_PATH/Build/Intermediates.noindex/ArchiveIntermediates/$scheme/BuildProductsPath/Release-$sdk/$scheme.swiftmodule" \
"$FRAMEWORK_MODULES_PATH/$scheme.swiftmodule"
fi

# Delete private and package swiftinterface
rm -f "$FRAMEWORK_MODULES_PATH/$scheme.swiftmodule/*.package.swiftinterface"
rm -f "$FRAMEWORK_MODULES_PATH/$scheme.swiftmodule/*.private.swiftinterface"
}

build_framework "macosx" "generic/platform=macOS" "$PACKAGE_NAME"

echo "Builds completed successfully."

cd $PROJECT_BUILD_DIR

XCFRAMEWORK_DESTINATION="$PROJECT_ROOT/../$PACKAGE_NAME.xcframework"

rm -rf $XCFRAMEWORK_DESTINATION
xcodebuild -create-xcframework \
-framework $PACKAGE_NAME-macosx.xcarchive/Products/Library/Frameworks/$PACKAGE_NAME.framework \
-output $XCFRAMEWORK_DESTINATION
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/// print "foo"
public func foo() {
print("foo")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// swift-tools-version: 5.8
import PackageDescription

let package = Package(
name: "DemoKit",
products: [
.library(name: "DemoKit", targets: ["DemoKit"]),
],
targets: [
.plugin(
name: "GenerateSymbolGraphPlugin",
capability: .command(
intent: .custom(
verb: "generate-symbol-graph",
description: "Generate symbol graph for all Swift source targets."
)
)
),
.binaryTarget(name: "FooKit", path: "FooKit.xcframework"),
.target(
name: "DemoKit",
dependencies: ["FooKit"]
),
]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2024 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for Swift project authors

import Foundation
import PackagePlugin

@main
struct GenerateSymbolGraphPlugin: CommandPlugin {
func performCommand(context: PluginContext, arguments: [String]) throws {
for target in context.package.targets where target is SwiftSourceModuleTarget {
let _ = try packageManager.getSymbolGraph(for: target, options: .init())
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import FooKit

public func greeting(_ name: String) {
FooKit.foo()
print("Hello \(name)")
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 += [
Copy link
Contributor

@bkhouri bkhouri Oct 30, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should. I'll refactor it to a small helper function here.

"-F", self.buildParameters.buildPath.pathString,
"-Xcc", "-F", "-Xcc", self.buildParameters.buildPath.pathString
]
}

// Emit the ObjC compatibility header if enabled.
Expand Down Expand Up @@ -656,6 +659,14 @@ public final class SwiftModuleBuildDescription {
// FIXME: only pass paths to the actual dependencies of the module
// Include search paths for swift module dependencies.
args += ["-I", self.modulesPath.pathString]

// 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,
"-Xcc", "-F", "-Xcc", self.buildParameters.buildPath.pathString
]
}

// FIXME: Only include valid args
// This condition should instead only include args which are known to be
Expand Down
2 changes: 1 addition & 1 deletion Sources/Build/BuildPlan/BuildPlan+Swift.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

@Kyle-Ye Kyle-Ye Oct 29, 2024

Choose a reason for hiding this comment

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

#7998 (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)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean swiftTarget.libraryBinaryPaths.insert(library.libraryPath) on line 48, libraryPath doesn't actually point to the place that -F would recognize anyway.

Copy link
Contributor Author

@Kyle-Ye Kyle-Ye Nov 5, 2024

Choose a reason for hiding this comment

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

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

}
swiftTarget.libraryBinaryPaths.insert(library.libraryPath)
}
Expand Down
19 changes: 19 additions & 0 deletions Tests/BuildTests/PluginsBuildPlanTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,23 @@ final class PluginsBuildPlanTests: XCTestCase {
)
}
}

func testDocCPluginForBinaryDependency() async throws {
#if !os(macOS)
// binaryTarget/xcframework is only supported on Darwin platform
throw XCTSkip("Test requires macOS")
#endif

try await fixture(name: "Miscellaneous/Plugins/SymbolGraphForBinaryDependency") { fixturePath in
let result = try await AsyncProcess.popen(arguments: [
fixturePath.appending(RelativePath("FooKit/Scripts/archive_xcframework.sh")).pathString,
"FooKit"
])
try print(result.utf8Output())
try print(result.utf8stderrOutput())
XCTAssertEqual(result.exitStatus, .terminated(code: 0))
// Before we add -F support for xcframework, this call will throw since the command will abort with a non-zero exit code
let _ = try await executeSwiftPackage(fixturePath, extraArgs: ["generate-symbol-graph"])
Kyle-Ye marked this conversation as resolved.
Show resolved Hide resolved
}
}
}