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.

Sign up for GitHub

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

Jump to bottom

refactor RelativePath to allow late stage canonicalization in support of windows #369

Merged
merged 7 commits into from
Apr 17, 2023
Merged

refactor RelativePath to allow late stage canonicalization in support of windows #369

merged 7 commits into from
Apr 17, 2023

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Nov 17, 2022 •
edited
Loading

motivation: delay canonicalization of relative path to the construction of absolute path from it, to better fit how windows paths work

changes:

  • remove the canonicalizing, non-throwing initializer of relative path
  • for tests, conform RelativePath and AbsolutePath to ExpressibleByStringLiteral and ExpressibleByStringInterpolation to make it easier to pass in string and static strings
  • remove incorrect validation of ~ as invalid relative path
  • update call sites and test

@tomerd tomerd changed the title refactoring of relative path to allow late stage canonicalization in support of windows [wip] refactoring of relative path to allow late stage canonicalization in support of windows Nov 17, 2022
@tomerd tomerd mentioned this pull request Nov 17, 2022
Merged
@tomerd
Copy link
Contributor Author

tomerd commented Nov 17, 2022

companion: swiftlang/swift-package-manager#5910

@tomerd tomerd mentioned this pull request Apr 11, 2023
Closed
@tomerd tomerd force-pushed the refactor/relative-path branch 2 times, most recently from 024c262 to 78feb0a Compare April 13, 2023 21:04
@tomerd tomerd changed the title [wip] refactoring of relative path to allow late stage canonicalization in support of windows refactore of relative path to allow late stage canonicalization in support of windows Apr 13, 2023
@tomerd tomerd force-pushed the refactor/relative-path branch from 78feb0a to 8edc44e Compare April 13, 2023 22:14
@tomerd
Copy link
Contributor Author

tomerd commented Apr 13, 2023

testing via swiftlang/swift-package-manager#5910

@tomerd tomerd changed the title refactore of relative path to allow late stage canonicalization in support of windows refactor RelativePath to allow late stage canonicalization in support of windows Apr 14, 2023
@compnerd
Copy link
Member

Hmm, merging this locally seems to not build on Windows? :/

@compnerd
Copy link
Member

@swift-ci please test Windows platform

@compnerd
Copy link
Member

@tomerd did you forget part of the commit?

S:\SourceCache\swift-project\swift-tools-support-core\Sources\TSCBasic\Path.swift:524:14: error: no exact matches in call to initializer
        self.init(normalizingAbsolutePath: path)

There is no Windowspath(normalizingAbsolutePath:) that I can see.

@compnerd
Copy link
Member

@swift-ci please test Windows platform

compnerd
compnerd requested changes Apr 14, 2023
View reviewed changes
Sources/TSCBasic/Path.swift Outdated Show resolved Hide resolved
compnerd
compnerd reviewed Apr 14, 2023
View reviewed changes
Sources/TSCBasic/Path.swift Outdated Show resolved Hide resolved
Sources/TSCBasic/Path.swift Show resolved Hide resolved
@compnerd
Copy link
Member

This does seem to regress the test suite:

Test Case 'ManifestSourceGenerationTests.testAdvancedFeatures' started at 2023-04-14 09:37:04.984
TSCBasic/Path.swift:974: Assertion failed
Current stack trace:
0    (null)                             0x00007ffe873af220 swift_stdlib_reportFatalErrorInFile + 132

neonichu
neonichu reviewed Apr 17, 2023
View reviewed changes
Sources/TSCBasic/Path.swift Outdated
@@ -338,6 +321,8 @@ public struct RelativePath: Hashable, Sendable {
}
}



Copy link
Contributor

Choose a reason for hiding this comment

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

why

tomerd
tomerd commented Apr 17, 2023
View reviewed changes
Sources/TSCBasic/Path.swift Outdated Show resolved Hide resolved
@tomerd tomerd requested a review from MaxDesiatov as a code owner April 17, 2023 18:13
@tomerd tomerd force-pushed the refactor/relative-path branch from 968d308 to e913e15 Compare April 17, 2023 18:15
@tomerd
Copy link
Contributor Author

tomerd commented Apr 17, 2023

tested via swiftlang/swift-package-manager#5910

@tomerd tomerd merged commit b2527eb into swiftlang:main Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd requested changes

@neonichu neonichu neonichu approved these changes

@aciidgh aciidgh Awaiting requested review from aciidgh

@abertelrud abertelrud Awaiting requested review from abertelrud

@friedbunny friedbunny Awaiting requested review from friedbunny

@MaxDesiatov MaxDesiatov Awaiting requested review from MaxDesiatov MaxDesiatov is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tomerd @compnerd @neonichu @abertelrud