'Fix' macOS build errors, TestXDG folder cleanup #988

e78l · 2017-05-18T07:43:30Z

'Fix' macOS build errors with swift compiler option -swift-version 3. Xcode seems to have a SWIFT_VERSION setting, but it doesn't have the same effect! Changes should be like those in Force use of Swift 3 compatibility mode to compile sources. #956.
Quiet some warnings with -Wno-nullability-completeness-on-arrays and -Wno-format-security for CoreFoundation - please review if GCC_WARN_TYPECHECK_CALLS_TO_PRINTF Xcode setting has the same effect/is better than -Wno-format-security
Has mangling changed on Mac? I've have had issues with comparing CF/NS strings, and not sure if it /was just/ a mangling issue, but got warning and error with macOS build, so updating __CFSwiftGetBaseClass and __CFConstantStringClassReference.
@slavapestov or @eeckstein - is there a way for a script to determine a mangled class/function name at build time?
The warning/error reported on macOS:

ld: warning: undefined base symbol '__TMC15SwiftFoundation19_NSCFConstantString' for alias '___CFConstantStringClassReference'
Undefined symbols for architecture x86_64:
  "__T015SwiftFoundation21__CFSwiftGetBaseClassyXlXpyF", referenced from:
      ___CFInitialize in libCoreFoundation.a(CFRuntime.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Refactored XDG helper, and created target in Xcode so tests pass on Mac (well 2 failures still in NSString):

TestNSString.test_FromContentsOfURLUsedEncodingUTF16BE : failed - URL for NSString-UTF16-BE-data.txt is nil
TestNSString.test_FromContentsOfURLUsedEncodingUTF16LE : failed - URL for NSString-UTF16-LE-data.txt is nil

Target depends on SwiftFoundation being at same level in build directory (added rpath in lieu of framework copy phase). Perhaps copy can be disabled for TestFoundation too?
@mamabusi Are these changes okay with you?
I am thinking, perhaps, xdgTestHelper can be renamed testHelper or similar and used for any other 'checks' needing another process in the future.

mattrajca · 2017-05-19T02:43:24Z

This fixed my recent macOS build failures. Thanks.

mamabusi · 2017-05-19T05:55:03Z

@e78l XDG Test executable changes look good to me. I second your opinion of renaming xdgTestHelper to a more generic name if it helps other tests needing another process.

parkera · 2017-05-19T22:41:30Z

I would really like to split up these fixes into a few different PRs; especially stuff about build settings vs warning fixes in CF.

parkera · 2017-05-19T22:44:12Z

Foundation.xcodeproj/project.pbxproj

+				CODE_SIGN_IDENTITY = "";
+				COMBINE_HIDPI_IMAGES = YES;
+				INFOPLIST_FILE = TestFoundation/xdgTestHelper/Info.plist;
+				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/../../.. @loader_path/../../.. @executable_path/../Frameworks";


This path is wacky. Is that really the right value?

I'm adding SwiftFoundation at the level of the build directory, so for the executable at:
Build/Products/Debug/xdgTestHelper.app/Contents/MacOS/xdgTestHelper
look for framework at:
Build/Products/Debug/SwiftFoundation.framework
Could also just copy the framework into @executable_path/../Frameworks but that seems unnecessary to me

This xdgTestHelper was only being built on Linux before, but this ports it as a .app to macOS right?

parkera · 2017-05-19T22:44:33Z

Foundation.xcodeproj/project.pbxproj

@@ -2652,6 +2722,8 @@
 					"-DCF_CHARACTERSET_UNICODE_DATA_B=\\\\\"CoreFoundation/CharacterSets/CFUnicodeData-B.mapping\\\\\"",
 					"-DCF_CHARACTERSET_UNICHAR_DB=\\\\\"CoreFoundation/CharacterSets/CFUniCharPropertyDatabase.data\\\\\"",
 					"-DCF_CHARACTERSET_BITMAP=\\\\\"CoreFoundation/CharacterSets/CFCharacterSetBitmaps.bitmap\\\\\"",
+					"-Wno-nullability-completeness-on-arrays",


Why did we need to turn these off, instead of maybe fixing the warnings?

Auditing CF for these warnings may be worthwhile, but I'm thinking it'll take quite a bit of time to fix+review. Meanwhile, there is a lot of noise; in particular, no-format-security flags CFSTRs which look fine.

parkera · 2017-05-19T22:45:30Z

TestFoundation/TestNSHTTPCookieStorage.swift

@@ -241,9 +241,14 @@ class TestNSHTTPCookieStorage: XCTestCase {
        let bundlePath = bundle.bundlePath
        let pathIndex = bundlePath.range(of: "/", options: .backwards)?.lowerBound
        let task = Process()
-        task.launchPath = bundlePath.substring(to: pathIndex!) + "/xdgTestHelper/xdgTestHelper"
+        #if os(macOS)


This should really just be using the Bundle API directly to find the correct path in the bundle, which would be platform independent.

Removed for now (tests on mac will crash)
xdgTestHelper is not in TestFoundation, so I would create Bundle with URL at path bundlePath.substring(to: pathIndex!) and then get executable path?

e78l · 2017-05-22T21:26:44Z

Hello @parkera,
Thanks for the review - I split some changes that were easy to do into #1006 and can split further+undo warnings silence if decided (would be a bit time consuming).
Removed Swift 4 changes which I think were covered by @spevans. After those changes, may be able to forget about -swift-version 3; in that case, may also want to update build.py.
@mamabusi Thanks also!

Add -swift-version 3 flag for compatibility

parkera · 2017-06-10T15:09:40Z

@swift-ci test

parkera · 2017-06-10T15:12:25Z

For some reason status isn't being updated here after request (cc @shahmishal @erg) but the PR is testing here: https://ci.swift.org/job/swift-corelibs-foundation-PR-Linux/872/

parkera · 2017-06-10T15:34:14Z

Looks like it passed. Thanks!

e78l force-pushed the macos-parity branch from 002cae8 to 95b0b6f Compare May 18, 2017 09:08

parkera reviewed May 19, 2017

View reviewed changes

pushkarnk mentioned this pull request May 22, 2017

Small cleanup: Use () -> ... instead of (Void) -> ... for no-argument closures #984

Merged

e78l force-pushed the macos-parity branch 2 times, most recently from 0f39ab1 to 509c054 Compare May 22, 2017 20:59

e78l mentioned this pull request May 22, 2017

Update Swift-mangled NSCFConstantString class name #1006

Merged

e78l force-pushed the macos-parity branch from f4e13ab to d23ede6 Compare May 23, 2017 20:46

e78l added 2 commits May 24, 2017 17:17

Specify -swift-version 3 and add TestXDG target for macOS

607c91f

Add -swift-version 3 flag for compatibility

Delete TestXDG directory after TestNSHTTPCookieStorage

3ee91d6

e78l force-pushed the macos-parity branch from d23ede6 to 3ee91d6 Compare May 24, 2017 21:17

parkera added 2 commits June 10, 2017 08:03

Merge branch 'master' into macos-parity

0c8fd1b

Merge branch 'master' into macos-parity

6412b1d

parkera changed the title ~~'Fix' macOS build errors, explicit tuple members (Swift 4), TestXDG folder cleanup~~ 'Fix' macOS build errors, TestXDG folder cleanup Jun 10, 2017

parkera merged commit eaf7dd3 into apple:master Jun 10, 2017

e78l deleted the macos-parity branch June 10, 2017 18:32

spevans mentioned this pull request Oct 26, 2017

Upgrade xcode project to latest Recommended Settings #1280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

'Fix' macOS build errors, TestXDG folder cleanup #988

'Fix' macOS build errors, TestXDG folder cleanup #988

e78l commented May 18, 2017

mattrajca commented May 19, 2017

mamabusi commented May 19, 2017

parkera commented May 19, 2017

parkera May 19, 2017

e78l May 22, 2017 •

edited

parkera May 23, 2017

e78l May 23, 2017

parkera May 19, 2017

e78l May 22, 2017

parkera May 19, 2017

e78l May 22, 2017

e78l commented May 22, 2017

parkera commented Jun 10, 2017

parkera commented Jun 10, 2017

parkera commented Jun 10, 2017

'Fix' macOS build errors, TestXDG folder cleanup #988

'Fix' macOS build errors, TestXDG folder cleanup #988

Conversation

e78l commented May 18, 2017

mattrajca commented May 19, 2017

mamabusi commented May 19, 2017

parkera commented May 19, 2017

parkera May 19, 2017

Choose a reason for hiding this comment

e78l May 22, 2017 • edited

Choose a reason for hiding this comment

parkera May 23, 2017

Choose a reason for hiding this comment

e78l May 23, 2017

Choose a reason for hiding this comment

parkera May 19, 2017

Choose a reason for hiding this comment

e78l May 22, 2017

Choose a reason for hiding this comment

parkera May 19, 2017

Choose a reason for hiding this comment

e78l May 22, 2017

Choose a reason for hiding this comment

e78l commented May 22, 2017

parkera commented Jun 10, 2017

parkera commented Jun 10, 2017

parkera commented Jun 10, 2017

e78l May 22, 2017 •

edited