FoundationNetworking: Split networking into its own module #2289

millenomi · 2019-05-22T17:17:22Z

This patch introduces a new module, FoundationNetworking, which will include all classes related to loading from remote URLs. Several types have been moved to that module, which downstream clients must import explicitly, e.g.:

#if canImport(FoundationNetworking)
import FoundationNetworking
#endif

Split the CFURLSessionInterface files out of Core Foundation and into their own library, libCFURLSessionInterface. This is produced by the CF CMakeLists.txt file on non-Darwin platforms. Just like CF, we strongly discourage linking to this library directly.
Split URLSession and related classes (except for URL and URLComponents) into a new FoundationNetworking module (SwiftFoundationNetworking on Darwin). Note:

The features in Boxing.swift are needed to both modules. This file is compiled separately in both modules, and relies on a NS_BUILDING_FOUNDATION_NETWORKING compilation condition to compile correctly in FoundationNetworking
SPI that was used in InputStream is now available through a public struct named _InputStreamSPIForFoundationNetworkingUseOnly for FoundationNetworking to use. This isn’t API, much like _ObjectiveCBridgeable.

Also:

Change some details to make tests fail without a fatal error for issues that aren’t programmer errors.
In some circumstances, CURL can set an immediate timeout reentrantly if the timeout callback invokes CURL hooks. Always dispatch the CURL hook call asynchronously from a request for an immediate timeout.
Types that have a contentsOf url: URL initializer can still accept non-file: URLs if FoundationNetworking is available in the address space of the current process (if it has been linked or dynamically loaded, for example). Loading non-file: URLs without this will result in a fatal error. Loading file: URLs will always succeed even in the absence of FoundationNetworking.

This patch introduces a new module, FoundationNetworking, which will include all classes related to loading from remote URLs. Several types have been moved to that module, which downstream clients must import explicitly, e.g.: ``` #if canImport(FoundationNetworking) import FoundationNetworking #endif ``` - Split the CFURLSessionInterface files out of Core Foundation and into their own library, libCFURLSessionInterface. This is produced by the CF CMakeLists.txt file on non-Darwin platforms. Just like CF, we strongly discourage linking to this library directly. - Split URLSession and related classes (except for URL and URLComponents) into a new FoundationNetworking module (SwiftFoundationNetworking on Darwin). Note: 1. The features in Boxing.swift are needed to both modules. This file is compiled separately in both modules, and relies on a NS_BUILDING_FOUNDATION_NETWORKING compilation condition to compile correctly in FoundationNetworking 2. SPI that was used in InputStream is now available through a public struct named _InputStreamSPIForFoundationNetworkingUseOnly for FoundationNetworking to use. This isn’t API, much like _ObjectiveCBridgeable. Also: - Change some details to make tests fail without a fatal error for issues that aren’t programmer errors. - In some circumstances, CURL can set an immediate timeout reentrantly if the timeout callback invokes CURL hooks. Always dispatch the CURL hook call asynchronously from a request for an immediate timeout. - Types that have a `contentsOf url: URL` initializer can still accept non-file: URLs if FoundationNetworking is available in the address space of the current process (if it has been linked or dynamically loaded, for example). Loading non-file: URLs without this will result in a fatal error. Loading file: URLs will always succeed even in the absence of FoundationNetworking.

millenomi · 2019-05-22T17:19:15Z

@swift-ci please test

millenomi · 2019-05-22T17:21:51Z

CoreFoundation/CMakeLists.txt

  set_target_properties(CoreFoundation
                        PROPERTIES LINK_FLAGS
                          -Xlinker;-alias_list;-Xlinker;Base.subproj/DarwinSymbolAliases;-twolevel_namespace;-sectcreate;__UNICODE;__csbitmaps;CharacterSets/CFCharacterSetBitmaps.bitmap;-sectcreate;__UNICODE;__properties;CharacterSets/CFUniCharPropertyDatabase.data;-sectcreate;__UNICODE;__data;CharacterSets/CFUnicodeData-L.mapping;-segprot;__UNICODE;r;r)
+  set_target_properties(CFURLSessionInterface
+                        PROPERTIES LINK_FLAGS
+                          -Xlinker;-alias_list;-Xlinker;Base.subproj/DarwinSymbolAliases;-twolevel_namespace;-sectcreate;__UNICODE;__csbitmaps;CharacterSets/CFCharacterSetBitmaps.bitmap;-sectcreate;__UNICODE;__properties;CharacterSets/CFUniCharPropertyDatabase.data;-sectcreate;__UNICODE;__data;CharacterSets/CFUnicodeData-L.mapping;-segprot;__UNICODE;r;r)


This shouldn't have CFCharacterSet mappings.

compnerd · 2019-05-22T17:23:53Z

CMakeLists.txt

