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

Parse dump-pif result and reduce XCBuildSupport dependency #183

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

Conversation

giginet
Copy link
Owner

@giginet giginet commented Feb 9, 2025

Use swift package dump-pif to construct PIF instead of the internal implementation of SwiftPM.

Using dump-pif command, we can reduce dependency and we can use stable specs.
Our final goal is to remove the XCBuildSupport dependency.

This PR introduces SwiftyJSON to manipulate PIFs. because it's tough to define a complete PIF using Codable. We only need a subset of PIF.

Instead, I concluded the JSON manipulation into the PIFKit target. ScipioKit can manipulate PIF in type-safe.

@giginet giginet marked this pull request as ready for review February 11, 2025 06:44
@giginet giginet requested review from ikesyo and freddi-kit February 11, 2025 06:44
@Suite
struct TargetTests {
@Test
func canParseTarget() async throws {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func canParseTarget() async throws {
func canParseTarget() throws {

struct TargetTests {
@Test
func canParseTarget() async throws {
let jsonData = try #require(FixtureLoader.load(named: "target.json"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let jsonData = try #require(FixtureLoader.load(named: "target.json"))
let jsonData = try #require(FixtureLoader.load(named: "Target.json"))

or rename the json file to target.json.

Comment on lines +4 to +10
static func load(named filename: String) -> Data? {
let url = URL(filePath: #filePath)
.deletingLastPathComponent()
.appending(components: "Fixtures", filename)
let fileManager = FileManager.default
return fileManager.contents(atPath: url.path)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static func load(named filename: String) -> Data? {
let url = URL(filePath: #filePath)
.deletingLastPathComponent()
.appending(components: "Fixtures", filename)
let fileManager = FileManager.default
return fileManager.contents(atPath: url.path)
}
static func load(named filename: String) throws -> Data {
let url = URL(filePath: #filePath)
.deletingLastPathComponent()
.appending(components: "Fixtures", filename)
return try Data(contentsOf: url)
}

Comment on lines +30 to +34
let encoder = JSONEncoder()
_ = try String(data: encoder.encode(configuration), encoding: .utf8)

let redecoded = try decoder.decode(BuildConfiguration.self, from: fixtureData)
#expect(configuration == redecoded)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test doesn't decode the encoded data.

Suggested change
let encoder = JSONEncoder()
_ = try String(data: encoder.encode(configuration), encoding: .utf8)
let redecoded = try decoder.decode(BuildConfiguration.self, from: fixtureData)
#expect(configuration == redecoded)
let encoder = JSONEncoder()
let encoded = try encoder.encode(configuration)
let redecoded = try decoder.decode(BuildConfiguration.self, from: encoded)
#expect(configuration == redecoded)

Comment on lines +37 to +45
private static let appendingFixtures: [(BuildConfiguration.MacroExpressionValue?, [String])] = [
(.bool(false), ["NO", "added"]),
(.bool(true), ["YES", "added"]),
(.string("foo"), ["foo", "added"]),
(.stringList(["foo", "bar"]), ["foo", "bar", "added"]),
(nil, ["$(inherited)", "added"]),
]
@Test("can append flags", arguments: appendingFixtures)
func appending(setting: BuildConfiguration.MacroExpressionValue?, expected: [String]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
private static let appendingFixtures: [(BuildConfiguration.MacroExpressionValue?, [String])] = [
(.bool(false), ["NO", "added"]),
(.bool(true), ["YES", "added"]),
(.string("foo"), ["foo", "added"]),
(.stringList(["foo", "bar"]), ["foo", "bar", "added"]),
(nil, ["$(inherited)", "added"]),
]
@Test("can append flags", arguments: appendingFixtures)
func appending(setting: BuildConfiguration.MacroExpressionValue?, expected: [String]) {
private static let appendingFixtures: [(BuildConfiguration.MacroExpressionValue?, [String])] = [
(.bool(false), ["NO", "added"]),
(.bool(true), ["YES", "added"]),
(.string("foo"), ["foo", "added"]),
(.stringList(["foo", "bar"]), ["foo", "bar", "added"]),
(nil, ["$(inherited)", "added"]),
]
@Test("can append flags", arguments: appendingFixtures)
func appending(setting: BuildConfiguration.MacroExpressionValue?, expected: [String]) {

dependencies: [
.target(name: "PIFKit"),
]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
)
),

Comment on lines +40 to +42
pifObject["contents"]["buildConfigurations"].arrayObject = target.buildConfigurations.compactMap { try? $0.toJSON() }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to reuse a single JSONEncoder I think.

Suggested change
pifObject["contents"]["buildConfigurations"].arrayObject = target.buildConfigurations.compactMap { try? $0.toJSON() }
}
let jsonEncoder = JSONEncoder()
pifObject["contents"]["buildConfigurations"].arrayObject = target.buildConfigurations.compactMap { try? $0.toJSON(using: jsonEncoder) }
}

Comment on lines +138 to +140
if case .string(let moduleMapFileContents) = configuration.buildSettings["MODULEMAP_FILE_CONTENTS"],
moduleMapFileContents.contains("\(target.name)-Swift.h") {
resolveModuleMapPath(of: target, configuration: &configuration, sdk: sdk)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if case .string(let moduleMapFileContents) = configuration.buildSettings["MODULEMAP_FILE_CONTENTS"],
moduleMapFileContents.contains("\(target.name)-Swift.h") {
resolveModuleMapPath(of: target, configuration: &configuration, sdk: sdk)
if case .string(let moduleMapFileContents) = configuration.buildSettings["MODULEMAP_FILE_CONTENTS"],
moduleMapFileContents.contains("\(target.name)-Swift.h")
{
resolveModuleMapPath(of: target, configuration: &configuration, sdk: sdk)

would be more readable (for me).

configuration.buildSettings["SKIP_INSTALL"] = false
configuration.buildSettings["INSTALL_PATH"] = "/usr/local/lib"
configuration.buildSettings["ONLY_ACTIVE_ARCH"] = false
configuration.buildSettings["SDKROOT"] = .string(sdk.settingValue)
Copy link
Collaborator

Choose a reason for hiding this comment

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

SDKROOT is added, for what reason is that required?

private let fileSystem: any FileSystem

init(
package: DescriptionPackage,
packageName: String,
packageLocator: some PackageLocator,
buildParameters: BuildParameters,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we don't use PIFBuilder.generatePIF anymore, only buildParameters.toolchain.toolchainLibDir is needed. How about passing the specific value instead of the entire BuildParameters?

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.

2 participants