various fixes for Windows path handling #6365
Conversation
Here's an example of your CHANGELOG entry: * various fixes for Windows path handling.
[compnerd](https://github.com/compnerd)
[#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)note: There are two invisible spaces after the entry's text. Generated by 🚫 Danger |
| public var relativeFile: String? { | ||
| let url = URL(fileURLWithPath: FileManager.default.currentDirectoryPath, isDirectory: true) | ||
| return file?.replacingOccurrences(of: url.filepath, with: "") | ||
| return file?.replacingOccurrences(of: url.filepath + "/", with: "") |
There was a problem hiding this comment.
This feels wrong - we should be computing that by stripping the parent components.
There was a problem hiding this comment.
You mean removing the prefix instead of replacing occurrences? Yes, this is a pre-existing issue unrelated to this PR, where relative path of /swiftlint/src/swiftlint/ with cwd=/swiftlint would break. Diff with origin:
/// The file path for this location relative to the current working directory.
public var relativeFile: String? {
- return file?.replacingOccurrences(of: FileManager.default.currentDirectoryPath + "/", with: "")
+ let url = URL(fileURLWithPath: FileManager.default.currentDirectoryPath, isDirectory: true)
+ return file?.replacingOccurrences(of: url.filepath + "/", with: "")
}
There was a problem hiding this comment.
Yeah, we should change this to use the parent path stripping instead IMO
| if filePath.hasPrefix(basePath) { | ||
| return String(filePath.dropFirst(basePath.count)) | ||
| } | ||
| return file |
There was a problem hiding this comment.
Sorry, I should've been more clear. We should de-compose the path with pathComponents based handling. we would need to ensure that the CWD is entirely present though.
There was a problem hiding this comment.
Almost - but rather than the .joined(separator: "/"), use a reduction of appendingPathComponent (or if we can use newer APIs, appending(components:)).
There was a problem hiding this comment.
Updated to reduce+append in 30b80c4, apparently appending doesn't work as "Swift doesn't allow spreading arrays into variadic parameters" but please lmk if there is a better way
There was a problem hiding this comment.
eh, this doesn't work either because we get ./ at the beginning of the path.. Maybe just keep .joined(separator: "/")? It seems like the most straightforward opion.
| return fileComponents.dropFirst(baseComponents.count).reduce(into: URL(filePath: "")) { | ||
| $0.append(component: $1) | ||
| }.relativePath | ||
| return fileComponents.dropFirst(baseComponents.count).joined(separator: "/") |
There was a problem hiding this comment.
This opens us up to issues with different path separators. This needs to be a proper file system representation somehow.
There was a problem hiding this comment.
fwiw there are already a couple other places where .joined(separator: "/") is used, and this is normally not really an issue on Windows as it can handle both? I find it really odd that there is no equivalent of python's os.path.join on Swift as that would be the obvious choice in cases like this.
There was a problem hiding this comment.
Those need to be adjusted as well. It is an issue, Windows cannot handle both. Win32 handles both, NT does not. If you have an extended path, it must be in the correct form.
The equivalent is to always just use URL.
There was a problem hiding this comment.
I'll look some more but I don't think URL is equivalent to os.path.join - the latter just joins paths without trying to be smart about it, URL has all these surprises with symlink resolution etc, which we run into in those bazel builds I believe.
btw I didn't realize how easy it was to repro the bazel stuff locally, turns out it's just
% brew install bazelisk
% bazelisk test //Tests:FileSystemAccessTests --test_filter=GlobTests.testMatchesCharacterInRange --test_output=streamed
and I now see the /var/tmp vs /private/var/tmp discrepancy locally. That makes it a bit easier to figure out those surprises :)
There was a problem hiding this comment.
Looking at what was done to ConfigurationTests in https://github.com/realm/SwiftLint/pull/6342/files, these two things stood out for me (the tests themselves were changed to this):
XCTAssert(FileManager.default.changeCurrentDirectoryPath(Mock.Dir.exclusionTests))
and
assertEqual(["directory/File1.swift", "directory/File2.swift", "directory/ExcludedFile.swift"], paths)
I think this might be 1) getting around some of the path shenanigans and 2) expecting the forward slash to be returned by e.g. lintablePaths.
Win32 handles both, NT does not. If you have an extended path, it must be in the correct form
I suppose you're talking about the API level, I was thinking more of the general tooling which often hides this, I'm guessing some kind of translation underneath, so you can cd "c:/" in cmd or just use forward slashes in python without running into issues (os.path.join still being preferable of course).
I'm not sure what we're trying to do here, as the change above is using forward slashes so maybe we want that to keep behaving the same way and hide the / -> \ as an implementation detail in some lower level?..
cc @SimplyDanny
There was a problem hiding this comment.
At some point, the string paths returned by lintablePaths need to be converted into URLs (before we eventually access real files with them). Not sure if and where this is currently happening. For the test, it's fine to work with the string paths as its purpose is to find the correct files in the first place.
There was a problem hiding this comment.
I narrowed down the bazel issue to this part of the change:
--- a/Tests/TestHelpers/TestResources.swift
+++ b/Tests/TestHelpers/TestResources.swift
@@ -9,7 +9,6 @@ public enum TestResources {
}
return folder
.appendingPathComponent("Resources")
- .path
- .absolutePathStandardized()
+ .filepath
}
}
Which is due to:
folder.filepath: /private/var/tmp/...
folder.path: /private/var/tmp/...
folder.path.absolutePathStandardized(): /var/tmp/...
What should be the intent here?
There was a problem hiding this comment.
I changed TestResources already a while back (the PR was laying around for fairly long) and so forgot to update it for the Windows use case.
Since it's referring to real files, it should return URL and later keep using URL up to the point when the path doesn't get modified further and is eventually used to access a file (then with the rather new .filepath).
There was a problem hiding this comment.
Okay I unbroke tests by switching TestResources path to URL and sprinkling symlink resolution in a few places.
I kept the "/" join as I'm not sure if we want that API to change, or some underlying implementation to change.
@compnerd maybe we could merge this like this (unless there are major non-Windows-specific issues with what I've changed) and iterate on follow up PRs?
There was a problem hiding this comment.
Discussed offline, restored to the original implementation so we can properly fix in a follow up
|
|
||
| public enum TestResources { | ||
| public static func path(_ calleePath: String = #filePath) -> String { | ||
| public static func path(_ calleePath: String = #filePath) -> URL { |
There was a problem hiding this comment.
Can symlinks be resolved once here? I assume symlinks appear when running with Bazel only?
There was a problem hiding this comment.
Updated. Yes, I've only seen symlinks in Bazel and specifically /var/tmp/ vs /private/var/tmp/. I haven't tested anything else.
| import XCTest | ||
|
|
||
| private let fixturesDirectory = "\(TestResources.path())/FileHeaderRuleFixtures" | ||
| private let fixturesDirectory = "\(TestResources.path().filepath)/FileHeaderRuleFixtures" |
There was a problem hiding this comment.
| private let fixturesDirectory = "\(TestResources.path().filepath)/FileHeaderRuleFixtures" | |
| private let fixturesDirectory = TestResources.path().appendingPathComponent("FileHeaderRuleFixtures").filepath |
| import XCTest | ||
|
|
||
| private let fixturesDirectory = "\(TestResources.path())/FileNameNoSpaceRuleFixtures" | ||
| private let fixturesDirectory = "\(TestResources.path().filepath)/FileNameNoSpaceRuleFixtures" |
There was a problem hiding this comment.
| private let fixturesDirectory = "\(TestResources.path().filepath)/FileNameNoSpaceRuleFixtures" | |
| private let fixturesDirectory = TestResources.path().appendingPathComponent("FileNameNoSpaceRuleFixtures").filepath |
|
|
||
| func testRulesWithFileThatCrashedSourceKitService() throws { | ||
| let file = try XCTUnwrap(SwiftLintFile(path: "\(TestResources.path())/ProjectMock/Level0.swift")) | ||
| let file = try XCTUnwrap(SwiftLintFile(path: "\(TestResources.path().filepath)/ProjectMock/Level0.swift")) |
There was a problem hiding this comment.
| let file = try XCTUnwrap(SwiftLintFile(path: "\(TestResources.path().filepath)/ProjectMock/Level0.swift")) | |
| let file = try XCTUnwrap(SwiftLintFile(path: TestResources.path()..appendingPathComponent("ProjectMock").appendingPathComponent("Level0.swift").filepath) |
| private typealias Configuration = RegexConfiguration<CustomRules> | ||
|
|
||
| private var testFile: SwiftLintFile { SwiftLintFile(path: "\(TestResources.path())/test.txt")! } | ||
| private var testFile: SwiftLintFile { SwiftLintFile(path: "\(TestResources.path().filepath)/test.txt")! } |
There was a problem hiding this comment.
| private var testFile: SwiftLintFile { SwiftLintFile(path: "\(TestResources.path().filepath)/test.txt")! } | |
| private var testFile: SwiftLintFile { SwiftLintFile(path: TestResources..path().appendingPathComponent("test.txt").filepath)! } |
| if let rootProjectDirectory = ProcessInfo.processInfo.environment["BUILD_WORKSPACE_DIRECTORY"] { | ||
| return "\(rootProjectDirectory)/Tests/\(folder.lastPathComponent)/Resources" | ||
| return URL( | ||
| fileURLWithPath: "\(rootProjectDirectory)/Tests/\(folder.lastPathComponent)/Resources", |
There was a problem hiding this comment.
| fileURLWithPath: "\(rootProjectDirectory)/Tests/\(folder.lastPathComponent)/Resources", | |
| fileURLWithPath: rootProjectDirectory.appendingPathComponent("Tests").appendingPathComponent(folder.lastPathComponent).appendingPathComponent("Resources").filepath, |
| fileName: "main.swift", | ||
| excluded: [], | ||
| excludedPaths: [".*/FileNameRuleFixtures/.*"] | ||
| excludedPaths: [#".*[\\/]FileNameRuleFixtures[\\/].*"#] |
There was a problem hiding this comment.
These patterns should be treated independently of what the right format is on the current OS. I propose that to support / only in these patterns and internally SwiftLint needs to make sure that they are mapped correctly to the underlying file/path representation.
Does that make sense?
There was a problem hiding this comment.
Well that's exactly what I was talking about earlier... I wasn't sure where you expected to see plain strings with forward slashes vs URLs. I personally find reasoning about POSIX paths much more convenient than generic paths that might have either of the slashes, they're easier to reason about and Windows users aren't that surprised when they see a forward slash. So to me personally plaintext POSIX makes sense as the top-level abstraction, but that's a different approach from "everything is an URL" so we need to pick one or the other. What does the glob support library (you were looking into) use by the way?
There was a problem hiding this comment.
The Glob library only works for Unix paths. It matches on strings using forward slashes.
I think that the current approach (using URLs internally) is quite right. We only need to make sure that when it comes to user input, path patterns will be Unix-style (only) and we need to be able to handle them correctly: probably like URL -> Unix path -> filter with patterns -> URL -> Unix/Windows output paths.
I just need to think more about where the right conversion points are. 😅
There was a problem hiding this comment.
Hmm. I'm not actually sure how you're planning to use that library. It seems to only provide an interface that tells you whether a given filename matches a pattern, but there is no interface for actually efficient globbing. You could list all files and then use that matching but that means traversing the whole tree? The proper implementation would require at least some integration with directory listing of a particular OS.
Example: an efficient traversal of a123/b456/*/1.json would only list items under a123/b456/ and not traverse any other part of the tree, so on Windows it needs to somehow get translated to FindFirstFile/NtQueryDirectoryFile specifically on a123\b456
There was a problem hiding this comment.
With #6366, file traversal is done with a directory enumerator which takes the cases into account that you describe in your example, only that it doesn't work for Windows paths yet. There, I just need to follow my own advice:
We only need to make sure that when it comes to user input, path patterns will be Unix-style (only) and we need to be able to handle them correctly: probably like URL -> Unix path -> filter with patterns -> URL -> Unix/Windows output paths.
Here, I commented on the patterns only considered by specific rules (file_name in this case). These rules also need to respect / over \ on Windows.
There was a problem hiding this comment.
Running this locally, it seems that the current version still fails to run the test suite - not just with failing tests but with abnormal termination. I'm not sure I see the value of merging this as is as it isn't really focused on a single issue but tries to fix a number of things. If this were a very targeted fix, I would agree.
There was a problem hiding this comment.
I started with the "URL everywhere and always" approach. It's quite a lot but looks promising so far. The glob stuff also works as it can operate in the file representation of URL which remains the Unix-style, so the logic doesn't need to change. It's only some back and forth with URL and string path representation. But what's passed around can be URL only.
On my tour through the whole code base, I came across a few more places that also need adaption. Using URL feels more natural now than operating on strings.
I'll need some more time to get this done but will let you know once it is. Performance check are critical then. It's not always obvious which operations on URLs consult the file system.
There was a problem hiding this comment.
Globing against the URI is reasonable, but, it should be explicitly documented.
Consider something like file:///C:/**/*.swift and file://filer/share/**/*.swift, file://filer.local/share/**/*.swift as patterns (for Windows and UNC paths).
This is also complicated as there are multiple spellings:
file:///c:/**/*.swiftfile:///C|/**/*.swiftfile:C|/**/*.swift
are all the same pattern (note separators, case changes, and scheme and path joining).
UNC path compatibility (needed for proper windows support) also has the oddity of the empty authority and preserved leader, i.e. file://///filer.local/share/**/*.swift.
Finally, note also the need to accept URL encoded patterns as certain characters must be URL encoded as they are invalid, e.g. $, which is used on OpenVMS and special paths on windows.
There was a problem hiding this comment.
@SimplyDanny thanks for the clarification!
@compnerd this is quickly escalating towards being worthy a 3rd party library :) which could then be used elsewhere not just SwiftLint
There was a problem hiding this comment.
Cross-platform globbing is not really a thing for a reason - file paths are not uniform.
What about HFS paths? Machintosh HD:Users:compnerd:Desktop wouldn't work very well with the glob patterns for Unix.
Perhaps a VMS path would be a good thing to also account for: DISK$USERS:[COMPNERD]FILE.TXT;4.
How would you account for that in the current globbing mechanism? We really should avoid the globbing or require the user to provide a generic regular expression to match paths.
An assortment of fixes that improve the test coverage on Windows. With this set, local testing reveals 1 failure (RemoteCycleDetection) and another set of test failures due to Windows globbing not matching the POSIX semantics.
SimplyDanny
force-pushed
the
assundry
branch
from
8bc5016 to
63f358c
Compare
4 participants
An assortment of fixes that improve the test coverage on Windows. With this set, local testing reveals 1 failure (RemoteCycleDetection) and another set of test failures due to Windows globbing not matching the POSIX semantics.