Add plist enum to Xcodeproj #622

aciidgh · 2016-08-30T06:20:19Z

No description provided.

aciidgh · 2016-08-30T06:20:43Z

@abertelrud @ddunbar review pls

aciidgh · 2016-08-30T14:17:54Z

This PR also has unrelated minor bugfix for SR-2526 (last commit)

ddunbar · 2016-08-30T18:40:24Z

I think the bugifx should go in its own PR...

aciidgh · 2016-08-30T18:46:41Z

Moved to #624

abertelrud · 2016-08-30T21:25:04Z

Sources/Basic/StringConversions.swift

+        var newString = String(utf8[utf8.startIndex..<pos])!
+        for char in utf8[pos..<utf8.endIndex] {
+            if char == UInt8(ascii: ascii) {
+                newString += "\\" + String(UnicodeScalar(ascii))


This doesn't look wrong, exactly, but I wonder about the performance of using String concatenation. Can we count on this being optimized out, so that this doesn't result in a lot heap allocations?

aciidgh · 2016-09-01T07:29:18Z

@abertelrud Implemented your review suggestions, please re-review

abertelrud · 2016-09-01T19:55:19Z

Sources/Xcodeproj/Module+PBXProj.swift

+        switch self {
+        case .string(let str):
+            // Only quote if string has a space character.
+            guard str.utf8.contains(UInt8(ascii: " ")) else {


This isn't sufficient. You also have to add quotes and escapes if the string contains quote characters or escape characters. Maybe the safest is to always quote.

Always quoting rules specific to each build setting? because quoting some settings became invalid for eg Product Module became "Hello"

Perhaps I'm misunderstanding the purpose of this function. I was reacting to what seems like a problems in the code, which is that input strings containing, say, just a quote or a backslash end up broken.

Is this for build settings? If so, then the appropriate distinction is probably to use strings vs arrays, and not to quote some strings and not others.

I'll take a look at the rest of the diffs to see if I can come up with more actionable suggestions.

abertelrud · 2016-09-01T20:01:07Z

Looks good in general. There is one actually issue, in quoteStrings(), and the rest is just suggestions for improvements. Thanks!

aciidgh · 2016-09-01T20:34:43Z

@abertelrud Updated

abertelrud · 2016-09-01T20:49:42Z

Sources/Xcodeproj/Module+PBXProj.swift

+    func quoteStrings() -> Plist {
+        switch self {
+        case .string(let str):
+            return .string("\"" + str + "\"")


The string inside the quotes still needs to be escaped, otherwise it will be broken if it contains a quote or an escape character.

won't that be taken care when serialize() is called? or you mean it should be escaped twice?

Taking a step back, though, I guess I don't see the reason why there is this function at all when there is serialize for plists. Wouldn't this be quoting things twice?

Build settings that are arrays are a different thing. Is that what this is used for?

When I say "still needs to be escaped", I'm not talking about whether or not Xcode expects a certain level of quoting. I mean that in a very local sense, if str is equal to " here, then you will end up with an output (of just this function) that is """. That's broken.

The bigger issue of whether to quote at all is valid, but my comment was specifically about the correctness of this layer.

This is only for the build settings layer, for eg: for a string abc
this will quote to "abc" and then serialize() will make it "\"abc\""

oh I see, I think I understand

Build settings that are arrays are a different thing. Is that what this is used for?

Yup and currently only called for HEADER_SEARCH_PATH

To clarify a bit: there are a couple of different layers here. At the bottom is the conversion of the plist node tree to a sequence of bytes. There is quoting going on there to make sure that any type of string contents can be represented. And where there is quoting there is also escaping: by assigning special meaning to quotes, there becomes a need to escape quotes that shouldn't have special meaning. And then also to escape the escape so that it doesn't have special meaning either. :)

Then the next layer up is what's put into the plist nodes. If what is being put into a plist node is a string build setting value, then no quoting is needed. If it is an array build setting value, though, then it will need to be turned into a string by quoting each array element and joining the elements with whitespace. Note that this decision (of which build settings get quoted) does not depend on what characters are in the value (e.g. whitespace), but rather on whether the build setting is an array or a string.

So rather than have the business logic deal with plist node types, I would keep build settings as a mapping from build setting names to values (each of which is either a string or a string array).

Then when serializing you go through it; any value that's already a string is unchanged, and any value that's an array of strings is turned into a string by quoting each array element and joining with whitespace. And then wrapped in a .string() plist node.

abertelrud · 2016-09-02T16:52:08Z

Sources/Xcodeproj/Module+PBXProj.swift

@@ -259,13 +272,13 @@ extension Module  {
                // example would be `@executable_path/../lib` but there are
                // other problems to solve first, e.g. how to deal with the
                // Swift standard library paths).
-                buildSettings["LD_RUNPATH_SEARCH_PATHS"] = buildSettings["LD_RUNPATH_SEARCH_PATHS"]! + " @executable_path"
+                buildSettings["LD_RUNPATH_SEARCH_PATHS"] = [toolchainPath, "@executable_path"]


It's actually better to keep this as appending to the value of the LD_RUNPATH_SEARCH_PATHS setting, since that is the semantic action here. It happens that the toolchain path is the thing that's assigned above, so right now it works out the same in practice, but if anyone adds code in between to add something else to LD_RUNPATH_SEARCH_PATHS, that would end up getting stomped by this line of code.

abertelrud · 2016-09-02T16:55:40Z

Ok, I think that looks good (there are of course other refinements that can be made over time, but this is a good improvement). Thanks!

I'll use some of this plist support code in my upcoming checkin adding a model layer. Right now I have my own but plan to switch to your new code for that once it's in.

abertelrud · 2016-09-02T16:56:05Z

@aciidb0mb3r, if you want to kick off a test we can merge this when they pass.

Also, moves Build settings to use this new type respecting proper quoting rules. - <rdar://problem/28039632> SR-1754: Spaces should be escaped in HEADER_SEARCH_PATHS

aciidgh · 2016-09-02T16:58:00Z

@swift-ci please test

aciidgh · 2016-09-02T16:58:22Z

@abertelrud can you start swift-ci, it doesnt work for me anymore

abertelrud · 2016-09-02T16:58:59Z

@swift-ci please test

abertelrud · 2016-09-02T16:59:17Z

Odd... it seems to have worked for me. That might be worth looking into.

aciidgh · 2016-09-02T17:00:07Z

Thanks, I'll get in touch with someone who can look into this

aciidgh · 2016-09-02T19:29:12Z

Linux failed due to unrelated error, this PR is about Xcode proj generation which doesn't work on linux. Locally testing on linux and proceeding with merge

aciidgh force-pushed the escape-space branch from 1f86605 to 1aa1050 Compare August 30, 2016 19:21

abertelrud reviewed Aug 30, 2016
View reviewed changes

aciidgh force-pushed the escape-space branch 2 times, most recently from 5f71a12 to 9ae038e Compare September 1, 2016 07:28

aciidgh assigned abertelrud Sep 1, 2016

abertelrud reviewed Sep 1, 2016
View reviewed changes

aciidgh force-pushed the escape-space branch 2 times, most recently from 2f4e57a to 8887a6b Compare September 1, 2016 20:34

abertelrud reviewed Sep 1, 2016
View reviewed changes

aciidgh force-pushed the escape-space branch 3 times, most recently from 217d776 to 6bbd805 Compare September 1, 2016 22:31

abertelrud reviewed Sep 2, 2016
View reviewed changes

[Xcodeproj] Add a Plist enum

b1f2f6c

Also, moves Build settings to use this new type respecting proper quoting rules. - <rdar://problem/28039632> SR-1754: Spaces should be escaped in HEADER_SEARCH_PATHS

aciidgh force-pushed the escape-space branch from 6bbd805 to b1f2f6c Compare September 2, 2016 16:57

aciidgh merged commit 580d2d7 into swiftlang:master Sep 2, 2016

aciidgh deleted the escape-space branch September 2, 2016 19:29

aciidgh mentioned this pull request Sep 6, 2016

[SR-1754] Spaces should be escaped in HEADER_SEARCH_PATHS #5256

Closed

Add plist enum to Xcodeproj #622

Add plist enum to Xcodeproj #622

Uh oh!

Conversation

aciidgh commented Aug 30, 2016

Uh oh!

aciidgh commented Aug 30, 2016

Uh oh!

aciidgh commented Aug 30, 2016

Uh oh!

ddunbar commented Aug 30, 2016

Uh oh!

aciidgh commented Aug 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Sep 1, 2016

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!

abertelrud commented Sep 1, 2016

Uh oh!

aciidgh commented Sep 1, 2016

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!

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!

abertelrud commented Sep 2, 2016

Uh oh!

abertelrud commented Sep 2, 2016

Uh oh!

aciidgh commented Sep 2, 2016

Uh oh!

aciidgh commented Sep 2, 2016

Uh oh!

abertelrud commented Sep 2, 2016

Uh oh!

abertelrud commented Sep 2, 2016

Uh oh!

aciidgh commented Sep 2, 2016

Uh oh!

aciidgh commented Sep 2, 2016

Uh oh!

Uh oh!