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.

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

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.

Since just testing for the `/` character at the begining of a path will result in no path ever being relative, we should use `FileManager.default.currentDirectoryPath` to check and see if we are actually running in root.

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.

I updated the tests to actually use `[URL]` instead of `[String]` when looking at the test results. For the working directory changes it was simplest to actually change the working directory and make sure we aren't dropping the first charcters from the paths.

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.

I suppose I could also implement the isRoot property from the swift-tools-support-core as well in FileIterator. That way it would work on Windows as well.

These tests do fail on Windows when trying to change the working directory. I'll update everything and get it working.

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.

main Fix support for FileIterator when working directory is /

a8d37ae

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

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

main Updated test to use actual currentDirectoryPath

4423b8d

Since just testing for the `/` character at the begining of a path will result in no path ever being relative, we should use `FileManager.default.currentDirectoryPath` to check and see if we are actually running in root.

allevato reviewed Oct 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

ahoppen left a comment

ahoppen Oct 25, 2024

macshome Oct 25, 2024

macshome Oct 25, 2024 •

edited

Loading

macshome commented Oct 25, 2024 •

edited

Loading

allevato commented Oct 25, 2024

macshome commented Oct 25, 2024

macshome commented Oct 28, 2024 •

edited

Loading

allevato Oct 28, 2024

macshome Oct 28, 2024

macshome Oct 28, 2024 •

edited

Loading

ahoppen Oct 28, 2024

macshome Oct 29, 2024

macshome Oct 29, 2024

macshome Oct 30, 2024

ahoppen Oct 30, 2024

macshome Oct 31, 2024

ahoppen Oct 31, 2024

Fix support for FileIterator when working directory is / #865

Are you sure you want to change the base?

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

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