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

Fix Swift Package Manager compile issue #4017

Merged
merged 3 commits into from
Sep 20, 2019
Merged

Fix Swift Package Manager compile issue #4017

merged 3 commits into from
Sep 20, 2019

Conversation

rynecheow
Copy link
Contributor

@rynecheow rynecheow commented Jun 7, 2019

Signed-off-by: Ryne Cheow rynecheow@gmail.com

Issue Link πŸ”—

#4016

Goals ⚽

  1. Resolve dependency errors from SPM builds or swift build

Implementation Details 🚧

Resolve dependencies for correct platform

Testing Details πŸ”

Compile for all platforms using Swift 5.0 toolchain

@rynecheow
Copy link
Contributor Author

@liuxuan30 Could you help to review this?

@liuxuan30
Copy link
Member

liuxuan30 commented Jun 10, 2019

Hmm this seems caused by Xcode 11 beta? @rynecheow any documentation or reason, about this behavior, that it's expected for Xcode 11, or new Swift version?

Because given the history dealing with swift or xcode upgrade, I have seen the similar changes about this import back and forth. At first adding the import, than delete it, and add it back again. What I'm concerning is this might be a bug for Xcode 11 beta, not us.

Any idea @jjatie ?

I could also prefer holding this PR until Xcode 11 GM is released.

But, for anyone testing Xcode 11 or iOS13, we could create a branch for it, and you can commit your PR to this branch. Is this ok for you?

@liuxuan30 liuxuan30 self-assigned this Jun 10, 2019
@liuxuan30 liuxuan30 self-requested a review June 10, 2019 02:36
@rynecheow
Copy link
Contributor Author

rynecheow commented Jun 10, 2019

Hey @liuxuan30 I just ran a deeper test using current stable tools (Swift 5.0 toolchain and Xcode 10.2.1) and build has failed as well. Updated the issue to reflect more accurate problem.

Basically swift build would fail, and it has nothing to do with Xcode 11 toolchains yet.

@liuxuan30
Copy link
Member

liuxuan30 commented Jun 10, 2019

@rynecheow it's strange, our CI and my Xcode 10.2.1 works just well.

@rynecheow
Copy link
Contributor Author

@liuxuan30 Currently CI is not building the SPM package according build script / build log I believe.

@jjatie
Copy link
Collaborator

jjatie commented Jun 10, 2019

This shouldn't be worked on until later this summer. We are only on beta 1 and things are subject to change (especially where all the SPM stuff was developed in private). I noticed the toolchain wasn't bumped in this PR

@rynecheow
Copy link
Contributor Author

@jjatie This pull request aims to fix the build error on current stable Swift toolchain - not Xcode 11
re. #4016

@liuxuan30
Copy link
Member

@rynecheow I kind of understand what you mean now.

if this is about fixing 'Swift Package Manager' compiling - which is rarely used by me or other people so far (you are the first one reporting this), I would like to know or maybe send a bug to APPL first to figure out why only this swift compile fails to build, while xcodebuild works well.

It's kind of inconsistent, as SPM fails to find the framework while xcodebuild can handle it?

@jjatie
Copy link
Collaborator

jjatie commented Jun 11, 2019

@rynecheow The current toolchain doesn't support iOS. I'm not sure how we could fix it without migrating to the new toolchain.

@jjatie
Copy link
Collaborator

jjatie commented Jun 11, 2019

Why was Package.swift not updated to the 5.0 toolchain?
Why were the demos touched at all when they aren't part of Package.swift?

andersonjulian added a commit to andersonjulian/Charts that referenced this pull request Aug 2, 2019
Fix Swift Package Manager compile issue ChartsOrg#4017
@liuxuan30 liuxuan30 removed their assignment Aug 7, 2019
@mattjgalloway
Copy link

mattjgalloway commented Aug 13, 2019

Chiming in here - I think that we should commit this. Reason being, things are being used when they're not explicitly imported. I haven't looked into why this works when compiling using the Xcode project, but just take a look at this:

DataApproximator+N.swift:

import Foundation

extension CGPoint {
    fileprivate func distanceToLine(from linePoint1: CGPoint, to linePoint2: CGPoint) -> CGFloat {
        let dx = linePoint2.x - linePoint1.x
        let dy = linePoint2.y - linePoint1.y
        
        let dividend = abs(dy * self.x - dx * self.y - linePoint1.x * linePoint2.y + linePoint2.x * linePoint1.y)
        let divisor = sqrt(dx * dx + dy * dy)
        
        return dividend / divisor
    }
}

How can we be extending CGPoint without importing CoreGraphics where CGPoint resides? AFAIK Foundation in no way should be importing CoreGraphics. Must be something about the Xcode project that means that CoreGraphics (or UIKit so CoreGraphics recursively) is imported when compiling this file. But really, we should be importing what we use anyway.

@jjoelson
Copy link

Just want to note that Xcode 11 GM is out and Swift PM still fails to compile the project without these imports. I don't know if this PR is the "right" fix, but it does make it usable via SwiftPM.

