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

Use Version type from SwiftPM #2670

Merged
merged 4 commits into from
Jan 22, 2019
Merged

Use Version type from SwiftPM #2670

merged 4 commits into from
Jan 22, 2019

Conversation

mdiep
Copy link
Member

@mdiep mdiep commented Dec 30, 2018

Depends on #2665.

This is the first step in building more off of SwiftPM. I think if we do this, then use their equivalent of VersionSpecifier, we can start reading Package.resolved and/or using the Swift PM resolver. Using common types will make those things easier.

expect(Version.from(PinnedVersion("1.4")).value) == Version(1, 4, 0)
expect(Version.from(PinnedVersion("v2.8.9")).value) == Version(2, 8, 9)
expect(Version.from(PinnedVersion("2.8.2-alpha")).value) == Version(2, 8, 2, prereleaseIdentifiers: ["alpha"])
expect(Version.from(PinnedVersion("2.8.2-alpha+build234")).value) == Version(2, 8, 2, prereleaseIdentifiers: ["alpha"], buildMetadataIdentifiers: ["build234"])

Choose a reason for hiding this comment

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

Line Length Violation: Line should be 160 characters or less: currently 162 characters (line_length)

Tests/CarthageKitTests/DB.swift Outdated Show resolved Hide resolved
Source/CarthageKit/Version.swift Outdated Show resolved Hide resolved
Source/CarthageKit/Version.swift Outdated Show resolved Hide resolved
Source/CarthageKit/CarthageKitVersion.swift Show resolved Hide resolved
@mdiep mdiep force-pushed the use-versions-from-swiftpm branch 3 times, most recently from 32b336f to c18dbca Compare December 31, 2018 16:28
@mdiep
Copy link
Member Author

mdiep commented Dec 31, 2018

Integration tests are failing due to warnings because libSwiftPM is built as a dynamic library. I've asked if this is necessary. We'll have to wait on that before proceeding here.

@mdiep mdiep force-pushed the use-versions-from-swiftpm branch 2 times, most recently from 78363b2 to 7d216ea Compare January 3, 2019 05:31
.gitignore Outdated Show resolved Hide resolved
@@ -1,5 +1,8 @@
import Foundation
import Result
import Utility

import struct Foundation.URL
Copy link
Member Author

Choose a reason for hiding this comment

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

This disambiguation is necessary because Utility defines its own URL type.

@mdiep mdiep force-pushed the use-versions-from-swiftpm branch 3 times, most recently from 885dd9c to 979d0a5 Compare January 3, 2019 07:02
@mdiep
Copy link
Member Author

mdiep commented Jan 6, 2019

I'm not sure why this is failing on CI. The tests always pass locally and I did get one green build. 😕

@@ -24,7 +18,7 @@ matrix:
env: JOB=CI_TEST_10_1
- os: osx
language: objective-c
osx_image: xcode9.4
osx_image: xcode10.1
Copy link
Member

Choose a reason for hiding this comment

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

I am a little confused now as what version of Xcode we required to build.

Requiring 10.1 cuts out High Sierra homebrew builds

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR would raise the requirement from 9.4 to either 10.0 or 10.1. BUT our deployment target is still 10.10, so you only need a newer OS to build. (And I think you could build on an older OS if you installed a newer swift first.)

SwiftPM's master requires Swift 4, which is the reason for this change.

We're using master because I had to submit a PR to add a static library version of SwiftPM. I could do that on a fork of SwiftPM against an older release, but I think we'll want to get more changes from SwiftPM master.

I think that building off of SwiftPM is a big enough advantage that dropping support for building on 10.11 isn't a big deal. But I'm open to other opinions here. @Carthage/carthage

Copy link
Member

Choose a reason for hiding this comment

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

dropping support for building on 10.11 isn't a big deal.

We don't have statistics so it's hard to say. I for one just updated last week since in my organization's IT department wouldn't let us.

Once this is released we effectively cut out whoever is relying on homebrew but on 10.10.

We could release a one time alternative formula of Carthage (cathage@0.31.2) at the current version which would still allow homebrew builds of 0.31.2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't people be able to install older versions of Carthage already?

If you're on an older OS, you have to use older toolchains anyway, so there shouldn't be a pressing need to upgrade your Carthage. And since you could install through the pkg installer, I'm not bothered by it—you can still upgrade if you need to.

I know this is potentially inconvenient, but I think the wins from building off of SwiftPM are too great not to move forward with this.

Copy link
Member

@tmspzz tmspzz Jan 8, 2019

Choose a reason for hiding this comment

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

I'm not suggesting to hold back at all :)

Sure the .pkg works but I think we can still keep the convenience offered by homebrew by adopting the practice of releasing an end-of-support formula every time a version of Xcode requires an OS update (which is in itself annoying) .

Copy link
Member

Choose a reason for hiding this comment

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

@mdiep mdiep merged commit a5f34f4 into master Jan 22, 2019
@mdiep mdiep deleted the use-versions-from-swiftpm branch January 22, 2019 11:41
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.

None yet

3 participants