Use SystemPath for ContainerResource.Filesystem.#1523
Conversation
Replaces URL-based path normalization with SystemPackage.FilePath in the three Filesystem factory methods (block, volume, virtiofs): - source: URL(fileURLWithPath: source).absolutePath() → FilePath(source).string All existing callers (Parser, Utility, SnapshotStore, RuntimeConfigurationTests) already pass absolute paths; the URL round-trip was a no-op in practice. FilePath preserves separator normalization equivalent to the URL form. Following the migration pattern from apple#1480 (HostDNSResolver), apple#1518 (PacketFilter), and discussion apple#1481.
chrisgeo
force-pushed
the
feat/chaos-1452-filesystem-absolute-path
branch
from
d6c355d to
2bfc155
Compare
| ) -> Filesystem { | ||
| .init( | ||
| type: .block(format: format, cache: cache, sync: sync), | ||
| source: URL(fileURLWithPath: source).absolutePath(), |
There was a problem hiding this comment.
For this if we want to be functionally equivalent, we'd need something like this:
func absoluteFilePath(for source: String) -> FilePath {
let path = FilePath(source)
guard path.isRelative else { return path.lexicallyNormalized() }
return FilePath(FileManager.default.currentDirectoryPath)
.pushing(path)
.lexicallyNormalized()
}There was a problem hiding this comment.
I went the private function route to keep the functionality centered on this changeset.
Please let me know if you'd prefer that be adjusted. Thanks again for the feedback!
Code Coverage
|
Restores functional equivalence with the prior URL(fileURLWithPath:).absolutePath() behavior, which resolved relative paths against the current working directory. Plain FilePath(source).string left relative inputs unresolved — a silent regression for any future caller. Per review feedback on apple#1523.
|
@chrisgeo A little discussion on this one as well. At first I was thinking, why doesn't the PR use FilePath for the source and destination path properties? For the source it needs to stay string because, possibly among other things, the source could be a network location. I think that in a follow-on we could consider changing the destination path to a FilePath, but what you have in this PR is a good first step toward that. Can you think of any possible instance where the destination path would be something else? Pretty sure any |
|
See discussion above, but the PR is fine as is, merged and thank you! |
We shouldn't rush such a change. Leaving it as String allows us to have the flexibility to define other meanings, such as for cases where the receiver of a Filesystem needs to compute the eventual destination path. |
@jglogan Sorry - I missed this before you merged. I took a look couldn't come up with anything we'd miss with with mount(2) options since I recall all unix-based system Happy to dive into options for a follow-up when I can |
3 participants
Type of Change
Motivation and Context
Continues the URL → FilePath migration discussed in #1481. Tiny diff (7 lines, 1 file).
Affects the three
Filesystemfactory methods:block,volume,virtiofs.The original
URL(fileURLWithPath: source).absolutePath()was a no-op round-trip on absolute inputs (which is what every caller passes today —Parser.swift,Utility.swift,SnapshotStore.swift, andRuntimeConfigurationTests).FilePath(source).stringpreserves the same separator normalization without the URL detour.Testing
swift test --filter ContainerResourceTestspasses — no regressions)