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.

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

SWBUtil: use FileManager for more operations #189

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

compnerd
Copy link
Member

unlink is deprecated on Windows, and RemoveFileW should be preferred. However, that would limit the path to MAX_PATH (261) characters. Prefer to use `FileManager to remove the file to avoid the path limit.

Adopt the fileManager path in more locations.

@compnerd
Copy link
Member Author

This is an extended change split out from the #184 based on @jakepetroules' comment that the use of raw C APIs is no longer a design constraint. This generally reduces complexity as FileManager already handles these error cases. On Windows, this also makes code more robust as this allows us to work with long paths.

@compnerd
Copy link
Member Author

@swift-ci please test

@jakepetroules
Copy link
Collaborator

On Windows, this also makes code more robust as this allows us to work with long paths.

That alone is a big reason to do this, I completely forgot about the POSIX APIs not handling long paths!

@jakepetroules
Copy link
Collaborator

@swift-ci test

// If the item at the path is a symlink, then we check whether it's a broken symlink or points to something that is not a directory.
if destinationExists {
// The destination does exist, so it's not a directory.
throw StubError.error("File is a symbolic link which references a path which is not a directory: \(path.str)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test expectations need to be updated to accommodate these specific error strings going away.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't see a good way to handle the check. There is no guarantee that the message is going to be identical across platforms and Foundation implementations :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine if we just check that it does fail in the expected cases without checking the specific error string.

`unlink` is deprecated on Windows, and `RemoveFileW` should be
preferred. However, that would limit the path to `MAX_PATH` (261)
characters. Prefer to use `FileManager to remove the file to avoid the
path limit.

Adopt the fileManager path in more locations.
@compnerd
Copy link
Member Author

compnerd commented Mar 2, 2025

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants