Skip to content

Set OtherLDFlags for dynamic framework type #206

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

S-Shimotori
Copy link
Contributor

@S-Shimotori S-Shimotori commented Apr 14, 2025

Closes #116

This is a draft that contains FIXME and doesn't have required tests.
I created this PR to disclose codes given by ikesyo san. I will close this issue if anyone can implement completely.


Issue

According to #116, current Scipio cannot create dynamic frameworks for these libraries:

The linker needs dependencies with -framework flag.

Idea

PIFGenerator

  • Pass -framework to OTHER_LDFLAGS when frameworkType=dynamic
    • Thank you ikesyo san
    • I need to solve swiftlint warning

Tests

I think these tests are required but I'm sorry couldn't create them by myself yet.

framework type condition
static -
"Atomics": .init(frameworkType: .static),
mergeable - Current Scipio seems not to be able to handle "Atomics": .init(frameworkType: .mergeable)
dynamic - swift-atomics and swift-nio are this type. We can test this type with swift-atomics or swift-nio at least.
I tried to create a package that contains a simple internal target, but current Scipio can create XCFramework without -framework. Maybe we should prepare a special target (e.g. Clang module?)
[2025-05-01] It seems that the linker requires -framework for @usableFromInline and Clang module.
dynamic platform condition We need to make PIFLibraryTargetModifier internal for checking OTHER_LDFLAGS.
dynamic configuration condition not available yet (SE-0273)
dynamic trait condition SE-0450

I think "OtherLDFlagsTestPackage" is not a good name but I need to decide what tests we need before giving name.

@S-Shimotori
Copy link
Contributor Author

I used swift-atomics for test on cd10ffc, but I found Scipio fails with @usableFromInline and a C function.

(package)/
┣━━ Package.swift
┗━━ Sources/
    ┣━━ (Clang module target)/
    ┃   ┣━ include/
    ┃   ┃  ┣━ (C header file)
    ┃   ┃  ┗━ module.modulemap
    ┃   ┗━ src/
    ┃      ┗━ (C source file)
    ┗━━ (swift target)/
        ┗━ (Swift source file)
void this_is_c_function();
#include "ClangModule.h"

void this_is_c_function() {
}
import ClangModule

@usableFromInline
struct Sample {
    @usableFromInline
    static func sample() {
        this_is_c_function()
    }
}

Please see 6bf01ab for the concrete example.

@S-Shimotori
Copy link
Contributor Author

We can check which libraries created xcframeworks depends on with otool -l.

Load command 10
          cmd LC_LOAD_DYLIB
      cmdsize 96
         name @rpath/ClangModuleForMacOS.framework/Versions/A/ClangModuleForMacOS (offset 24)

I think we need to make PIFGenerator or PIFLibraryTargetModifier internal to confirm that they pass minimum required -framework flags for each platform.

@S-Shimotori
Copy link
Contributor Author

Summary

Cause of Issue

The linker cannot link Clang modules for @usableFromInline by itself.

Changes

PIFGenerator.swift

  • PIFLibraryTargetModifier will make lists of dependencies and pass them with -framework to OTHER_LDFLAGS.
  • I refactored PIFGenerator.swift to reduce the number of lines.
  • I left FIXME comment for trait condition. I'm sorry, but I would be happy if someone familiar with trait condition could implement it.

DynamicFrameworkTests.swift

  • DynamicFrameworkTests checks whether a generated xcframework contains minimum necessary dependencies.
    • Ideally, we should check the list of -frameworks in OTHER_LDFLAGS because the generated xcframework won't contain unnecessary dependencies even if Scipio passes additional modules.

IntegrationTests.swift

  • testDynamicFramework() checks whether Scipio can generate xcframeworks from a package that contains platform condition and @usableFromInline.

UsableFromInlinePackage, DynamicFrameworkOtherLDFlagsTestPackage

  • This package contains not only @usableFromInline but also platform condition (condition: .when(platforms:)) for coverage.

@S-Shimotori S-Shimotori marked this pull request as ready for review May 4, 2025 02:25
@giginet giginet requested review from giginet and ikesyo June 3, 2025 05:26
@giginet
Copy link
Owner

giginet commented Jun 3, 2025

@S-Shimotori Sorry for waiting you. It's ready to review. Could you rebase the current main?

@S-Shimotori
Copy link
Contributor Author

@giginet @Ryu0118

I have to fix this PR because it still depends on SwiftPM, not PIFKit.

Could you review my idea and give me some advice?

What I should do to merge PIFKit logic into this PR

Retrieve dependencies

When PIFGenerator depended on SwiftPM

descriptionPackage.graph.allModules.first(where: { $0.c99name == c99Name })

