Fix Swift Package Manager compile issue #4017

rynecheow · 2019-06-07T12:21:37Z

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

Issue Link 🔗

#4016

Goals ⚽

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 · 2019-06-10T01:44:03Z

@liuxuan30 Could you help to review this?

liuxuan30 · 2019-06-10T02:22:53Z

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?

rynecheow · 2019-06-10T05:52:01Z

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 · 2019-06-10T06:37:17Z

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

rynecheow · 2019-06-10T08:34:37Z

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

jjatie · 2019-06-10T10:23:31Z

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 · 2019-06-10T10:49:25Z

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

liuxuan30 · 2019-06-11T03:56:35Z

@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 · 2019-06-11T17:48:38Z

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

jjatie · 2019-06-11T17:52:49Z

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?

Fix Swift Package Manager compile issue ChartsOrg#4017

mattjgalloway · 2019-08-13T19:19:13Z

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 · 2019-09-12T12:31:22Z

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 · 2019-09-13T15:57:57Z

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

RamblinWreck77 · 2019-09-13T16:05:14Z

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 · 2019-09-16T03:11:36Z

@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

ChartsDemo-iOS/Swift/Demos/CombinedChartViewController.swift

liuxuan30 · 2019-09-16T03:17:31Z

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

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

jjoelson · 2019-09-16T12:48:49Z

@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 · 2019-09-17T00:14:55Z

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.

liuxuan30 · 2019-09-17T00:15:15Z

.gitignore

@@ -72,3 +72,4 @@ fastlane/test_output
 Carthage
 Charts.framework.zip
 ChartsRealm.framework.zip
+.swiftpm


.swiftpm or .swiftpm/?

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

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

rynecheow · 2019-09-17T01:07:57Z

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 · 2019-09-20T08:31:52Z

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 · 2019-09-20T08:52:02Z

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

liuxuan30 mentioned this pull request Jun 10, 2019

Cannot subclass LineChartRenderer with Clang 11 #4018

Closed

1 task

liuxuan30 self-assigned this Jun 10, 2019

liuxuan30 self-requested a review June 10, 2019 02:36

This was referenced Jul 1, 2019

fixed Xcode11 SwiftPM build #4045

Closed

Fix Xcode 11 errors in SPM project #4048

Closed

andersonjulian added a commit to andersonjulian/Charts that referenced this pull request Aug 2, 2019

danielgindi/Charts PR ChartsOrg#4017

0535aa2

Fix Swift Package Manager compile issue ChartsOrg#4017

liuxuan30 removed their assignment Aug 7, 2019

liuxuan30 requested changes Sep 16, 2019

View reviewed changes

Source/Supporting Files/Info.plist Outdated Show resolved Hide resolved

ChartsDemo-iOS/Swift/Demos/CombinedChartViewController.swift Outdated Show resolved Hide resolved

liuxuan30 mentioned this pull request Sep 16, 2019

SPM support for iOS, macOS, tvOS #4118

Closed

Fix Swift Package Manager compile issue

bf5db2f

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

liuxuan30 reviewed Sep 17, 2019

View reviewed changes

rynecheow added 2 commits September 17, 2019 08:57

Unspecify library type

1086579

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

Fix gitignore to ignore .swiftpm as a directory

7d1296d

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

rynecheow requested a review from liuxuan30 September 17, 2019 01:08

liuxuan30 approved these changes Sep 20, 2019

View reviewed changes

liuxuan30 merged commit 593630e into ChartsOrg:master Sep 20, 2019

liuxuan30 mentioned this pull request Sep 26, 2019

Unable to compile with Swift Package Manager due to missing dependencies #4016

Closed

1 task

liuxuan30 mentioned this pull request Oct 9, 2019

Adds 'Source' folder as path to the target in Package.swift #4163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Swift Package Manager compile issue #4017

Fix Swift Package Manager compile issue #4017

rynecheow commented Jun 7, 2019 •

edited

Loading

rynecheow commented Jun 10, 2019

liuxuan30 commented Jun 10, 2019 •

edited

Loading

rynecheow commented Jun 10, 2019 •

edited

Loading

liuxuan30 commented Jun 10, 2019 •

edited

Loading

rynecheow commented Jun 10, 2019

jjatie commented Jun 10, 2019 •

edited

Loading

rynecheow commented Jun 10, 2019

liuxuan30 commented Jun 11, 2019

jjatie commented Jun 11, 2019

jjatie commented Jun 11, 2019

mattjgalloway commented Aug 13, 2019 •

edited

Loading

jjoelson commented Sep 12, 2019

RamblinWreck77 commented Sep 13, 2019

RamblinWreck77 commented Sep 13, 2019

liuxuan30 commented Sep 16, 2019

liuxuan30 commented Sep 16, 2019

jjoelson commented Sep 16, 2019

liuxuan30 commented Sep 17, 2019 •

edited

Loading

liuxuan30 Sep 17, 2019

rynecheow Sep 17, 2019

rynecheow commented Sep 17, 2019

liuxuan30 commented Sep 20, 2019 •

edited

Loading

liuxuan30 commented Sep 20, 2019

Fix Swift Package Manager compile issue #4017

Fix Swift Package Manager compile issue #4017

Conversation

rynecheow commented Jun 7, 2019 • edited Loading

Issue Link 🔗

Goals ⚽

Implementation Details 🚧

Testing Details 🔍

rynecheow commented Jun 10, 2019

liuxuan30 commented Jun 10, 2019 • edited Loading

rynecheow commented Jun 10, 2019 • edited Loading

liuxuan30 commented Jun 10, 2019 • edited Loading

rynecheow commented Jun 10, 2019

jjatie commented Jun 10, 2019 • edited Loading

rynecheow commented Jun 10, 2019

liuxuan30 commented Jun 11, 2019

jjatie commented Jun 11, 2019

jjatie commented Jun 11, 2019

mattjgalloway commented Aug 13, 2019 • edited Loading

jjoelson commented Sep 12, 2019

RamblinWreck77 commented Sep 13, 2019

RamblinWreck77 commented Sep 13, 2019

liuxuan30 commented Sep 16, 2019

liuxuan30 commented Sep 16, 2019

jjoelson commented Sep 16, 2019

liuxuan30 commented Sep 17, 2019 • edited Loading

liuxuan30 Sep 17, 2019

Choose a reason for hiding this comment

rynecheow Sep 17, 2019

Choose a reason for hiding this comment

rynecheow commented Sep 17, 2019

liuxuan30 commented Sep 20, 2019 • edited Loading

liuxuan30 commented Sep 20, 2019

rynecheow commented Jun 7, 2019 •

edited

Loading

liuxuan30 commented Jun 10, 2019 •

edited

Loading

rynecheow commented Jun 10, 2019 •

edited

Loading

liuxuan30 commented Jun 10, 2019 •

edited

Loading

jjatie commented Jun 10, 2019 •

edited

Loading

mattjgalloway commented Aug 13, 2019 •

edited

Loading

liuxuan30 commented Sep 17, 2019 •

edited

Loading

liuxuan30 commented Sep 20, 2019 •

edited

Loading