-
Notifications
You must be signed in to change notification settings - Fork 1.4k
adopt swift-collections #3590
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
adopt swift-collections #3590
Conversation
|
this is the SwiftPM counterpart for to swiftlang/swift-tools-support-core#222 |
|
@swift-ci please smoke test |
| @_exported import OrderedCollections | ||
| @_exported import TSCBasic | ||
| // override TSC versions until deprecated | ||
| // TODO: remove once TSC removes these | ||
| public typealias OrderedSet = OrderedCollections.OrderedSet | ||
| public typealias OrderedDictionary = OrderedCollections.OrderedDictionary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much cleaner solution than what I did in #3533.
Just one question, though: Since there are only a few files that use OrderedCollections, is there any performance disadvantage for files that don't use OrderedCollections to import Basics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I totally missed #3533. sorry about that!
is there any performance disadvantage for files that don't use OrderedCollections to import Basics?
I dont believe so
| args.swift_argument_parser_source_dir = os.path.join(args.project_root, "..", "swift-argument-parser") | ||
| args.swift_driver_source_dir = os.path.join(args.project_root, "..", "swift-driver") | ||
| args.swift_crypto_source_dir = os.path.join(args.project_root, "..", "swift-crypto") | ||
| args.swift_collections_source_dir = os.path.join(args.project_root, "..", "swift-collections") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in this file might conflict with #3582
| import Build | ||
| import PackageGraph | ||
| import SPMBuildCore | ||
| import TSCBasic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By a rough count, there are about 10% of changes to imports not because of the changes to Basics. Would it be better to separate them into their own PR, to reduce the amount of files changed in this PR? Although, the PR probably still would contain changes to about 200 files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, this is why I ended up doing together since it would anyways be a huge (# of files) PR
| $> git clone https://github.com/apple/swift-crypto --branch 1.1.3 | ||
| ``` | ||
|
|
||
| 5. Clone [swift-collections](https://github.com/apple/swift-collections) beside the SwiftPM directory and check out tag with the [latest version](https://github.com/apple/swift-collections/tags). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be list number 6, instead of 5
|
|
||
| For example, if the latest tag is 0.0.3: | ||
| ```bash | ||
| $> git clone https://github.com/apple/swift-collections --branch 0.0.3 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the command examples be indented by 3 spaces so it's structurally more clear that they're part of the list items?
Also, could argument parser and swift crypto's versions in the commands be updated to 0.4.3 and 1.1.4, to align with what's in Package.swift?
| See http://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
| */ | ||
|
|
||
| import TSCBasic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be removing the TSCBasic imports from all files since it currently has plenty of other types besides collections. We may want to remove the exports file prior not having TSCBasic anymore and then we have to put all those imports back again. If we have any files that only import TSCBasic for collections, we can remove the import from those of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I did here was to replace the direct import on TSCBasic with an import of Basics which currently re-exports by TSCBasics. The idea is that we can evolve Basics over time until we can eventually eliminate the need in TSCBasic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda feel like that could make it harder to remove TSCBasic? Since now every client of Basics will see the API of TSCBasic for an extended period of time. IMO, it would be good to get rid of the re-export quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is #3595 better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, I think I prefer that approach.
motivation: replace TSC versions of ordered collections with the new ones from swift-collections changes: * pull in swift-collections as a dependency * update bootstrap script * adapt callsites to swift-collections * reduce redundant imports, especially in tests across Basics, TSCBasic, TSCUtilities and SPMTestSupport
e941551 to
6c02248
Compare
|
@swift-ci please smoke test |
motivation: replace TSC versions of ordered collections with the new ones from swift-collections
changes: