Windows Support #343
Windows Support #343
Conversation
Rather than exhaustively enumerating via OS use the unstable feature to detect if ObjC interop is enabled. This allows building on Windows. Unfortunately, the entire test suite does not pass on Windows, with path issues due to PathKit's handling of paths.
Migrate from PathKit to Foundation for the file management operations. This enables the use of Stencil on Windows. With this set of changes, the complete test suite passes on Windows. This is a breaking change as it removes some deprecated APIs as well as changes the shape of the API to fully remove the dependency on PathKit.
| } | ||
|
|
||
| public func loadTemplate(names: [String], environment: Environment) throws -> Template { | ||
| for path in paths { | ||
| for templateName in names { | ||
| let templatePath = try path.safeJoin(path: Path(templateName)) | ||
| let templatePath = URL(fileURLWithPath: templateName, relativeTo: URL(fileURLWithPath: path)) | ||
| if !templatePath.withUnsafeFileSystemRepresentation({ String(cString: $0!) }).hasPrefix(path) { |
There was a problem hiding this comment.
Is this equivilent to the old implementation? There doesn't seem to be an explict asolute path, perhaps this is happening for free by this construct?
There also doesn't appear to be any test cases for SuspiciousFileOperation.
There was a problem hiding this comment.
Yes, it should be equivalent to the previous implementation. The pathJoin is concatenating two path components. The FSR is going to the canonicalised concatenation of the paths. The current tests do invoke this method IIRC and do pass.
There was a problem hiding this comment.
I'd rather have this in a tiny extension on URL or FileManager, instead of inside the loadTemplate function.
There was a problem hiding this comment.
I'm not completely against dropping PathKit, it hasn't been updated in quite a while, and pulls in Spectre as a test dependency. Thing to note though: most tools/libraries I know that import Stencil also use PathKit for typed paths.
I noticed there's a PR in PathKit to add Windows support. Does such a PR supersede this one?
| self.paths = paths | ||
| public init(paths: [URL]) { | ||
| self.paths = paths.map { | ||
| $0.withUnsafeFileSystemRepresentation { String(cString: $0!) } |
There was a problem hiding this comment.
Why would we go through c-strings? Can't we just use the .path property of a (file) URL for conversion to String?
Note: not relevant if we just use (file) URLs.
There was a problem hiding this comment.
No, we cannot. The .path does not give you a file system representation. It gives you a URL path representation. This just happens to work on some platforms, but is not guaranteed, and does in fact break on Windows. The filesystem operations operate on the file system representation, which is only provided as a C-string.
| @@ -34,15 +33,17 @@ extension Loader { | |||
|
|
|||
| // A class for loading a template from disk | |||
| public class FileSystemLoader: Loader, CustomStringConvertible { | |||
| public let paths: [Path] | |||
| public let paths: [String] | |||
There was a problem hiding this comment.
Why Strings? I'd rather use a more specific type like URL. Unfortunately Swift conflates local & remote urls 🤷
There was a problem hiding this comment.
Sure, I can change this to [URL].
| } | ||
|
|
||
| public func loadTemplate(names: [String], environment: Environment) throws -> Template { | ||
| for path in paths { | ||
| for templateName in names { | ||
| let templatePath = try path.safeJoin(path: Path(templateName)) | ||
| let templatePath = URL(fileURLWithPath: templateName, relativeTo: URL(fileURLWithPath: path)) | ||
| if !templatePath.withUnsafeFileSystemRepresentation({ String(cString: $0!) }).hasPrefix(path) { |
There was a problem hiding this comment.
I'd rather have this in a tiny extension on URL or FileManager, instead of inside the loadTemplate function.
| class SuspiciousFileOperation: Error { | ||
| let basePath: Path | ||
| let path: Path | ||
| let basePath: String |
There was a problem hiding this comment.
Same as above, I'd prefer a more concrete type.
| } | ||
|
|
||
| /// Create a template with a file found at the given URL | ||
| @available(*, deprecated, message: "Use Environment/FileSystemLoader instead") |
There was a problem hiding this comment.
So if we're dropping these because we no longer have the Path type, I'm assuming it'll be at least a "minor" version bump (considering we're still on 0.x.x)
| #if os(Linux) | ||
| return nil | ||
| #else | ||
| #if _runtime(_ObjC) |
There was a problem hiding this comment.
As discussed in #341, I don't there's need for this, and we can just use canImport for now.
There was a problem hiding this comment.
Yeah, that commit needs to be dropped, I couldn't easily do so though.
No, it does not. That change does allow compilation, but pretty quickly falls apart. Fixing that would require a pretty invasive rewrite that I don't think is valuable (at least not sufficiently that I would be willing to do that work). |
Wondering what makes it so difficult, is it a subset of APIs are hard to port like glob or something more fundamental? Edit: feel free to answer that in the PathKit PR if that makes more sense as its not all that related to Stencil :) |
In order to support Windows, PathKit must be removed. Porting PathKit turned out to be more difficult than anticipated and it was easier to remove that in favour of Foundation.