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

Allow conversion of dependencies to binary targets #9

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

Conversation

albertodebortoli
Copy link
Member

@albertodebortoli albertodebortoli commented Dec 17, 2024

This PR allows the creation of Package.swift files using binary targets instead of local or remote dependencies.
This can be particularly useful when relying on cached dependencies in the form of XCFrameworks.

Quite a few changes were needed and I recommend reviewing the commit history.

PackageGenerator can now be called with a new --dependencies-as-binary-targets flag and some related options:

$ PackageGenerator generate-package \
--spec <PATH_TO_PACKAGE_SPEC> \
--dependencies <PATH_TO_DEPENDENCIES_FILE> \
--template <PATH_TO_STENCIL_TEMPLATE> \
--dependencies-as-binary-targets \
--relative-dependencies-path <REALTIVE_PATH_TO_FOLDER_CONTAINING_DEPENDENCIES_AS_XCFRAMEWORKS> \
--version-refs-path <PATH_TO_VERSION_REFS_FILE> \
--exclusions <DEPENDENCIES_TO_EXCLUDE_FROM_BINARY_TARGETS>

@albertodebortoli albertodebortoli force-pushed the allow-dependencies-as-binary-targets branch from 109439d to c344094 Compare December 17, 2024 16:46
@albertodebortoli albertodebortoli force-pushed the allow-dependencies-as-binary-targets branch from 78c7c4c to 8300885 Compare January 20, 2025 13:43
@albertodebortoli albertodebortoli force-pushed the allow-dependencies-as-binary-targets branch from 5c3ec2e to c72c9ab Compare January 20, 2025 14:04
@albertodebortoli albertodebortoli marked this pull request as ready for review January 20, 2025 14:17
import Foundation
import Yams

final class DTOLoader {

Choose a reason for hiding this comment

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

or even enum as it's only namespace

Suggested change
final class DTOLoader {
struct DTOLoader {

)
)
}

Choose a reason for hiding this comment

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

both if and else has similar code

result.append(
                    PackageDependency(
                        name: dependency.name,
                        type: .remote(tag: dependency.version)
                    )
                )

I would suggest to simplify:

let type = if ... {
  .local(hash: versionRef)
} else {
  .remote(tag: dependency.version)
}

result.append(
  PackageDependency(
    name: dependency.name,
    type: type
  )
)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a guard in the way. This is meant to stay like this for readability.

Choose a reason for hiding this comment

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

guard I think can stay in place

let type: Type
 if ... {
  guard ... { throw .... }
  type = .local(hash: versionRef)
} else {
  type = .remote(tag: dependency.version)
}

result.append(
  PackageDependency(
    name: dependency.name,
    type: type
  )
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed ac64d1d

// MARK: - Helper Functions

private func parseDependencies(_ dependencies: [DependencySpec], versionRefsPath: String) throws -> [PackageDependency] {
var result = [PackageDependency]()

Choose a reason for hiding this comment

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

Suggested change
var result = [PackageDependency]()
var result = Set<PackageDependency>()

and you don't need to convert it below to Set

)
}

private var specUrl: URL

Choose a reason for hiding this comment

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

let for properties

if exclusions.contains(dependency.name) { return nil }
return Spec.LocalBinaryTarget(
name: dependency.name,
path: "\(relativeDependenciesPath)/\(dependency.name)/\(dependency.revision)/\(dependency.name).xcframework"

Choose a reason for hiding this comment

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

you can use join - [relativeDependenciesPath, dependency.name, ...].join()


func cachableSpec(additionalLocalBinaryTargets: [LocalBinaryTarget], exclusions: [String]) -> Spec {
let products = products.map { product in
if product.productType == .library {

Choose a reason for hiding this comment

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

guard?


extension URL: @retroactive Comparable {

public static func < (

Choose a reason for hiding this comment

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

imho, such a comparison is not quite unclear. would remove/replace to more descriptive sortByLenght.... because URL("-1.0") URL("10") what do we compare here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a leftover. I'll remove it.

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