Fix support for `FileIterator` when working directory is `/` #865

macshome · 2024-10-25T14:28:49Z

Added an additional check that tests to see if the working directory is root to solve #862.

Simple Fix

Simple check that looks to see if the working directory is root.

 let relativePath =
          path.hasPrefix(workingDirectory.path) && FileManager.default.currentDirectoryPath != "/"
          ? String(path.dropFirst(workingDirectory.path.count + 1))
          : path
        output =
          URL(fileURLWithPath: relativePath, isDirectory: false, relativeTo: workingDirectory)

Testing

I couldn't find a non-intrusive way to simulate changing the working directory of the unit tests. This path remains uncovered as a result of that.

Manual testing with the debug build. The behavior of #862 is resolved.

Works from /tmp

/tmp % pwd
/tmp

/tmp % ls -l testsf/*.swift 
-rw-r--r--@ 1 josh.wisenbaker  wheel  308 Nov  4  2022 testsf/bp.swift

/tmp % <trimmed>/Products/Debug/swift-format lint -rs .
bp.swift:1:31: warning: [GroupNumericLiterals] group every 3 digits in this decimal literal using a '_' separator

Now works from / as well

% cd /           
/ % pwd                 
/

/ % ls -l /tmp/testsf/*.swift
-rw-r--r--@ 1 josh.wisenbaker  wheel  308 Nov  4  2022 /tmp/testsf/bp.swift

/ % <trimmed>/Products/Debug/swift-format lint -rs /tmp/testsf
/private/tmp/testsf/bp.swift:1:31: warning: [GroupNumericLiterals] group every 3 digits in this decimal literal using a '_' separator

I'll need to spin up a linux VM to test if we get relative paths when running from root.

ahoppen

Thanks for opening a PR and addressing this issue @macshome.

To test this, what do you think about making workingDirectory an argument to FileIterator.init and just default it to URL(fileURLWithPath: ".")? That way you could set the working directory and we can also craft tests for Windows to ensure we don’t have any issues there.

ahoppen · 2024-10-25T18:44:18Z

Sources/SwiftFormat/Utilities/FileIterator.swift

@@ -145,7 +145,7 @@ public struct FileIterator: Sequence, IteratorProtocol {
        // be displayed as relative paths. Otherwise, they will still be displayed as absolute
        // paths.
        let relativePath =
-          path.hasPrefix(workingDirectory.path)
+          path.hasPrefix(workingDirectory.path) && path.first != "/"


Doesn’t this mean that we never relativize paths on Unix because all paths will start with a /? Don’t you want to check that workingDirectory is /? I might be misunderstanding your change though.

Hmm... You seem to be correct here. As far as I can tell the reason for using relative paths is just for nice diagnostic output.

// We attempt to relativize the URLs based on the current working directory, not the // directory being iterated over, so that they can be displayed better in diagnostics. Thus, // if the user passes paths that are relative to the current working directory, they will // be displayed as relative paths. Otherwise, they will still be displayed as absolute // paths.

And with workingDirectory being a constant of . this may take a bit more work. If we did change workingDirectory to be passed in then it would also allow for easier testing. I'll work on it a bit more and see what I can come up with.

Pushed a quick update to actually check if the current working directory is root or not rather than checking for a string of /.

path.hasPrefix(workingDirectory.path) && FileManager.default.currentDirectoryPath != "/"

macshome · 2024-10-25T20:01:41Z

Thanks for opening a PR and addressing this issue @macshome.

To test this, what do you think about making workingDirectory an argument to FileIterator.init and just default it to URL(fileURLWithPath: ".")? That way you could set the working directory and we can also craft tests for Windows to ensure we don’t have any issues there.

That's what I had thought about for setting a working directory, but that value is a constant in the code...

private let workingDirectory = URL(fileURLWithPath: ".")

Making it the default would probably work fine though.

allevato · 2024-10-25T20:14:00Z

That's what I had thought about for setting a working directory, but that value is a constant in the code...
private let workingDirectory = URL(fileURLWithPath: ".")

If you pass it in the initializer, you could make that the default value but then let tests specify whatever they want there. It's a member property, so it can certainly be changed by the initializer.

macshome · 2024-10-25T20:23:10Z

If you pass it in the initializer, you could make that the default value but then let tests specify whatever they want there. It's a member property, so it can certainly be changed by the initializer.

I'll take a trip down this path and see about getting it covered with tests so it doesn't break again.

macshome · 2024-10-28T13:57:09Z

@allevato I updated the tests to use [URL] instead of [String] and that let me then actually change the working directory to test that we don't clip names when running from / and that relative names are also used properly when applicable.

It takes the coverage on FileIterator.swift from 95.5% to a mighty 96.4%.

allevato · 2024-10-28T14:07:47Z

Sources/SwiftFormat/Utilities/FileIterator.swift

@@ -145,7 +145,7 @@ public struct FileIterator: Sequence, IteratorProtocol {
        // be displayed as relative paths. Otherwise, they will still be displayed as absolute
        // paths.
        let relativePath =
-          path.hasPrefix(workingDirectory.path)
+          path.hasPrefix(workingDirectory.path) && FileManager.default.currentDirectoryPath != "/"


@compnerd What will this do on Windows? Probably not what's intended, I think?

That was my concern, I don't know how Foundation will deal with that there.

Actually it might be OK.

https://github.com/swiftlang/swift-foundation/blob/ba000ae07c106149c2710bb9cb8c6993e20734b2/Sources/FoundationEssentials/FileManager/FileManager%2BDirectories.swift#L467-L476

(Giving up on trying to make it look pretty here in the comment.)

FileManager.default.currentDirectoryPath will never be / on Windows because Windows always has a drive letter, eg. C:\. Checking if a URL is a root path on Windows is not quite trivial because you also need to account for networks shares eg. \\your-server\blah and the only good way of doing it issuing the PathCchIsRoot Windows API (eg. https://github.com/swiftlang/swift-tools-support-core/blob/5b130e04cc939373c4713b91704b0c47ceb36170/Sources/TSCBasic/Path.swift#L491).

But maybe it’s not really an issue on Windows anyway, depending on how test output looks like. This is why I suggested making workingDirectory changeable from tests and then we can just check.

I don't know that it is an issue on Windows either. Most people seem to have discovered it when using WASM. Are there Windows and Linux runners for CI so that we can see if this breaks anything? Otherwise I can install a toolchain on Windows here and check.

I can still make workingDirectory changeable from tests, but at this point I'm worried about the validity of the entire approach on Windows.

If you don’t have a Windows machine to test this on, just write code that works to the best of you knowledge and some test cases for it. We have Windows PR testing (we just need to approve runs) and I’m happy to get the Windows code over the finish line if there are minor issues.

I set a toolchain up yesterday on my gaming machine so I will at least be sure that we pass the tests everywhere.

Oh, that’s great. Let me know if you have any questions.

Will do. Hoping to get back to this today.

Let me know when you think the PR is ready for review again.

macshome · 2024-11-06T14:48:26Z

Converting to draft while I work out the Windows stuff.

macshome · 2024-11-12T18:37:26Z

@ahoppen OK, I'm throwing in the towel on getting things to work as I expect on Windows. In the meantime I skipped the test for iterating in root on Windows. If someone who is better versed in Windows Foundation wants to take a swing at it that would be great.

All tests pass on macOS and Windows now, I've run swift format on the files I changed.

Issues I'm running into on Windows

After digging around in the Foundation sources it seems like FileManager should be doing all the right things on Windows. I made a simple CLI tool to make the calls and validate that.

// The Swift Programming Language
// https://docs.swift.org/swift-book
import WinSDK
import Foundation

var pwd = FileManager.default.currentDirectoryPath
print(pwd) //Output: C:\Users\macsh\Swift\pwd\pwd

var isRoot = pwd.withCString(encodedAs: UTF16.self, PathCchIsRoot)
print(isRoot) //Output: false

FileManager.default.changeCurrentDirectoryPath("/")
pwd = FileManager.default.currentDirectoryPath
isRoot = pwd.withCString(encodedAs: UTF16.self, PathCchIsRoot)

print(pwd) //Output: C:\
print(isRoot) //Output: true

That looks great! So I made a simple extension to URL and added a isRoot property to check for. That also works on Windows, but when I run the tests I end up with a bunch of extra / in the paths. Instead of the nice path strings from above I end up with things like /C:/Users/macsh/AppData/Local/Temp/TemporaryItems/(A document being saved by swiftformatPackageTestxctest 24 which are clearly incorrect for a number of reasons. I think that it's getting tripped up by the test runner, but I don't know enough on the Windows tooling to figure it out in a timely manner.

compnerd · 2024-11-12T20:57:27Z

Without checking the implementation, the thing that comes to mind is incorrect API usage. url.path will generally give you a Unix style path (/C:/) vs. url.withUnsafeFileSystemRepresentation { String(cString: $0!) } will give you a proper FSR (C:\). It could be that the CustomStringConvertible implementation goes through the latter. (As I've not verified that this is the case, this might just be entirely incorrect and is really pure speculation).

ahoppen · 2024-11-12T22:45:16Z

Thanks for your work on this change @macshome. I looked at the getting the tests passing on Windows as well and these are the changes I came up with. ahoppen@fe056af

Do you want to incorporate them into your PR?

macshome · 2024-11-13T17:57:52Z

Without checking the implementation, the thing that comes to mind is incorrect API usage. url.path will generally give you a Unix style path (/C:/) vs. url.withUnsafeFileSystemRepresentation { String(cString: $0!) } will give you a proper FSR (C:\). It could be that the CustomStringConvertible implementation goes through the latter. (As I've not verified that this is the case, this might just be entirely incorrect and is really pure speculation).

Yeah there are a lot of different callers flying around and I had the thought that maybe something was just taking a misstep somewhere. I'll take another look and see if I can reduce that particular issue more.

macshome · 2024-11-13T17:59:56Z

Thanks for your work on this change @macshome. I looked at the getting the tests passing on Windows as well and these are the changes I came up with. ahoppen@fe056af

Do you want to incorporate them into your PR?

Sure I can pull those changes in and then update my PR.

macshome · 2024-11-13T18:13:26Z

@ahoppen I synced up with main and pulled in your changes to update the PR.

ahoppen · 2024-11-13T21:28:38Z

@macshome Instead of merging main into your branch, could you rebase your branch? Otherwise LGTM.

macshome · 2024-11-14T19:35:17Z

@macshome Instead of merging main into your branch, could you rebase your branch? Otherwise LGTM.

@ahoppen Rebase onto your branch or onto main? Not trying to be obtuse here. We don't use rebase much in our shop so I just want to be sure I'm doing the right thing.

ahoppen · 2024-11-14T21:09:22Z

I meant to rebase onto main so that your branch doesn’t contain any merge commits.

macshome · 2024-11-19T17:47:29Z

Since I had been clicking the update button in GitHub along the way, there are a few merges to try to get rid of. It's probably easiest to just make a clean branch from main and then rebase the changes onto it. I'll need to create a new PR, but at least we can get the changes in.

ahoppen · 2024-11-19T19:12:53Z

You can rebase your changes and then force-push your branch to update this PR. There’s no need to create a new branch or open a new PR.

macshome · 2024-11-19T20:13:45Z

OK, I'll give that a spin. I figured a force-push was a non-starter.

macshome · 2024-11-26T15:00:50Z

Alright, I was on PTO for a wedding for a while (Not mine, but it was out of town) and I will get back to cleaning up the PR now.

macshome · 2024-12-02T22:15:51Z

@ahoppen Phew. git, am I right?

Hopefully the diff is now ready to test.

ahoppen · 2024-12-03T22:05:18Z

It looks like the git history got a little confused and there’s still a merge conflict. Could you rebase your commits so there’s no merge conflict and maybe also squash your commits into one?

macshome · 2024-12-05T15:41:41Z

I rebased interactively, fixed the conflict, and squashed into one commit. Each commit still shows on it's own also, I don't think that's an issue, but let me know if you want me to try and resolve that with a new PR linked to this one.

ahoppen · 2024-12-05T16:08:37Z

Not entirely sure what went on with your branch but I just rebased and squashed the commits. Maybe things got more complicated because this PR is from the main branch of your fork and thus it wasn’t easy for you to rebase on top of swift-format’s main branch.

ahoppen · 2024-12-05T17:35:44Z

I can take care of the CI failures

Added an additional check that tests to see if the working directory is root. swiftlang#862

macshome · 2024-12-06T19:27:39Z

Not entirely sure what went on with your branch but I just rebased and squashed the commits. Maybe things got more complicated because this PR is from the main branch of your fork and thus it wasn’t easy for you to rebase on top of swift-format’s main branch.

Yeah, it ended up being more changes than I expected. I'll make sure to be clean and regular on the next one.

Thanks for all the help!

macshome requested review from ahoppen, allevato, bnbarham and shawnhyam as code owners October 25, 2024 14:28

ahoppen reviewed Oct 25, 2024

View reviewed changes

allevato reviewed Oct 28, 2024

View reviewed changes

macshome marked this pull request as draft November 6, 2024 14:48

macshome marked this pull request as ready for review November 12, 2024 18:37

macshome force-pushed the main branch from 636ce0b to 90931a6 Compare December 2, 2024 22:13

macshome force-pushed the main branch from e14af5b to 6fa922a Compare December 5, 2024 15:36

macshome requested a review from shahmishal as a code owner December 5, 2024 15:36

ahoppen force-pushed the main branch from 6fa922a to e14ee65 Compare December 5, 2024 16:07

ahoppen approved these changes Dec 5, 2024

View reviewed changes

ahoppen force-pushed the main branch 2 times, most recently from 8d330cc to e3a2ef4 Compare December 5, 2024 18:52

macshome and others added 2 commits December 5, 2024 11:02

Fix support for FileIterator when working directory is /

003208b

Added an additional check that tests to see if the working directory is root. swiftlang#862

Add an api-breakages file to allow API breakages

8358e38

ahoppen force-pushed the main branch from e3a2ef4 to 8358e38 Compare December 5, 2024 19:02

ahoppen merged commit 3b27ab5 into swiftlang:main Dec 5, 2024
19 checks passed

This was referenced Dec 6, 2024

FileIterator returns broken URLs when the working directory is / #862

Closed

Cannot run swift-format from root (/) #811

Open

Fix support for FileIterator when working directory is / #865

Fix support for FileIterator when working directory is / #865

Conversation

macshome commented Oct 25, 2024 • edited Loading

Simple Fix

Testing

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macshome Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

macshome commented Oct 25, 2024 • edited Loading

allevato commented Oct 25, 2024

macshome commented Oct 25, 2024

macshome commented Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macshome Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macshome commented Nov 6, 2024

macshome commented Nov 12, 2024

Issues I'm running into on Windows

compnerd commented Nov 12, 2024

ahoppen commented Nov 12, 2024

macshome commented Nov 13, 2024

macshome commented Nov 13, 2024

macshome commented Nov 13, 2024

ahoppen commented Nov 13, 2024 • edited Loading

macshome commented Nov 14, 2024 • edited Loading

ahoppen commented Nov 14, 2024

macshome commented Nov 19, 2024

ahoppen commented Nov 19, 2024

macshome commented Nov 19, 2024

macshome commented Nov 26, 2024

macshome commented Dec 2, 2024

ahoppen commented Dec 3, 2024

macshome commented Dec 5, 2024

ahoppen commented Dec 5, 2024

ahoppen commented Dec 5, 2024

macshome commented Dec 6, 2024

Fix support for `FileIterator` when working directory is `/` #865

Fix support for `FileIterator` when working directory is `/` #865

macshome commented Oct 25, 2024 •

edited

Loading

macshome Oct 25, 2024 •

edited

Loading

macshome commented Oct 25, 2024 •

edited

Loading

macshome commented Oct 28, 2024 •

edited

Loading

macshome Oct 28, 2024 •

edited

Loading

ahoppen commented Nov 13, 2024 •

edited

Loading

macshome commented Nov 14, 2024 •

edited

Loading