Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Windows Support #343

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Windows Support #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e22abe
Stencil: use unstable features for portability
compnerd Jun 25, 2023
a378159
Migrate away from PathKit to Foundation
compnerd Jun 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions Package.resolved
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,6 @@
{
"object": {
"pins": [
{
"package": "PathKit",
"repositoryURL": "https://github.com/kylef/PathKit.git",
"state": {
"branch": null,
"revision": "3bfd2737b700b9a36565a8c94f4ad2b050a5e574",
"version": "1.0.1"
}
},
{
"package": "Spectre",
"repositoryURL": "https://github.com/kylef/Spectre.git",
Expand Down
5 changes: 1 addition & 4 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,10 @@ let package = Package(
.library(name: "Stencil", targets: ["Stencil"])
],
dependencies: [
.package(url: "https://github.com/kylef/PathKit.git", from: "1.0.1"),
.package(url: "https://github.com/kylef/Spectre.git", from: "0.10.1")
],
targets: [
.target(name: "Stencil", dependencies: [
"PathKit"
]),
.target(name: "Stencil"),
.testTarget(name: "StencilTests", dependencies: [
"Stencil",
"Spectre"
Expand Down
2 changes: 0 additions & 2 deletions Sources/Stencil/Include.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
// MIT Licence
//

import PathKit

class IncludeNode: NodeType {
let templateName: Variable
let includeContext: String?
Expand Down
53 changes: 17 additions & 36 deletions Sources/Stencil/Loader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
//

import Foundation
import PathKit

/// Type used for loading a template
public protocol Loader {
Expand Down Expand Up @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Strings? I'd rather use a more specific type like URL. Unfortunately Swift conflates local & remote urls 🤷

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can change this to [URL].


public init(paths: [Path]) {
self.paths = paths
public init(paths: [URL]) {
self.paths = paths.map {
$0.withUnsafeFileSystemRepresentation { String(cString: $0!) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

public init(bundle: [Bundle]) {
self.paths = bundle.map { bundle in
Path(bundle.bundlePath)
self.paths = bundle.map {
URL(fileURLWithPath: $0.bundlePath).withUnsafeFileSystemRepresentation { String(cString: $0!) }
compnerd marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -51,27 +52,19 @@ public class FileSystemLoader: Loader, CustomStringConvertible {
}

public func loadTemplate(name: String, environment: Environment) throws -> Template {
for path in paths {
let templatePath = try path.safeJoin(path: Path(name))

if !templatePath.exists {
continue
}

let content: String = try templatePath.read()
return environment.templateClass.init(templateString: content, environment: environment, name: name)
}

throw TemplateDoesNotExist(templateNames: [name], loader: self)
return try loadTemplate(names: [name], environment: environment)
}

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather have this in a tiny extension on URL or FileManager, instead of inside the loadTemplate function.

throw SuspiciousFileOperation(basePath: path, path: templateName)
}

if templatePath.exists {
let content: String = try templatePath.read()
if FileManager.default.fileExists(atPath: templatePath.path) {
let content = try String(contentsOf: templatePath)
return environment.templateClass.init(templateString: content, environment: environment, name: templateName)
}
}
Expand Down Expand Up @@ -107,23 +100,11 @@ public class DictionaryLoader: Loader {
}
}

extension Path {
func safeJoin(path: Path) throws -> Path {
let newPath = self + path

if !newPath.absolute().description.hasPrefix(absolute().description) {
throw SuspiciousFileOperation(basePath: self, path: newPath)
}

return newPath
}
}

class SuspiciousFileOperation: Error {
let basePath: Path
let path: Path
let basePath: String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, I'd prefer a more concrete type.

let path: String

init(basePath: Path, path: Path) {
init(basePath: String, path: String) {
self.basePath = basePath
self.path = path
}
Expand Down
15 changes: 1 addition & 14 deletions Sources/Stencil/Template.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
//

import Foundation
import PathKit

#if os(Linux)
// swiftlint:disable:next prefixed_toplevel_constant
Expand Down Expand Up @@ -41,19 +40,7 @@ open class Template: ExpressibleByStringLiteral {
throw NSError(domain: NSCocoaErrorDomain, code: NSFileNoSuchFileError, userInfo: nil)
}

try self.init(URL: url)
}

/// Create a template with a file found at the given URL
@available(*, deprecated, message: "Use Environment/FileSystemLoader instead")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that seems fair.

public convenience init(URL: Foundation.URL) throws {
try self.init(path: Path(URL.path))
}

/// Create a template with a file found at the given path
@available(*, deprecated, message: "Use Environment/FileSystemLoader instead")
public convenience init(path: Path, environment: Environment? = nil, name: String? = nil) throws {
self.init(templateString: try path.read(), environment: environment, name: name)
try self.init(templateString: String(contentsOf: url))
}

// MARK: ExpressibleByStringLiteral
Expand Down
6 changes: 3 additions & 3 deletions Sources/Stencil/Variable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,12 @@ public struct Variable: Equatable, Resolvable {
} else if let string = context as? String {
return resolve(bit: bit, collection: string)
} else if let object = context as? NSObject { // NSKeyValueCoding
#if os(Linux)
return nil
#else
#if _runtime(_ObjC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in #341, I don't there's need for this, and we can just use canImport for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that commit needs to be dropped, I couldn't easily do so though.

if object.responds(to: Selector(bit)) {
return object.value(forKey: bit)
}
#else
return nil
#endif
} else if let value = context as? DynamicMemberLookup {
return value[dynamicMember: bit]
Expand Down
3 changes: 1 addition & 2 deletions Tests/StencilTests/EnvironmentBaseAndChildTemplateSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
// MIT Licence
//

import PathKit
import Spectre
@testable import Stencil
import XCTest
Expand All @@ -17,7 +16,7 @@ final class EnvironmentBaseAndChildTemplateTests: XCTestCase {
override func setUp() {
super.setUp()

let path = Path(#file as String) + ".." + "fixtures"
let path = URL(fileURLWithPath: #file).deletingLastPathComponent().appendingPathComponent("fixtures")
let loader = FileSystemLoader(paths: [path])
environment = Environment(loader: loader)
childTemplate = ""
Expand Down
3 changes: 1 addition & 2 deletions Tests/StencilTests/EnvironmentIncludeTemplateSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
// MIT Licence
//

import PathKit
import Spectre
@testable import Stencil
import XCTest
Expand All @@ -17,7 +16,7 @@ final class EnvironmentIncludeTemplateTests: XCTestCase {
override func setUp() {
super.setUp()

let path = Path(#file as String) + ".." + "fixtures"
let path = URL(fileURLWithPath: #file).deletingLastPathComponent().appendingPathComponent("fixtures")
let loader = FileSystemLoader(paths: [path])
environment = Environment(loader: loader)
template = ""
Expand Down
3 changes: 1 addition & 2 deletions Tests/StencilTests/IncludeSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
// MIT Licence
//

import PathKit
import Spectre
@testable import Stencil
import XCTest

final class IncludeTests: XCTestCase {
private let path = Path(#file as String) + ".." + "fixtures"
private let path = URL(fileURLWithPath: #file).deletingLastPathComponent().appendingPathComponent("fixtures")
private lazy var loader = FileSystemLoader(paths: [path])
private lazy var environment = Environment(loader: loader)

Expand Down
3 changes: 1 addition & 2 deletions Tests/StencilTests/InheritanceSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
// MIT Licence
//

import PathKit
import Spectre
import Stencil
import XCTest

final class InheritanceTests: XCTestCase {
private let path = Path(#file as String) + ".." + "fixtures"
private let path = URL(fileURLWithPath: #file).deletingLastPathComponent().appendingPathComponent("fixtures")
private lazy var loader = FileSystemLoader(paths: [path])
private lazy var environment = Environment(loader: loader)

Expand Down
8 changes: 5 additions & 3 deletions Tests/StencilTests/LexerSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
// MIT Licence
//

import PathKit
import Spectre
@testable import Stencil
import XCTest
Expand Down Expand Up @@ -140,8 +139,11 @@ final class LexerTests: XCTestCase {
}

func testPerformance() throws {
let path = Path(#file as String) + ".." + "fixtures" + "huge.html"
let content: String = try path.read()
let path = URL(fileURLWithPath: #file)
.deletingLastPathComponent()
.appendingPathComponent("fixtures")
.appendingPathComponent("huge.html")
let content: String = try String(contentsOf: path)

measure {
let lexer = Lexer(templateString: content)
Expand Down
3 changes: 1 addition & 2 deletions Tests/StencilTests/LoaderSpec.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
// MIT Licence
//

import PathKit
import Spectre
import Stencil
import XCTest

final class TemplateLoaderTests: XCTestCase {
func testFileSystemLoader() {
let path = Path(#file as String) + ".." + "fixtures"
let path = URL(fileURLWithPath: #file).deletingLastPathComponent().appendingPathComponent("fixtures")
let loader = FileSystemLoader(paths: [path])
let environment = Environment(loader: loader)

Expand Down