Don't require xcodegen settings preset files. All configuration shoul… #84

rahul-malik · 2018-10-16T17:12:40Z

…d be

specified by xchammer.[yaml|yml] files

jerrymarino · 2018-10-16T22:41:39Z

Sources/XCHammer/XCBuildSettings.swift

 struct XCBuildSettings: Encodable {
    var copts: [String] = []
-    var productName: First<String>?
+    var productName: First<String>? = First("$(TARGET_NAME)")


So if we allocate this, it sets it to target name as the default? If we want to always set it to TARGET_NAME we should just remove the variable if it isn't necessary

jerrymarino · 2018-10-16T22:42:01Z

Sources/XCHammer/XcodeTarget.swift

@@ -454,10 +454,13 @@ public class XcodeTarget: Hashable, Equatable {
            return xcTargetName + ".xctest"
            case .AppExtension, .XPCService, .Watch1App, .Watch2App, .Watch1Extension, .Watch2Extension, .TVAppExtension:
            return xcTargetName + ".appex"
-            case .StaticLibrary:
+            case .StaticLibrary, .Tool:


Is this related?

Nope but it's correct

jerrymarino · 2018-10-16T22:42:06Z

Sources/XCHammer/XcodeTarget.swift

            default:
-            fatalError()
+            print("ERR: Product type \(productType)")


jerrymarino · 2018-10-16T22:42:47Z

Sources/XCHammer/XcodeTarget.swift

            return xcTargetName
+            case .Bundle:


Do we actually need this one? I thought bundles weren't built

I actually hit this case

jerrymarino · 2018-10-16T22:43:44Z

Sources/XCHammer/XcodeTarget.swift

        settings.productName <>= self.bundleName.map { First($0) }

+        if settings.productName == nil {
+            settings.productName = First(self.extractBuiltProductName())


Would this code path ever be hit? It doesn't seem like it if you set the value in the initializer?

it will now that there isn't a default value

jerrymarino · 2018-10-17T19:06:37Z

Sources/XCHammer/XCBuildSettings.swift

@@ -163,6 +166,11 @@ struct XCBuildSettings: Encodable {
    var useHeaderMap: First<String>? = First("NO")
    var testTargetName: First<String>?
    var pythonPath: First<String>?
+    var sdkRoot: First<String>?
+    var targetedDeviceFamily: OrderedArray<String> = OrderedArray.empty
+


Minor: remove spaces?

kastiglione · 2018-10-17T20:38:42Z

Sources/XCHammer/XCBuildSettings.swift

@@ -163,6 +166,11 @@ struct XCBuildSettings: Encodable {
    var useHeaderMap: First<String>? = First("NO")
    var testTargetName: First<String>?
    var pythonPath: First<String>?
+    var sdkRoot: First<String>?
+    var targetedDeviceFamily: OrderedArray<String> = OrderedArray.empty


What's an OrderedArray?

Ordered array is poorly named, it's basically an implementation of ordered set that conform to a few protocols used to serialize build settings. We wanted uniqueness with consistent ordering which is why this was originally written, happy to revisit

kastiglione · 2018-10-17T20:39:13Z

Didn't mean to accept, just meant to leave a comment.

kastiglione · 2018-10-17T20:39:48Z

What's the deal with all the scheme changes and developer machine specific path changes?

…d be specified by xchammer.[yaml|yml] files - Update sample project - Make Product name fallback the target name - Add target device families and sdk root for ios, watchos, tvos, macos

rahul-malik · 2018-10-17T23:14:40Z

@kastiglione - Filed tickets to address the scheme changes and dev machine paths

jerrymarino · 2018-10-18T00:06:19Z

@rahul-malik - there is a ton of errors on the CI and locally from XcodeGen

XCHammer: ** Completed read_aspects in 20.9972s
XCHammer: ** Completed query_genfiles in 2.2947s
XCHammer: Generating project UrlGet
XCHammer: ** Completed compute_deps_hash in 0.3823s
XCHammer: Converting to XcodeGen specification
Unknown product type: genrule. Unable to extract xcode equivalent
Unknown product type: swift_runtime_linkopts. Unable to extract xcode equivalent
Unknown product type: apple_binary. Unable to extract xcode equivalent
Unknown product type: entitlements. Unable to extract xcode equivalent
Unknown product type: apple_binary. Unable to extract xcode equivalent
Unknown product type: entitlements. Unable to extract xcode equivalent
Unknown product type: objc_bundle. Unable to extract xcode equivalent
Unknown product type: swift_runtime_linkopts. Unable to extract xcode equivalent
Unknown product type: swift_runtime_linkopts. Unable to extract xcode equivalent
Unknown product type: apple_binary. Unable to extract xcode equivalent
Unknown product type: _ios_test_bundle. Unable to extract xcode equivalent
Unknown product type: apple_binary. Unable to extract xcode equivalent
Unknown product type: filegroup. Unable to extract xcode equivalent
Unknown product type: swift_runtime_linkopts. Unable to extract xcode equivalent
Unknown product type: entitlements. Unable to extract xcode equivalent
Unknown product type: _ios_test_bundle. Unable to extract xcode equivalent
Unknown product type: swift_runtime_linkopts. Unable to extract xcode equivalent
Unknown product type: apple_binary. Unable to extract xcode equivalent
Unknown product type: swift_runtime_linkopts. Unable to extract xcode equivalent
Unknown product type: apple_binary. Unable to extract xcode equivalent

rahul-malik · 2018-10-18T00:07:23Z

I added that print statement, it’s not an error

jerrymarino · 2018-10-18T00:11:49Z

Ah ok - can you submit a followup to remove? People might be confused because we support all of the things that are emitted as unknown product type. The function extractProductType() is only for Bazel notions that map to Xcode Targets, which many don't.

should be (#84)" This reverts commit ed81cc2. This commit caused build errors in Pinterest that weren't tested in the OSS CI. This PR is simply documentation of the issue. The outcome of addresssing this PR is to understand _why_ it happened, and finding the best solution to address it.

rahul-malik requested a review from jerrymarino October 16, 2018 17:12

rahul-malik force-pushed the rmalik-remove-presets branch 2 times, most recently from 6f76738 to 88b10b6 Compare October 16, 2018 22:20

jerrymarino reviewed Oct 16, 2018

View reviewed changes

jerrymarino reviewed Oct 17, 2018

View reviewed changes

rahul-malik force-pushed the rmalik-remove-presets branch from 54583d3 to 01c56c5 Compare October 17, 2018 20:20

jerrymarino approved these changes Oct 17, 2018

View reviewed changes

kastiglione approved these changes Oct 17, 2018

View reviewed changes

rahul-malik force-pushed the rmalik-remove-presets branch 2 times, most recently from 82e57f0 to cf35e3e Compare October 17, 2018 22:00

Don't require xcodegen settings preset files. All configuration shoul…

2dfaa57

…d be specified by xchammer.[yaml|yml] files - Update sample project - Make Product name fallback the target name - Add target device families and sdk root for ios, watchos, tvos, macos

rahul-malik force-pushed the rmalik-remove-presets branch from cf35e3e to 2dfaa57 Compare October 17, 2018 22:48

rahul-malik merged commit ed81cc2 into master Oct 17, 2018

rahul-malik deleted the rmalik-remove-presets branch October 17, 2018 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't require xcodegen settings preset files. All configuration shoul… #84

Don't require xcodegen settings preset files. All configuration shoul… #84

rahul-malik commented Oct 16, 2018

jerrymarino Oct 16, 2018

jerrymarino Oct 16, 2018

rahul-malik Oct 17, 2018

jerrymarino Oct 16, 2018

rahul-malik Oct 17, 2018

jerrymarino Oct 16, 2018

rahul-malik Oct 17, 2018

jerrymarino Oct 16, 2018

rahul-malik Oct 17, 2018

jerrymarino Oct 17, 2018

kastiglione Oct 17, 2018

rahul-malik Oct 17, 2018

kastiglione commented Oct 17, 2018

kastiglione commented Oct 17, 2018

rahul-malik commented Oct 17, 2018

jerrymarino commented Oct 18, 2018

rahul-malik commented Oct 18, 2018

jerrymarino commented Oct 18, 2018

Don't require xcodegen settings preset files. All configuration shoul… #84

Don't require xcodegen settings preset files. All configuration shoul… #84

Conversation

rahul-malik commented Oct 16, 2018

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

kastiglione commented Oct 17, 2018

kastiglione commented Oct 17, 2018

rahul-malik commented Oct 17, 2018

jerrymarino commented Oct 18, 2018

rahul-malik commented Oct 18, 2018

jerrymarino commented Oct 18, 2018