@RamblinWreck77
Copy link

@liuxuan30 Also confirming this issue still occurs in SwiftPM on the Gold Master of Xcode11.

@RamblinWreck77
Copy link

Also, the Package.swift specifies:

 .library(name: "Charts", type: .dynamic, targets: ["Charts"])

While the docs specify it is best to leave it nil and let SwiftPM decide. Is there any reason to force .dynamic? Dynamic libraries impact app boot times on iOS.

@liuxuan30
Copy link
Member

@jjoelson can you check what @RamblinWreck77 said?

I'm open to merge this, if the final compiler takes it this way.

Source/Supporting Files/Info.plist Outdated Show resolved Hide resolved
@liuxuan30
Copy link
Member

#4118
seems this is ready to merge, if you don't want to commit more?

Signed-off-by: Ryne Cheow <rynecheow@gmail.com>
@jjoelson
Copy link

@liuxuan30 The SwiftPM docs do say to leave type nil unless the library disallows a linkage type for other reasons:

Leave this parameter unspecified to let to let the Swift Package Manager choose between static or dynamic linking (recommended). If you do not support both linkage types, use .static or .dynamic for this parameter.

But then there's also an example that has separate products for static and dynamic, so it's a bit confusing:

let package = Package(
    name: "Paper",
    products: [
        .executable(name: "tool", targets: ["tool"]),
        .library(name: "Paper", targets: ["Paper"]),
        .library(name: "PaperStatic", type: .static, targets: ["Paper"]),
        .library(name: "PaperDynamic", type: .dynamic, targets: ["Paper"]),
    ],
    dependencies: [
        .package(url: "http://example.com.com/ExamplePackage/ExamplePackage", from: "1.2.3"),
        .package(url: "http://some/other/lib", .exact("1.2.3")),
    ],
    targets: [
        .target(
            name: "tool",
            dependencies: [
                "Paper",
                "ExamplePackage"
            ]),
        .target(
            name: "Paper",
            dependencies: [
                "TSCBasic",
                .target(name: "Utility"),
                .product(name: "AnotherExamplePackage"),
            ])
    ]
)

@liuxuan30
Copy link
Member

liuxuan30 commented Sep 17, 2019

there is no reason to only use dynamic, but by default, this "library" is built as framework, so I guess that's why it's dynamic. I think we should be fine with nil type, as long as SPM has no bugs handling it.

@rynecheow can you also check what #4118 did for Package.swift ? Basically I would like to merge this as you are the first PR. I didn't see you touch this file, but #4118 change a few, especially

.target(name: "Charts", dependencies: [], path: "./Source", publicHeadersPath: "./Source/Supporting Files")

I'm not sure the impact about this, as I'm not following SPM trend now. Besides, I will be grateful if you could modify the type to nil as discussed above, so I don't have to pull your branch. Thanks.

.gitignore Outdated
@@ -72,3 +72,4 @@ fastlane/test_output
Carthage
Charts.framework.zip
ChartsRealm.framework.zip
.swiftpm
Copy link
Member

Choose a reason for hiding this comment

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

.swiftpm or .swiftpm/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rightfully .swiftpm/ - let me change that. Did .swiftpm cause it does the trick.

Signed-off-by: Ryne Cheow <rynecheow@gmail.com>
Signed-off-by: Ryne Cheow <rynecheow@gmail.com>
@rynecheow
Copy link
Contributor Author

there is no reason to only use dynamic, but by default, this "library" is built as framework, so I guess that's why it's dynamic. I think we should be fine with nil type, as long as SPM has no bugs handling it.

@rynecheow can you also check what #4118 did for Package.swift ? Basically I would like to merge this as you are the first PR. I didn't see you touch this file, but #4118 change a few, especially

.target(name: "Charts", dependencies: [], path: "./Source", publicHeadersPath: "./Source/Supporting Files")

I'm not sure the impact about this, as I'm not following SPM trend now. Besides, I will be grateful if you could modify the type to nil as discussed above, so I don't have to pull your branch. Thanks.

path: "./Source" is unnecessary as it is the parameter automatically defaults to a certain pattern if it is not specified.
https://developer.apple.com/documentation/swift_packages/target/2880335-path

publicHeadersPath: "./Source/Supporting Files" I am not sure if this is completely necessary as I believe SPM already handles that at its build process.

@liuxuan30
Copy link
Member

liuxuan30 commented Sep 20, 2019

Seems green light now. BTW, do you think we need to add script code for travis to build via SPM? Just to build it to make sure it builds fine, No UT is needed. Or will it be fine with the config file, as long as Xcode build, it won't fail for SPM? Because currently it looks the opposite way.

Ideally we could have a separate SPM item for travis, let's say Charts for SPM, or you can simply put it in the main item. I'm not sure how long it would take to build via SPM, if it takes a significant time, I suggest use a separate one.

Otherwise I will merge next Monday.

@liuxuan30
Copy link
Member

A second thought we should just go ahead, if you think travis ci is needed, let's add later. Not surehowpopular spm can be

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.

6 participants