Remote Version Validation #1533

rodchile · 2016-10-16T02:15:28Z

Verifies if the local version of Carthage is up to date.
It should solve #1181 and #1130

Credits also to @mdiep for his help!

ikesyo · 2016-10-16T03:04:21Z

I think this would have a negative impact for GitHub API's rate limiting, especially on CI environments.

mdiep · 2016-10-16T11:39:13Z

I think this would have a negative impact for GitHub API's rate limiting, especially on CI environments.

CI environments are already unable to use the GitHub API since it's aggressively rate-limited. Using the API features is basically dependent on authenticated use of the API. So I think this is okay.

mdiep

I think this will be a big improvement. Users frequently have problems when running older versions.

mdiep · 2016-10-16T11:50:02Z

Source/CarthageKit/Scannable.swift

+
+	/// Attemps to create an instance of NSScaner and then use the convenience
+	/// function specified in the protocol
+	static public func fromString(string: String) -> Result<Self,CarthageError>


There should be a here after the,.

mdiep · 2016-10-16T11:50:32Z

Source/CarthageKit/Scannable.swift

+
+extension Scannable {
+
+	/// Attemps to create an instance of NSScaner and then use the convenience


A couple typos here: attempts, NSScanner

mdiep · 2016-10-16T11:50:46Z

Source/CarthageKit/Scannable.swift

+	/// function specified in the protocol
+	static public func fromString(string: String) -> Result<Self,CarthageError>
+	{
+		return Self.fromScanner(NSScanner(string:string))


There should be a here after the:.

mdiep · 2016-10-16T11:52:43Z

Source/carthage/BuildVersion.swift

+	}
+}
+
+func fetchLatestCarthageVersion() -> CarthageVersion{


I think it'd be better to split this into two code units:

one to get the latest version from the server

one to get the local version (which could be used for carthage version)

mdiep · 2016-10-16T11:53:44Z

Source/carthage/BuildVersion.swift

+		.timeoutWithError(CarthageError.GitHubAPITimeout, afterInterval: 0.1, onScheduler: QueueScheduler.mainQueueScheduler)
+		.first()
+
+	return CarthageVersion(localVersion: SemanticVersion.fromString(versionString).value!, remoteVersion: latestVersion!.value!)


This shouldn't use !s with latestVersion because it shouldn't be a hard error if this API request fails.

mdiep · 2016-10-16T11:56:38Z

Source/carthage/main.swift

+let carthageVersion = fetchLatestCarthageVersion()
+
+if carthageVersion.shouldUpgrade() == true {
+	print("Please update to the latest Carthage version: \(carthageVersion.remoteVersion). You currently are on \(carthageVersion.localVersion)")


We should use carthage.println here to format and color this text. (See Formatting.swift.)

NachoSoto

There are lots of style violations. I could swear the repo had SwiftLint (I can work on adding it), but in the mean time can you fix them by hand?

NachoSoto · 2016-10-17T03:48:58Z

Source/CarthageKit/Scannable.swift

+	/// Attempts to create an instance of NSScanner and then use the convenience
+	/// function specified in the protocol
+	static public func fromString(string: String) -> Result<Self, CarthageError>
+	{


Should go on the previous line.

NachoSoto · 2016-10-17T03:49:22Z

Source/carthage/BuildVersion.swift

+import Result
+import Tentacle
+
+public struct CarthageVersion{


Space before {

NachoSoto · 2016-10-17T03:49:46Z

Source/carthage/BuildVersion.swift

+public struct CarthageVersion{
+
+	public let localVersion:SemanticVersion
+	public let remoteVersion:SemanticVersion


Spaces after :. Didn't SwiftLint catch these?

Done!

BTW, I activated SwiftLint and got ˜15000 issues :D in the project. I think that it isn't being used.

NachoSoto · 2016-10-17T03:49:56Z

Source/carthage/BuildVersion.swift

+	public let remoteVersion:SemanticVersion
+
+	public init (localVersion: SemanticVersion, remoteVersion:SemanticVersion){
+		self.localVersion = localVersion


No space before (

NachoSoto · 2016-10-17T03:50:40Z

Source/carthage/BuildVersion.swift

+		.attemptMap { (release) -> Result<SemanticVersion, CarthageError> in
+			return SemanticVersion.fromString(release.tag)
+		}
+		.timeoutWithError(CarthageError.GitHubAPITimeout, afterInterval: 0.1, onScheduler: QueueScheduler.mainQueueScheduler)


I've updated the value to 3 seconds. Thanks for catching this.

NachoSoto · 2016-10-17T03:51:03Z

Source/carthage/BuildVersion.swift

+			return SemanticVersion.fromString(release.tag)
+		}
+		.timeoutWithError(CarthageError.GitHubAPITimeout, afterInterval: 0.1, onScheduler: QueueScheduler.mainQueueScheduler)
+		.first()


Hmm why make this blocking?

Because we want to validate first Carthage version before doing any task. In case their version is behind, we let know right away about that.

NachoSoto · 2016-10-17T03:51:27Z

Source/carthage/main.swift

+
+	let formatting = ColorOptions.Formatting(true)
+
+	carthage.println(formatting.bullets + "Please update to the latest Carthage version: \(carthageVersion.remoteVersion). You currently are on \(carthageVersion.localVersion)")


NachoSoto · 2016-10-17T03:52:30Z

Source/carthage/BuildVersion.swift

+		self.remoteVersion = remoteVersion
+	}
+
+	func shouldUpgrade() ->Bool {


NachoSoto · 2016-10-17T03:52:57Z

Source/carthage/BuildVersion.swift

+
+	func shouldUpgrade() ->Bool {
+
+		if localVersion < remoteVersion {


This is redundant, can just return localVersion < remoteVersion

Woah - totally agree. Thanks for catching this.

NachoSoto · 2016-10-17T03:53:16Z

Source/carthage/BuildVersion.swift

+	public let localVersion:SemanticVersion
+	public let remoteVersion:SemanticVersion
+
+	public init (localVersion: SemanticVersion, remoteVersion:SemanticVersion){


NachoSoto · 2016-10-17T03:56:43Z

Source/carthage/BuildVersion.swift

+			return releases.first!
+		}
+		.mapError(CarthageError.GitHubAPIRequestFailed)
+		.attemptMap { (release) -> Result<SemanticVersion, CarthageError> in


This can be

.map { $0.tag } .attemptMap(SemanticVersion.fromString)

I think that the more verbose version is easier to understand. @mdiep what do you think?

I'd go with @NachoSoto's suggestion. 👍

I tried this suggestion and seems that there is an issue with the inferrer based on the $0.tag value.

mdiep

This is great! Almost there!

mdiep · 2016-11-06T19:49:17Z

Source/carthage/BuildVersion.swift

+		.attemptMap { (release) -> Result<SemanticVersion, CarthageError> in
+			return SemanticVersion.fromString(release.tag)
+		}
+		.timeoutWithError(CarthageError.GitHubAPITimeout, afterInterval: 3, onScheduler: QueueScheduler.mainQueueScheduler)


This timeout should be much lower. I wouldn't want to keep users waiting for 3 seconds on every carthage invocation. Maybe 0.5?

mdiep · 2016-11-06T19:51:02Z

Source/carthage/BuildVersion.swift

+	func shouldUpgrade() -> Bool {
+		return localVersion < remoteVersion
+	}
+}


I don't think this struct gets us anything. Lets get rid of it and just use localVersion and remoteVersion directly.

mdiep · 2016-11-06T19:51:37Z

Source/carthage/main.swift

@@ -20,6 +20,15 @@ guard ensureGitVersion().first()?.value == true else {
 	exit(EXIT_FAILURE)
 }

+let carthageVersion = fetchLatestCarthageVersion()
+
+if carthageVersion.shouldUpgrade() == true {


With the changes I've suggested, this would become:

if let remoteVersion = remoteVersion() where localVersion() < remoteVersion {

mdiep · 2016-11-06T19:52:56Z

Source/carthage/BuildVersion.swift

+		.timeoutWithError(CarthageError.GitHubAPITimeout, afterInterval: 3, onScheduler: QueueScheduler.mainQueueScheduler)
+		.first()
+
+	return latestRemoteVersion!.value!


This definitely shouldn't use !. This feature is optional, so if GitHub goes down, carthage shouldn't become unusable. The function return a SemanticVersion? and this should be return latestRemoveVersion?.value.

…r than remote, prints message in the console

mdiep · 2016-11-06T20:48:29Z

🚢

mdiep requested changes Oct 16, 2016

View reviewed changes

NachoSoto reviewed Oct 17, 2016

View reviewed changes

rodchile force-pushed the master branch from e2d43ea to 94cebcb Compare October 22, 2016 18:21

rodchile added a commit to rodchile/Carthage that referenced this pull request Oct 22, 2016

Includes improvements suggestions from PR Carthage#1533

94cebcb

mdiep requested changes Nov 6, 2016

View reviewed changes

mdiep reviewed Nov 6, 2016

View reviewed changes

rodchile added 4 commits November 6, 2016 15:26

Implements remote version verification. In case local version is olde…

c1ae067

…r than remote, prints message in the console

Fixes areas suggested in PR

5035702

Includes improvements suggestions from PR Carthage#1533

7348638

Includes improvements from PR comments

e125f16

rodchile force-pushed the master branch from 94cebcb to e125f16 Compare November 6, 2016 20:27

mdiep approved these changes Nov 6, 2016

View reviewed changes

mdiep merged commit 4d28e96 into Carthage:master Nov 6, 2016


		extension Scannable {

		/// Attemps to create an instance of NSScaner and then use the convenience


		let formatting = ColorOptions.Formatting(true)

		carthage.println(formatting.bullets + "Please update to the latest Carthage version: \(carthageVersion.remoteVersion). You currently are on \(carthageVersion.localVersion)")


		func shouldUpgrade() ->Bool {

		if localVersion < remoteVersion {

Remote Version Validation #1533

Remote Version Validation #1533

Conversation

rodchile commented Oct 16, 2016 • edited

ikesyo commented Oct 16, 2016

mdiep commented Oct 16, 2016

mdiep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NachoSoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdiep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdiep commented Nov 6, 2016

rodchile commented Oct 16, 2016 •

edited