guard let resolvedTarget = descriptionPackage.graph.allModules.first(where: { $0.c99name == c99Name }) else {

PIFCompiler passed descriptionPackage: DescriptionPackage to PIFGenerator, but it was deleted with #183.

What to do with PIFKit

Configure build settings

When PIFGenerator depended on SwiftPM

Scipio used PIF.BuildSettings as a container.

PIF.BuildSettings has consists of some dictionaries for platform-specific settings.
c.f. swift-package-manager/Sources/XCBuildSupport/PIF.swift at 29e562052de0305417c9bd71d8a7c64bee2759fb · swiftlang/swift-package-manager

What to do with PIFKit

PIFKit.BuildConfiguration's buildSettings is a simple dictionary.

  • Option 1: Scipio doesn't consider PackageCondition nor set platform-specific values there.
  • Option 2: PIFGenerator configures them somehow to handle PackageConditions.

Do you have any ideas? If we choose the latter option, how should I update updateBuildConfiguration(_:) ?

@giginet
Copy link
Owner

giginet commented Jun 27, 2025

@S-Shimotori I'm sorry for the long wait.

As you mentioned, we should configure settings considering the Platform Filter, but PIFKit currently does not have that interface.

As far as I understand, the new ResolvedModule also has an interface to retrieve the PlatformFilter.
Since we only need an interface to set __platform_filter for PIFKit, I added it in #225.

With these, I believe we can reproduce the original implementation. What do you think?

@S-Shimotori
Copy link
Contributor Author

@giginet

Thank you. I updated this PR with #225

Comment on lines 255 to 258
case .mergeable:
settings[.OTHER_LDFLAGS, default: ["$(inherited)"]]
.append("-Wl,-make_mergeable")
case .dynamic:
Copy link
Owner

Choose a reason for hiding this comment

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

I believe we need to take the same path for mergeable as well.

Comment on lines 5 to 8
private let fixturePath = URL(fileURLWithPath: #filePath)
.deletingLastPathComponent()
.appendingPathComponent("Resources")
.appendingPathComponent("Fixtures")
Copy link
Owner

Choose a reason for hiding this comment

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

[nits] Please use the modern API

Suggested change
private let fixturePath = URL(fileURLWithPath: #filePath)
.deletingLastPathComponent()
.appendingPathComponent("Resources")
.appendingPathComponent("Fixtures")
private let fixturePath = URL(filePath: #filePath)
.deletingLastPathComponent()
.appending(components: "Resources", "Fixtures")

Comment on lines 135 to 138
case .mergeable:
configuration.buildSettings["OTHER_LDFLAGS"]
.append("-Wl,-make_mergeable")
case .dynamic:
Copy link
Owner

Choose a reason for hiding this comment

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

These flag settings will be required also for mergeable

Comment on lines 17 to 19
let outputDir = fileManager.temporaryDirectory
.appendingPathComponent("Scipio")
.appendingPathComponent(packageName)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let outputDir = fileManager.temporaryDirectory
.appendingPathComponent("Scipio")
.appendingPathComponent(packageName)
let outputDir = fileManager.temporaryDirectory
.appending(components: "Scipio", packageName)

.appendingPathComponent(packageName)

defer {
print("remove output directory: \(outputDir.path)")
Copy link
Owner

Choose a reason for hiding this comment

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

Needed?

verbose: false
)
)
let packageDir = fixturePath.appendingPathComponent(packageName)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let packageDir = fixturePath.appendingPathComponent(packageName)
let packageDir = fixturePath.appending(component: packageName)

dependencies: [String: Set<Destination>],
outputDir: URL
) async throws {
let xcFrameworkPath = outputDir.appendingPathComponent(framework.xcFrameworkName)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let xcFrameworkPath = outputDir.appendingPathComponent(framework.xcFrameworkName)
let xcFrameworkPath = outputDir.appending(component: framework.xcFrameworkName)

Comment on lines 88 to 91
let binaryPath = xcFrameworkPath
.appendingPathComponent(arch.rawValue)
.appendingPathComponent("\(framework.name).framework")
.appendingPathComponent(framework.name)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let binaryPath = xcFrameworkPath
.appendingPathComponent(arch.rawValue)
.appendingPathComponent("\(framework.name).framework")
.appendingPathComponent(framework.name)
let binaryPath = xcFrameworkPath
.appending(components: arch.rawValue, "\(framework.name).framework", framework.name)

@S-Shimotori
Copy link
Contributor Author

Changes:

  • Added fallthrough to apply this logic to mergeable type
  • Improve file path
  • Added test to check both dynamic and mergeable

private let fileManager: FileManager = .default

@Test(
arguments: [FrameworkType.dynamic, .mergeable]
Copy link
Owner

Choose a reason for hiding this comment

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

👍

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.

Resolve undefined symbols error of dynamic xcframework
2 participants