Avoid FoundationNetworking and libcurl on non-Darwin platforms #681
Conversation
`Data(contentsOf:)` on non-Darwin platforms comes from the `FoundationNetworking` module, which pulls in a massive amount of transitive dependencies of libcurl. That makes it harder to redistribute a statically linked executable binary of DocC.
MaxDesiatov
requested review from
compnerd,
d-ronnqvist,
ethan-kusters and
franklinsch
MaxDesiatov
added
the
enhancement
|
@swift-ci test |
| @@ -19,7 +19,7 @@ let swiftSettings: [SwiftSetting] = [ | |||
| let package = Package( | |||
| name: "SwiftDocC", | |||
| platforms: [ | |||
| .macOS(.v10_15), | |||
| .macOS(.v11), | |||
There was a problem hiding this comment.
Bumped to .v11 per a discussion with @franklinsch, this allows us to use throwing FileHandle.readToEnd() unlike the unsafe non-throwing FileHandle.readDataToEndOfFile that crashes the test suite on Linux.
| @@ -90,7 +90,7 @@ public class FileServer { | |||
| xlog("Tried to load an invalid path: \(path).\nFalling back to serve index.html.") | |||
| } | |||
| mimeType = "text/html" | |||
| data = self.data(for: path.appendingPathComponent("/index.html")) | |||
| data = self.data(for: "/index.html") | |||
There was a problem hiding this comment.
Can you explain why this isn't a change in behavior? What's needless about this?
There was a problem hiding this comment.
Only path component of url that data(for:) took as argument was used. The rest of the components are ignored by DocC server implementations.
Additionally, HTTPRequest unlike URLRequest doesn't store full URLs, only paths. The authority and scheme components can be assembled from scheme and authority properties on HTTPRequest , but those aren't used anywhere in the DocC codebase, other than a test in this diff below where we have to construct a new URLRequest to pass it to WKWebView on Darwin platforms.
There was a problem hiding this comment.
But if request.path is "/something" then this used to pass "/something/index.html" as an argument but now it only passes "index.html". Isn't that returning data for the wrong path?
There was a problem hiding this comment.
If request.path is "/something" then that is handled by the if branch above, no this else branch. The latter branch didn't use request.path previously, that logic is unchanged, as verified by the test suite.
|
@swift-ci test |
| public func response(to request: URLRequest) -> (URLResponse, Data?) { | ||
| guard let url = request.url else { | ||
| return (HTTPURLResponse(url: baseURL, statusCode: 400, httpVersion: "HTTP/1.1", headerFields: nil)!, nil) | ||
| public func response(to request: HTTPRequest) -> (HTTPTypes.HTTPResponse, Data?) { |
There was a problem hiding this comment.
This is a source breaking change. Is there anywhere to maintain both declarations to ease this transition?
There was a problem hiding this comment.
I can guard the old function that uses URLRequest and URLResponse on #if canImport(Darwin) if that's ok? Otherwise its presence on non-Darwin platforms adds a dependency on FoundationNetworking with curl and the rest of the system networking/crypto libraries, which is what this PR was meant to remove in the first place.
There was a problem hiding this comment.
I'm not sure. I'm concerned that someone could have an existing setup on Linux with curl and the rest of the system networking libraries and that their setup would break when they update to a new DocC version.
We try to provide a transition period for breaking changes. If that's not possible then so be it (although we would try to inform people about it ahead of time in the Forums). But it there's a way to stage this, then I think that it would be less disruptive that way.
Would it be possible to use a compile time define to exclude FoundationNetworking? That way we could have a 3 stage transition (first opt-in to exclude, then exclude by default, and finally remove it completely)
Co-authored-by: David Rönnqvist <[email protected]>
| let fileHandle = try FileHandle(forReadingFrom: url) | ||
| guard let data = try fileHandle.readToEnd() else { | ||
| throw FileSystemError.noDataReadFromFile(path: url.path) | ||
| } |
There was a problem hiding this comment.
If we have to do this instead of Data(contentsOf:) everywhere in the future then I'm concerned that either we or someone new to the repo would forget and break things.
Is there anyway that we can shim Data(contentsOf:) on non-Darwin platforms to do this or any way that we could mark Data(contentsOf:) as unavailable so that we don't accidentally break things in the future?
There was a problem hiding this comment.
I'm not aware of a way to do that. There's no way to prevent someone from adding import FoundationNetworking in the future and shadowing the locally declared shim. The right way to do this is for swift-corelibs-foundation to deprecate this initializer, but that's up to its maintainers and whether they want to do the same thing on Darwin so that these different platform implementations don't diverge even more.
There was a problem hiding this comment.
Another thing that comes to mind is just checking for FoundationNetworking imports in ./bin/test and raising an error if that's found. That will mean if anyone adds Data(contentsOf:) that will fail to build on Linux and Windows unless they add import FoundationNetworking, which in turn will make ./bin/test.
Is ./bin/test what you run on CI?
There was a problem hiding this comment.
Applied the latter suggestion in f2b5908.
There was a problem hiding this comment.
Is ./bin/test what you run on CI?
Yes
|
@swift-ci test |
|
@swift-ci test |
MaxDesiatov
changed the title
|
@swift-ci test |
|
@swift-ci test |
Sources/SwiftDocCUtilities/PreviewServer/RequestHandler/FileRequestHandler.swift
Show resolved
Hide resolved
|
I will get back to this PR and make exclusion of the old API conditional on a build setting. Marking it as a draft in the meantime. |
d-ronnqvist
added
the
source breaking
Summary
When linking
doccbinary statically with Swift Foundation and stdlib (withswift build --product docc --static-swift-stdlib) it still depends on FoundationNetworking on non-Darwin platforms. That library itself has a dependency on curl and pulls in a huge list of transitive dependencies because of that. Such libraries are still linked dynamically, and this can be shown with this command on Linux:This makes the
doccbinary hard to distribute on Linux even when linking Foundation and stdlib statically.DocC doesn't actually need anything from
FoundationNetworkingother than these two things:Data(contentsOf: URL)initializer. This one pulls networking code as it doesn't know upfront if a URL is local or remote. I've audited its uses in the DocC codebase and there's no reliance on remote URLs. In fact, I'd advocate replacing such uses ofURLwith Swift System'sFilePathin a future PR to guarantee that these are actual file paths at compile-time on the type system level and no URLs can imply use of some HTTP client under the hood. In the meantime calls toData(contentsOf:)are replaced withFileHandle.readToEnd().URLRequestandURLResponse, but only as model types and no actual networking calls are made with them on non-Darwin platforms.HTTPRequestandHTTPResponsefrom the new apple/swift-http-types library covers this functionality.Dependencies
A new dependency on apple/swift-http-types 0.2.1 is introduced to replace existing uses of
URLRequestandURLResponseon non-Darwin platforms.Testing
Build on Linux with
swift build --product docc --static-swift-stdliband runldd ./.build/debug/docc, observe that the list of linked libraries is as small as this:Checklist
./bin/testscript and it succeeded