Adopt Swift Collections' OrderedSet and OrderedDictionary
#3533
Conversation
WowbaggersLiquidLunch
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
|
This needs updating in the CMake build before it can be merged. The nightlies build needs to be updated for this as well. |
Sorry I don't really understand what this means. Is there something I should do in order for the CMake and nightly builds to be updated? |
|
I don't know what nightlies refers to, but the CMake build is used to bootstrap SPM on the CI. Take a look at when swift-crypto was added, #3202, for an example of the kinds of changes that are needed to add a Swift package dependency. |
|
Converted to draft while I work on |
WowbaggersLiquidLunch
force-pushed
the
adopt-swift-collections
branch
2 times, most recently
from
a48848b
to
374bf40
Compare
|
I finished adding swift-collections to Also, this PR is still blocked by swiftlang/swift-tools-support-core#222, because I haven't figured out how to add swift-collections to its CMake build. |
WowbaggersLiquidLunch
force-pushed
the
adopt-swift-collections
branch
from
374bf40
to
d0722e3
Compare
| build_with_cmake(args, cmake_flags, args.swift_crypto_source_dir, args.swift_crypto_build_dir) | ||
| build_with_cmake(args, cmake_flags, args.tsc_source_dir, args.tsc_build_dir) | ||
|
|
||
| def build_yams(args): |
There was a problem hiding this comment.
There's a lot of duplication here — I realize that there was already some, but this is adding another case of it. Would it make sense to consolidate some of this into a function? I did that with the code to install a dylib and its associated module, and it might make sense to follow that model and pass parameters as needed. The output directory could potentially be a return value.
There was a problem hiding this comment.
I agree. It makes sense to consolidate them into a function.
There was a problem hiding this comment.
I opened #3582 to consolidate the functions, instead of doing it in this PR, in order to avoid merge conflicts before this PR is ready.
|
I converted this PR into a draft again earlier today, because I found a problem where |
WowbaggersLiquidLunch
changed the title
OrderedSet and OrderedDictionaryOrderedSet and OrderedDictionary
|
The problem of duplicate keys isn't really related to Swift Collections' |
…ions’ Swift Collections’ are better optimised and tested.
Also updated version numbers used for swift-argument-parser and swift-crypto in the documentation.
WowbaggersLiquidLunch
force-pushed
the
adopt-swift-collections
branch
from
d0722e3
to
cfc1047
Compare
…ependencyGraphBuilder.serve` [One of the callers](https://github.com/apple/swift-package-manager/blob/20eba126ffa32088abb46d642bd0481dbae212ef/Tests/PackageGraphTests/PubgrubTests.swift#L472-L475) of the function passes in a dictionary literal with duplicate keys. Although the logic in this function suggests that duplicate keys are allowed, `TSCBasic.OrderedDictionary` preserves only [the final](https://github.com/apple/swift-tools-support-core/blob/21a79185b2ea8f89b9253ed8cdf2338bf564c499/Sources/TSCBasic/OrderedDictionary.swift#L97-L99) of all duplicate keys' values in a dictionary literal. In addition, with swiftlang#3533, Swift Collections' `OrderedDictionary` [traps](https://github.com/apple/swift-collections/blob/c0549b6284aadd5fd13156316f43fcb43c7fca77/Sources/OrderedCollections/OrderedDictionary/OrderedDictionary%2BInitializers.swift#L88-L91) on duplicate keys. `KeyValuePairs` seems like the right replacement that allows duplicate keys.
…er.serve` (#3564) * [PubgrubTests] Replace `OrderedDictionary` with `KeyValuePairs` in `DependencyGraphBuilder.serve` [One of the callers](https://github.com/apple/swift-package-manager/blob/20eba126ffa32088abb46d642bd0481dbae212ef/Tests/PackageGraphTests/PubgrubTests.swift#L472-L475) of the function passes in a dictionary literal with duplicate keys. Although the logic in this function suggests that duplicate keys are allowed, `TSCBasic.OrderedDictionary` preserves only [the final](https://github.com/apple/swift-tools-support-core/blob/21a79185b2ea8f89b9253ed8cdf2338bf564c499/Sources/TSCBasic/OrderedDictionary.swift#L97-L99) of all duplicate keys' values in a dictionary literal. In addition, with #3533, Swift Collections' `OrderedDictionary` [traps](https://github.com/apple/swift-collections/blob/c0549b6284aadd5fd13156316f43fcb43c7fca77/Sources/OrderedCollections/OrderedDictionary/OrderedDictionary%2BInitializers.swift#L88-L91) on duplicate keys. `KeyValuePairs` seems like the right replacement that allows duplicate keys. * [PubgrubTests] append, not assign, `packageDependencies` to value in dictionary keyed by `product` `dependencies` passed to `DependencyGraphBuilder.serve` can have duplicate product keys, so when `dependencies` are iterated, the same `product` can appear more than once, each time with possibly different `filteredDependencies`. Assigning `packageDependencies` mapped from a `product`'s `filteredDependencies` to the value in a different dictionary keyed by the same `product` overrides any existing value from a previous assignment. Appending `packageDependencies` instead preserves all previous values.
|
This will need to be rebased, as I just made some changes to the same portion of the bootstrap script. |
Thanks for the notice. I'm actually going to open a new PR (update: #3582) first that consolidate all the |
|
Closing this in favour of #3590 |
They're better optimised and tested than TSCBasic's.
This PR needs to be tested together with swiftlang/swift-tools-support-core#222 and swiftlang/swift#37431.