[SR-6829] Add "DEBUG" to Xcode debug configuration #1518

krzyzanowskim · 2018-03-04T19:23:17Z

https://bugs.swift.org/browse/SR-6829

Add "DEBUG" to SWIFT_ACTIVE_COMPILATION_CONDITIONS for generated Xcode project.

The additional changes allows to set the values on the model level, rather than on the project generation level, like I tried here: 9082167

aciidgh

Thanks a lot! This looks good, I left some minor comments. Please also add a basic unit test.

aciidgh · 2018-03-04T19:33:59Z

Sources/Xcodeproj/pbxproj().swift

@@ -164,6 +164,7 @@ func xcodeProject(
    projectSettings.debug.GCC_OPTIMIZATION_LEVEL = "0"
    projectSettings.debug.ONLY_ACTIVE_ARCH = "YES"
    projectSettings.debug.SWIFT_OPTIMIZATION_LEVEL = "-Onone"
+    projectSettings.debug.SWIFT_ACTIVE_COMPILATION_CONDITIONS += ["DEBUG"]


We should add this define in SwiftPM's build system too. This can go somewhere around here https://github.com/apple/swift-package-manager/blob/master/Sources/Build/BuildPlan.swift#L306

This should be:

projectSettings.debug.SWIFT_ACTIVE_COMPILATION_CONDITIONS = ["$(inherited)", "DEBUG"]

I guess the $(inherited) applies to the OTHER_SWIFT_FLAGS too?

Looks like OTHER_SWIFT_FLAGS already has that entry https://github.com/apple/swift-package-manager/blob/master/Sources/Xcodeproj/pbxproj().swift#L454

I think we should move -DXcode from OTHER_SWIFT_FLAGS to this build setting but that is orthogonal to this PR.

aciidgh · 2018-03-04T19:39:39Z

Sources/Xcodeproj/XcodeProjectModelSerialization.swift

@@ -431,7 +431,11 @@ fileprivate func combineBuildSettingsPropertyLists(
        // FIXME: We should resolve `$(inherited)` here.  The way this works is
        // that occurrences of `$(inherited)` in the overlay are replaced with
        // the overlaid value in the base.
-        resultDict[name] = value
+        if let prevValue = resultDict[name] {


I think this shouldn't be needed if we use $(inherited) in the target specific build settings. See comment below.

The FIXME's says $(inherited) doesn't work. This change makes a step in that direction though so the values are combined rather than replaced.

I am not sure if we should be resolving build settings that come from project settings. But I've not really worked in this area much. @abertelrud should be able to explain!

I think, for now, lets leave this area unchanged as using $(inherited) seems like the right solution.

maybe. The story behind this change is here: 9082167

although we "combine" common and debug BuildSettings, it's not really combined, it's replaced.

I'll revert this.

krzyzanowskim · 2018-03-06T10:55:24Z

@aciidb0mb3r thanks! feedback adressed. back to you.

aciidgh · 2018-03-06T16:14:23Z

Sources/Xcodeproj/XcodeProjectModelSerialization.swift

@@ -574,6 +574,34 @@ public enum PropertyList {
    case string(String)
    case array([PropertyList])
    case dictionary([String: PropertyList])
+
+    static func + (lhs: PropertyList, rhs: PropertyList) -> PropertyList {


This is no longer required, right?

no longer. I still think the serialization FIXME should be addressed, but agree it's not necessarily with this PR

aciidgh · 2018-03-06T16:15:54Z

Sources/Build/BuildPlan.swift

+        switch buildParameters.configuration {
+        case .debug:
+            compilationConditions += ["-DDEBUG"]
+        default:


Instead of default, can you add the explicit .release case? This is helpful when we're adding new cases to the enum. The compiler will produce an error and we will be forced to rethink this switch case.

aciidgh · 2018-03-06T16:16:59Z

Sources/Build/BuildPlan.swift

+
+        switch buildParameters.configuration {
+        case .debug:
+            compilationConditions += ["-DDEBUG"]


Hm, I wonder if we should also add this define to C language targets. /cc @ddunbar

Tha makes sense. Should go to the GCC_PREPROCESSOR_DEFINITIONS (that's where the Xcode put it for newly created projects)

We should add -DDEBUG=1 here as well: https://github.com/apple/swift-package-manager/blob/master/Sources/Build/BuildPlan.swift#L198

along with the -DSWIFT_PACKAGE=1 or... is it set already?

…iguration.

krzyzanowskim · 2018-03-15T19:45:08Z

@aciidb0mb3r thank! back to you.

aciidgh · 2018-03-20T21:27:52Z

Sources/Xcodeproj/pbxproj().swift

@@ -162,8 +162,10 @@ func xcodeProject(
    projectSettings.debug.DEBUG_INFORMATION_FORMAT = "dwarf"
    projectSettings.debug.ENABLE_NS_ASSERTIONS = "YES"
    projectSettings.debug.GCC_OPTIMIZATION_LEVEL = "0"
+    projectSettings.debug.GCC_PREPROCESSOR_DEFINITIONS = ["DEBUG=1", "$(inherited)"]


This should be projectSettings.debug.GCC_PREPROCESSOR_DEFINITIONS = ["$(inherited)", "DEBUG=1"]

I KNOW. but... it's the other way around in the newly created Xcode project.

🤷‍♂️okay then

aciidgh · 2018-03-20T21:30:07Z

Sources/Build/BuildPlan.swift

+
+        switch buildParameters.configuration {
+        case .debug:
+            compilationConditions += ["-DDEBUG"]


We should add -DDEBUG=1 here as well: https://github.com/apple/swift-package-manager/blob/master/Sources/Build/BuildPlan.swift#L198

aciidgh · 2018-03-20T22:42:11Z

@swift-ci please smoke test

aciidgh · 2018-03-21T00:16:20Z

Failures look related

krzyzanowskim · 2018-03-21T16:17:49Z

@swift-ci please smoke test

aciidgh · 2018-03-21T16:19:31Z

@swift-ci please smoke test

aciidgh · 2018-03-21T17:39:32Z

@swift-ci please smoke test linux

aciidgh · 2018-03-21T17:42:07Z

@swift-ci please smoke test Linux

SWIFT_ACTIVE_COMPILATION_CONDITIONS is a list

ed25e3c

aciidgh reviewed Mar 4, 2018

View reviewed changes

aciidgh reviewed Mar 6, 2018

View reviewed changes

krzyzanowskim added 5 commits March 15, 2018 20:08

Add DEBUG value to SWIFT_ACTIVE_COMPILATION_CONDITIONS for Debug conf…

e7d4292

…iguration.

Set debug flag during project generation.

3006531

Test SWIFT_ACTIVE_COMPILATION_CONDITIONS serialization

681202b

Add explicit release case

232c382

Add DEBUG to projectSettings.debug.GCC_PREPROCESSOR_DEFINITIONS

b855418

aciidgh reviewed Mar 20, 2018

View reviewed changes

Add activeCompilationConditions to the Clang targets

a964e17

aciidgh approved these changes Mar 20, 2018

View reviewed changes

Update BuildPlan tests.

cfe4871

aciidgh merged commit 7d68f81 into swiftlang:master Mar 21, 2018

Yasumoto mentioned this pull request Aug 23, 2018

[SR-8108] CHANGELOG for Swift 4.1 and 4.2 from commit logs. #1755

Merged

krzyzanowskim mentioned this pull request Mar 4, 2018

[SR-6829] Expose configuration as compilation condition #4840

Closed

[SR-6829] Add "DEBUG" to Xcode debug configuration #1518

[SR-6829] Add "DEBUG" to Xcode debug configuration #1518

Uh oh!

Conversation

krzyzanowskim commented Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aciidgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krzyzanowskim Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krzyzanowskim Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krzyzanowskim commented Mar 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krzyzanowskim commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Mar 20, 2018

Uh oh!

aciidgh commented Mar 21, 2018

Uh oh!

krzyzanowskim commented Mar 21, 2018

Uh oh!

aciidgh commented Mar 21, 2018

Uh oh!

aciidgh commented Mar 21, 2018

Uh oh!

aciidgh commented Mar 21, 2018

Uh oh!

Uh oh!

krzyzanowskim commented Mar 4, 2018 •

edited

Loading

krzyzanowskim Mar 4, 2018 •

edited

Loading

krzyzanowskim Mar 4, 2018 •

edited

Loading

aciidgh Mar 6, 2018 •

edited

Loading