Adopt Swift Collections' `OrderedSet` and `OrderedDictionary` #3533

WowbaggersLiquidLunch · 2021-06-07T01:14:58Z

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.

compnerd · 2021-06-07T01:46:14Z

This needs updating in the CMake build before it can be merged. The nightlies build needs to be updated for this as well.

WowbaggersLiquidLunch · 2021-06-07T02:02:47Z

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?

finagolfin · 2021-06-07T15:13:51Z

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.

WowbaggersLiquidLunch · 2021-06-07T16:55:27Z

Converted to draft while I work on CMakeLists and the bootstrap script.

Package.swift

Sources/Build/BuildPlan.swift

WowbaggersLiquidLunch · 2021-06-09T17:51:24Z

I finished adding swift-collections to CMakeLists and bootstrap, but wasn't able to test them locally.

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.

Documentation/Contributing.md

Package.swift

abertelrud · 2021-06-14T18:41:18Z

Utilities/bootstrap

-    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'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.

I agree. It makes sense to consolidate them into a function.

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.

WowbaggersLiquidLunch · 2021-06-14T20:39:07Z

I converted this PR into a draft again earlier today, because I found a problem where PubgrubTests passes to DependencyGraphBuilder.serve an ordered dictionary with duplicate keys, which Swift Collection's OrderedDictionary traps.

WowbaggersLiquidLunch · 2021-06-19T17:43:35Z

The problem of duplicate keys isn't really related to Swift Collections' OrderedDictionary, so I opened a separate PR #3564 for it.

…ions’ Swift Collections’ are better optimised and tested.

Also updated version numbers used for swift-argument-parser and swift-crypto in the documentation.

…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.

finagolfin · 2021-06-30T23:31:44Z

This will need to be rebased, as I just made some changes to the same portion of the bootstrap script.

WowbaggersLiquidLunch · 2021-07-01T00:12:22Z

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 build_ functions as @abertelrud suggested.

WowbaggersLiquidLunch · 2021-07-02T00:59:04Z

Closing this in favour of #3590

WowbaggersLiquidLunch requested review from abertelrud, friedbunny, neonichu and tomerd as code owners June 7, 2021 01:14

This was referenced Jun 7, 2021

[update-checkout] add swift-collections swiftlang/swift#37809

Closed

Adopt Swift Collections' OrderedSet and OrderedDictionary swiftlang/swift-tools-support-core#222

Open

WowbaggersLiquidLunch marked this pull request as draft June 7, 2021 16:54

tomerd reviewed Jun 7, 2021

View reviewed changes

Package.swift Outdated Show resolved Hide resolved

tomerd reviewed Jun 7, 2021

View reviewed changes

Sources/Build/BuildPlan.swift Show resolved Hide resolved

WowbaggersLiquidLunch force-pushed the adopt-swift-collections branch 2 times, most recently from a48848b to 374bf40 Compare June 9, 2021 17:23

WowbaggersLiquidLunch marked this pull request as ready for review June 9, 2021 17:47

WowbaggersLiquidLunch commented Jun 9, 2021

View reviewed changes

Documentation/Contributing.md Outdated Show resolved Hide resolved

WowbaggersLiquidLunch force-pushed the adopt-swift-collections branch from 374bf40 to d0722e3 Compare June 13, 2021 22:24

WowbaggersLiquidLunch marked this pull request as draft June 14, 2021 18:17

abertelrud reviewed Jun 14, 2021

View reviewed changes

Package.swift Outdated Show resolved Hide resolved

abertelrud reviewed Jun 14, 2021

View reviewed changes

tomerd self-assigned this Jun 14, 2021

tomerd added the WIP Work in progress label Jun 15, 2021

WowbaggersLiquidLunch mentioned this pull request Jun 19, 2021

[PubgrubTests] Handle duplicate product keys in DependencyGraphBuilder.serve #3564

Merged

WowbaggersLiquidLunch changed the title ~~Adopt Swift Collection's OrderedSet and OrderedDictionary~~ Adopt Swift Collections' OrderedSet and OrderedDictionary Jun 19, 2021

WowbaggersLiquidLunch added 2 commits June 19, 2021 20:17

replace TSC's OrderedSet and OrderedDictionary with Swift Collect…

de1cd10

…ions’ Swift Collections’ are better optimised and tested.

add swift-collections to CMakeLists and bootstrap

93c63f5

[doc] add cloning swift-collection to bootstrap documentation

cfc1047

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 June 20, 2021 00:18

This was referenced Jul 1, 2021

[Bootstrap] Consolidate some build_... functions into build_dependency #3582

Merged

adopt swift-collections #3590

Closed

WowbaggersLiquidLunch closed this Jul 2, 2021

Adopt Swift Collections' OrderedSet and OrderedDictionary #3533

Adopt Swift Collections' OrderedSet and OrderedDictionary #3533

Uh oh!

Conversation

WowbaggersLiquidLunch commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

compnerd commented Jun 7, 2021

Uh oh!

WowbaggersLiquidLunch commented Jun 7, 2021

Uh oh!

finagolfin commented Jun 7, 2021

Uh oh!

WowbaggersLiquidLunch commented Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WowbaggersLiquidLunch commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abertelrud Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

WowbaggersLiquidLunch Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

WowbaggersLiquidLunch Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

WowbaggersLiquidLunch commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WowbaggersLiquidLunch commented Jun 19, 2021

Uh oh!

finagolfin commented Jun 30, 2021

Uh oh!

WowbaggersLiquidLunch commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WowbaggersLiquidLunch commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Adopt Swift Collections' `OrderedSet` and `OrderedDictionary` #3533

Adopt Swift Collections' `OrderedSet` and `OrderedDictionary` #3533

WowbaggersLiquidLunch commented Jun 7, 2021 •

edited

Loading

WowbaggersLiquidLunch commented Jun 7, 2021 •

edited

Loading

WowbaggersLiquidLunch commented Jun 9, 2021 •

edited

Loading

WowbaggersLiquidLunch commented Jun 14, 2021 •

edited

Loading

WowbaggersLiquidLunch commented Jul 1, 2021 •

edited

Loading