+                    ${CMAKE_C_COMPILER_TARGET}
+                  CFLAGS
+                    ${MSVCRT_C_FLAGS}
+                    ${deployment_target}


This should no longer be needed, deployment_target has been removed in favour of TARGET_OS_* from TargetConditionals.h

compnerd · 2019-05-22T17:25:11Z

CMakeLists.txt

+                  CFLAGS
+                    ${MSVCRT_C_FLAGS}
+                    ${deployment_target}
+                    ${deployment_enable_libdispatch}


I believe that I have cleaned this up as well. This was previously needed as there was a mode for CoreFoundation to be built without libdispatch which is no longer supported.

compnerd · 2019-05-22T17:28:52Z

CMakeLists.txt

+                    ${libdispatch_ldflags}
+                    -lFoundation
+                    ${Foundation_INTERFACE_LIBRARIES}
+					${CFURLSessionInterface_LIBRARIES}


Indentation seems odd

compnerd · 2019-05-22T17:29:03Z

CMakeLists.txt

+                  LINK_FLAGS
+                    ${MSVCRT_LINK_FLAGS}
+                    -L${CMAKE_CURRENT_BINARY_DIR}
+                    ${ICU_UC_LIBRARY} ${ICU_I18N_LIBRARY}


Does this part actually use ICU?

compnerd · 2019-05-22T17:29:24Z

CMakeLists.txt

+                    ${Foundation_RPATH}
+                    ${WORKAROUND_SR9138}
+                    ${WORKAROUND_SR9995}
+                    $<$<PLATFORM_ID:Windows>:$<TARGET_OBJECTS:CoreFoundationResources>>


Why do you need the CoreFoundationResources? That is for the TimeZone information

compnerd · 2019-05-22T17:29:31Z

CMakeLists.txt

+                    $<$<PLATFORM_ID:Windows>:$<TARGET_OBJECTS:CoreFoundationResources>>
+                  SWIFT_FLAGS
+                    -DDEPLOYMENT_RUNTIME_SWIFT
+					-DNS_BUILDING_FOUNDATION_NETWORKING


Indentation seems odd

compnerd · 2019-05-22T17:29:42Z

CMakeLists.txt

+                    -DDEPLOYMENT_RUNTIME_SWIFT
+					-DNS_BUILDING_FOUNDATION_NETWORKING
+                    ${deployment_enable_libdispatch}
+                    -I;${ICU_INCLUDE_DIR}


Do we really need ICU here?

compnerd · 2019-05-22T17:45:57Z

CoreFoundation/CMakeLists.txt

 endif()

 if("${CMAKE_C_SIMULATE_ID}" STREQUAL "MSVC")
  target_compile_options(CoreFoundation
                         PRIVATE
                           -fblocks
                           /EHsc)
