Add deployment target #205

yonaskolb · 2017-12-22T06:33:04Z

This changes how platform versions are applied.

removed all platform versions from the setting presets. This lets Xcode determine the latest version
added options.platformVersions to set project level versions
added target.platformVersion to set application specific version

options:
  platformVersions:
    watchOS: 3.0
    tvOS: 11.0
targets:
  App_iOS:
    type: application
    platform: iOS
    platformVersion: 10.0

These new options just apply default build settings, any any custom settings will still override this.

This is technically a breaking change, as those projects relying on the old default versions set in the setting presets will now have the latest platform version, which for example upgrade iOS 10 to iOS 11.1 (depending on Xcode version)

EDIT: renamed to deploymentTarget

yonaskolb · 2017-12-22T06:47:39Z

Is sdkVersion a better name than platformVersion

alvarhansen · 2017-12-22T07:40:32Z

I think platformVersion is ok. sdkVersion should be avoided as it may create confusion with SDKROOT build setting.

SDKROOT = iphoneos11.2;
IPHONEOS_DEPLOYMENT_TARGET = 11.2;

yonaskolb · 2017-12-22T10:01:25Z

@allu22 Yeah fair enough. Another possible name would be deploymentTarget to match Xcode's UI terminology.

Also the properties on options.settingPresets could potentially just live directly on options with names like iOSVersion (or iOSDeploymentTarget). That means it's a bit cleaner if a spec just has one platform

toshi0383 · 2017-12-22T11:41:18Z

Sources/ProjectSpec/ProjectSpec.swift

@@ -51,7 +52,7 @@ public struct ProjectSpec {
            }
        }

