-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid FoundationNetworking and libcurl on non-Darwin platforms #681
base: main
Are you sure you want to change the base?
Changes from 7 commits
66353a1
0db65fa
c1b5529
992d221
58a5d0e
f042a68
71fe790
8c4e6d0
fb38bef
f2b5908
ea5bccb
98561ac
30500a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,11 @@ public struct LocalFileSystemDataProvider: DocumentationWorkspaceDataProvider, F | |
|
||
public func contentsOfURL(_ url: URL) throws -> Data { | ||
precondition(url.isFileURL, "Unexpected non-file url '\(url)'.") | ||
return try Data(contentsOf: url) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have to do this instead of Is there anyway that we can shim There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not aware of a way to do that. There's no way to prevent someone from adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thing that comes to mind is just checking for Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied the latter suggestion in f2b5908. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes |
||
|
||
return data | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,8 @@ | |
*/ | ||
|
||
import Foundation | ||
import HTTPTypes | ||
import SymbolKit | ||
#if canImport(FoundationNetworking) | ||
import FoundationNetworking | ||
#endif | ||
|
||
fileprivate let slashCharSet = CharacterSet(charactersIn: "/") | ||
|
||
|
@@ -56,49 +54,49 @@ public class FileServer { | |
/** | ||
Returns the data for a given URL. | ||
*/ | ||
public func data(for url: URL) -> Data? { | ||
public func data(for path: String) -> Data? { | ||
MaxDesiatov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let providerKey = providers.keys.sorted { (l, r) -> Bool in | ||
l.count > r.count | ||
}.filter { (path) -> Bool in | ||
return url.path.trimmingCharacters(in: slashCharSet).hasPrefix(path) | ||
}.filter { (providerPath) -> Bool in | ||
return path.trimmingCharacters(in: slashCharSet).hasPrefix(providerPath) | ||
}.first ?? "" //in case missing an exact match, get the root one | ||
guard let provider = providers[providerKey] else { | ||
fatalError("A provider has not been passed to a FileServer.") | ||
} | ||
return provider.data(for: url.path.trimmingCharacters(in: slashCharSet).removingPrefix(providerKey)) | ||
return provider.data(for: path.trimmingCharacters(in: slashCharSet).removingPrefix(providerKey)) | ||
} | ||
|
||
/** | ||
Returns a tuple with a response and the given data. | ||
- Parameter request: The request coming from a web client. | ||
- Returns: The response and data which are going to be served to the client. | ||
*/ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a source breaking change. Is there anywhere to maintain both declarations to ease this transition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can guard the old function that uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
guard let path = request.path as NSString? else { | ||
return (.init(status: 400), nil) | ||
} | ||
var data: Data? = nil | ||
let response: URLResponse | ||
let response: HTTPTypes.HTTPResponse | ||
|
||
let mimeType: String | ||
|
||
// We need to make sure that the path extension is for an actual file and not a symbol name which is a false positive | ||
// like: "'...(_:)-6u3ic", that would be recognized as filename with the extension "(_:)-6u3ic". (rdar://71856738) | ||
if url.pathExtension.isAlphanumeric && !url.lastPathComponent.isSwiftEntity { | ||
data = self.data(for: url) | ||
mimeType = FileServer.mimeType(for: url.pathExtension) | ||
if path.pathExtension.isAlphanumeric && !path.lastPathComponent.isSwiftEntity { | ||
data = self.data(for: path as String) | ||
mimeType = FileServer.mimeType(for: path.pathExtension) | ||
} else { // request is for a path, we need to fake a redirect here | ||
if url.pathComponents.isEmpty { | ||
xlog("Tried to load an invalid URL: \(url.absoluteString).\nFalling back to serve index.html.") | ||
if path.pathComponents.isEmpty { | ||
xlog("Tried to load an invalid path: \(path).\nFalling back to serve index.html.") | ||
} | ||
mimeType = "text/html" | ||
data = self.data(for: baseURL.appendingPathComponent("/index.html")) | ||
data = self.data(for: "/index.html") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why this isn't a change in behavior? What's needless about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only Additionally, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
} | ||
|
||
if let data = data { | ||
response = URLResponse(url: url, mimeType: mimeType, expectedContentLength: data.count, textEncodingName: nil) | ||
response = .init(status: .ok, headerFields: [.contentType: mimeType, .contentLength: "\(data.count)"]) | ||
} else { | ||
response = URLResponse(url: url, mimeType: nil, expectedContentLength: 0, textEncodingName: nil) | ||
response = .init(status: .ok, headerFields: [.contentType: "application/octet-stream"]) | ||
} | ||
|
||
return (response, data) | ||
|
@@ -175,7 +173,8 @@ public class FileSystemServerProvider: FileServerProvider { | |
|
||
public func data(for path: String) -> Data? { | ||
let finalURL = directoryURL.appendingPathComponent(path) | ||
return try? Data(contentsOf: finalURL) | ||
let fileHandle = try? FileHandle(forReadingFrom: finalURL) | ||
return try? fileHandle?.readToEnd() | ||
} | ||
|
||
} | ||
|
@@ -230,7 +229,12 @@ public class MemoryFileServerProvider: FileServerProvider { | |
|
||
for file in enumerator { | ||
guard let file = file as? String else { fatalError("Enumerator returned an unexpected type.") } | ||
guard let data = try? Data(contentsOf: URL(fileURLWithPath: path).appendingPathComponent(file)) else { continue } | ||
let fileURL = URL(fileURLWithPath: path).appendingPathComponent(file) | ||
guard | ||
let fileHandle = try? FileHandle(forReadingFrom: fileURL), | ||
let data = try? fileHandle.readToEnd() | ||
else { continue } | ||
|
||
if recursive == false && file.contains("/") { continue } // skip if subfolder and recursive is disabled | ||
addFile(path: "/\(trimmedSubPath)/\(file)", data: data) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped to
.v11
per a discussion with @franklinsch, this allows us to use throwingFileHandle.readToEnd()
unlike the unsafe non-throwingFileHandle.readDataToEndOfFile
that crashes the test suite on Linux.