+  target_compile_options(CFURLSessionInterface


Does CFURLSessionInterface use blocks? Or exceptions /EHsc enables exceptions

I feel like these two should be a baseline for all CF code, and will keep them as-is.

compnerd · 2019-05-22T17:46:32Z

CoreFoundation/CMakeLists.txt

 endif()
 if(CMAKE_SYSTEM_NAME STREQUAL Android)
  target_link_libraries(CoreFoundation
                        PRIVATE
                          log)
+  target_link_libraries(CFURLSessionInterface
+                        PRIVATE
+                          log)


Does CFURLSessionInterface use the android log apis?

compnerd · 2019-05-22T17:46:52Z

CoreFoundation/CMakeLists.txt

 endif()
 target_link_libraries(CoreFoundation
                      PRIVATE
                        Threads::Threads
                        ${CMAKE_DL_LIBS})
+target_link_libraries(CFURLSessionInterface


Does CFURLSessionInterface use threads and dl* functions?

compnerd · 2019-05-22T17:47:19Z

CoreFoundation/CMakeLists.txt

 if(CMAKE_SYSTEM_NAME STREQUAL Windows)
  target_link_libraries(CoreFoundation
                        PRIVATE
                          AdvAPI32
                          Secur32
                          User32
                          mincore)
+  target_link_libraries(CFURLSessionInterface


I think that you probably don't need to link against most of these libraries. WS2_32 and mincore are probably all that are needed?

I can't test Windows builds at the moment; is it okay if I leave them in and you can remove them later?

Sounds fine to me - unlike Unix, the semantics on Windows are that they will be dropped if unused anyways.

compnerd · 2019-05-22T17:47:33Z

CoreFoundation/CMakeLists.txt

  set_target_properties(CoreFoundation
                        PROPERTIES LINK_FLAGS
                          -Xlinker;-alias_list;-Xlinker;Base.subproj/DarwinSymbolAliases;-twolevel_namespace;-sectcreate;__UNICODE;__csbitmaps;CharacterSets/CFCharacterSetBitmaps.bitmap;-sectcreate;__UNICODE;__properties;CharacterSets/CFUniCharPropertyDatabase.data;-sectcreate;__UNICODE;__data;CharacterSets/CFUnicodeData-L.mapping;-segprot;__UNICODE;r;r)
+  set_target_properties(CFURLSessionInterface


This re-embeds the character set data, why?

millenomi · 2019-05-22T18:48:54Z

Collectively responding: I replicated the existing setup for CF line-for-line. Certain things are flat-out incorrect, like the character data; certain are not useful, like icu. I'll do a pass for all the objections.

millenomi · 2019-05-22T20:17:40Z

You are correct. Yay, sleep deprivation. Fixing.

millenomi · 2019-05-22T20:36:36Z

@compnerd I addressed everything that I could locally check would compile.

millenomi · 2019-05-22T20:36:52Z

@swift-ci please test

millenomi · 2019-05-22T20:37:03Z

@swift-ci please test

millenomi · 2019-05-22T22:22:04Z

@swift-ci please test

compnerd

I think that this seems good now.

compnerd · 2019-05-22T23:36:32Z

CMakeLists.txt

+                    uuid
+                    CoreFoundation
+                    $<$<PLATFORM_ID:Windows>:CoreFoundationResources>
+				    Foundation)


Indentation is off

Missed it; I'll fix it before merge.

jrose-apple · 2019-05-23T20:29:19Z

Foundation/NSObjCRuntime.swift

+
+@available(*, unavailable, message: "This type has moved to the FoundationNetworking module. Import that module to use it.",
+    renamed: "FoundationNetworking.CachedURLResponse")
+public typealias CachedURLResponse = AnyObject


Note: these should not use renamed:, because we want people to continue using the unqualified spelling to refer to them once they've imported FoundationNetworking. (That should still work without ambiguity because unavailable declarations should only be used if there's no valid available one.)

We're relying on the latter here.

In general, I cannot mandate a spelling be qualified or not; it is my job to make both work.

(but I agree we shouldn't be recommending to use the qualified one if possible.)

Right. I'm specifically talking about renamed: because that changes the error message and creates a fix-it.

…tions.

millenomi · 2019-05-24T18:00:08Z

@swift-ci please test and merge

millenomi · 2019-05-24T18:00:23Z

@swift-ci please test and merge

millenomi commented May 22, 2019

View reviewed changes

compnerd reviewed May 22, 2019

View reviewed changes

millenomi mentioned this pull request May 22, 2019

Implement CachedURLResponse. #2245

Merged

compnerd reviewed May 22, 2019

View reviewed changes

millenomi added 2 commits May 22, 2019 13:21

Add diagnostics for newly missing types in Foundation.

b3a3d01

Fix feedback the CMake setup.

d697422

compnerd reviewed May 22, 2019

View reviewed changes

jrose-apple reviewed May 23, 2019

View reviewed changes

millenomi added 2 commits May 24, 2019 10:36

Fix whitespace.

f9d93ec

Take feedback: Remove renamed: arguments to the availability annota…

db36644

…tions.

swift-ci merged commit 40244c8 into swiftlang:master May 24, 2019

JRHeaton mentioned this pull request Oct 12, 2019

Fix broken build on Linux/Docker Swift 5.1 SwiftyBeaver/SwiftyBeaver#360

Merged

miguelangel-dev mentioned this pull request Apr 27, 2020

Linux bow-swift/bow#591

Merged

This was referenced May 4, 2020

Linux bow-swift/bow-openapi#33

Merged

Linux bow-swift/nef-editor-server#14

Merged

jaredh mentioned this pull request Nov 18, 2020

Upgrade to Swift 5.3.1 Shopify/syrup#30

Merged

shoebox mentioned this pull request Jan 12, 2021

fix(linux support): add the missing import for linux massiveinteractive/SwagGen#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FoundationNetworking: Split networking into its own module #2289

FoundationNetworking: Split networking into its own module #2289

millenomi commented May 22, 2019 •

edited

Loading

millenomi commented May 22, 2019

millenomi May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

millenomi May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

millenomi May 22, 2019

compnerd May 22, 2019

compnerd May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

compnerd left a comment

compnerd May 22, 2019

millenomi May 23, 2019

jrose-apple May 23, 2019

millenomi May 24, 2019

millenomi May 24, 2019

millenomi May 24, 2019

jrose-apple May 24, 2019

millenomi commented May 24, 2019

millenomi commented May 24, 2019

FoundationNetworking: Split networking into its own module #2289

FoundationNetworking: Split networking into its own module #2289

Conversation

millenomi commented May 22, 2019 • edited Loading

millenomi commented May 22, 2019

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

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

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

millenomi commented May 22, 2019

compnerd 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

millenomi commented May 24, 2019

millenomi commented May 24, 2019

millenomi commented May 22, 2019 •

edited

Loading