-        public init(carthageBuildPath: String? = nil, createIntermediateGroups: Bool = false, bundleIdPrefix: String? = nil, settingPresets: SettingPresets = .all, developmentLanguage: String? = nil, indentWidth: Int? = nil, tabWidth: Int? = nil, usesTabs: Bool? = nil, xcodeVersion: String? = nil) {
+        public init(carthageBuildPath: String? = nil, createIntermediateGroups: Bool = false, bundleIdPrefix: String? = nil, settingPresets: SettingPresets = .all, developmentLanguage: String? = nil, indentWidth: Int? = nil, tabWidth: Int? = nil, usesTabs: Bool? = nil, xcodeVersion: String? = nil, platformVersions: PlatformVersions = .init()) {


I think you should consider adding more new-lines.
It's hard for reviewers to scroll horizontally to see what has changed.

+1, is there a swift format setting to do this automatically?

Will do 👍

toshi0383 · 2017-12-22T11:42:32Z

Tests/Fixtures/TestProject/Project.xcodeproj/project.pbxproj

@@ -953,6 +948,7 @@
 				SWIFT_OPTIMIZATION_LEVEL = "-Owholemodule";
 				SWIFT_VERSION = 4.0;
 				VALIDATE_PRODUCT = YES;
+				WATCHOS_DEPLOYMENT_TARGET = 2.0;


Is this on purpose?

Yeah, the TestProject spec adds this

toshi0383

LGTM👌

rahul-malik · 2017-12-22T20:56:13Z

Sources/ProjectSpec/PlatformVersions.swift

+        return lhs.iOS == rhs.iOS &&
+            lhs.tvOS == rhs.tvOS &&
+            lhs.watchOS == rhs.watchOS &&
+            lhs.macOS == rhs.macOS


This will probably be fine but will fail for instances where you might compare "11" and "11.0" since we are dealing with string equality vs a structured representation of a semantic version

That's true. We could potentially implement a proper Version type (or use a dependency, like Swift PM), but at the moment it's probably fine

I think you could copy this implementation that I made for Sake https://github.com/xcodeswift/sake/blob/master/Sources/SakefileUtils/Version.swift that has the equatable defined. That'd solve this issue easily.

Done. One change I had to make was have a getter on that version type that returns an Apple deployment version, as they use 9.0 not 9.0.0

rahul-malik · 2017-12-22T20:57:50Z

Sources/ProjectSpec/PlatformVersions.swift

+    public init(jsonDictionary: JSONDictionary) throws {
+
+        func parseVersion(_ platform: String) -> String? {
+            if let string: String = jsonDictionary.json(atKeyPath: .key(platform)) {


Should we just have the version input type as String for simplicity?

I just wanted to support the easy format of just iOS: 11.1 without wrapping it in a string

rahul-malik · 2017-12-22T21:00:27Z

Tests/XcodeGenKitTests/SpecLoadingTests.swift

+                "bundleIdPrefix": "com.test",
+                "createIntermediateGroups": true,
+                "developmentLanguage": "ja",
+                "platformVersions": ["iOS": 11.1, "tvOS": 10.0, "watchOS": "3.0", "macOS": "10.12" ]


Might be good for the test to have the case where we have 3 components to the version ("10.12.1" for example)

yonaskolb · 2017-12-23T04:30:30Z

I think I will change this to deploymentTarget. Any objections?

pepicrft · 2017-12-23T08:29:43Z

Sources/ProjectSpec/PlatformVersions.swift

+
+extension Platform {
+
+    public var versionBuildSetting: String {


I think it'd be great to unit test that this map is properly done.

pepicrft

Just a minor comment. Nice addition @yonaskolb

yonaskolb requested review from kastiglione, keith, bkase, pepicrft, rahul-malik, ryohey and toshi0383 December 22, 2017 06:34

yonaskolb force-pushed the platform_version branch from 8854ea2 to 4472ff3 Compare December 22, 2017 09:29

yonaskolb force-pushed the platform_version branch 2 times, most recently from 55763fb to 1379733 Compare December 22, 2017 10:55

toshi0383 reviewed Dec 22, 2017

View reviewed changes

toshi0383 approved these changes Dec 22, 2017

View reviewed changes

rahul-malik reviewed Dec 22, 2017

View reviewed changes

pepicrft reviewed Dec 23, 2017

View reviewed changes

pepicrft approved these changes Dec 23, 2017

View reviewed changes

yonaskolb changed the title ~~Add platform version~~ Add deployment target Dec 26, 2017

yonaskolb added 7 commits December 26, 2017 17:36

remove platform versions from setting presets

cd07453

add options.platformVersions and target.platformVersion

f0486c6

add platform version docs

7ce3dba

add platform version tests

09620aa

add yaml format in ProjectSpec docs

a1e46eb

Add Version type

4a90d33

rename platformVersion to deploymentTarget

cd6d179

yonaskolb force-pushed the platform_version branch from 406be17 to cd6d179 Compare December 26, 2017 09:37

rename deploymentTargets to deploymentTarget

e3a6739

yonaskolb merged commit 554a9d2 into master Dec 26, 2017

yonaskolb deleted the platform_version branch December 26, 2017 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deployment target #205

Add deployment target #205

yonaskolb commented Dec 22, 2017 •

edited

Loading

yonaskolb commented Dec 22, 2017 •

edited

Loading

alvarhansen commented Dec 22, 2017

yonaskolb commented Dec 22, 2017 •

edited

Loading

toshi0383 Dec 22, 2017

rahul-malik Dec 22, 2017

yonaskolb Dec 26, 2017

toshi0383 Dec 22, 2017

yonaskolb Dec 22, 2017

toshi0383 left a comment

rahul-malik Dec 22, 2017

yonaskolb Dec 23, 2017

pepicrft Dec 23, 2017

yonaskolb Dec 26, 2017

rahul-malik Dec 22, 2017

yonaskolb Dec 23, 2017

rahul-malik Dec 22, 2017

yonaskolb commented Dec 23, 2017

pepicrft Dec 23, 2017

pepicrft left a comment


		extension Platform {

		public var versionBuildSetting: String {

Add deployment target #205

Add deployment target #205

Conversation

yonaskolb commented Dec 22, 2017 • edited Loading

yonaskolb commented Dec 22, 2017 • edited Loading

alvarhansen commented Dec 22, 2017

yonaskolb commented Dec 22, 2017 • edited Loading

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

toshi0383 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

yonaskolb commented Dec 23, 2017

Choose a reason for hiding this comment

pepicrft left a comment

Choose a reason for hiding this comment

yonaskolb commented Dec 22, 2017 •

edited

Loading

yonaskolb commented Dec 22, 2017 •

edited

Loading

yonaskolb commented Dec 22, 2017 •

edited

Loading