Skip to content

adopt swift-collections #3681

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
Merged
merged 3 commits into from
Feb 1, 2022
Merged

adopt swift-collections #3681

merged 3 commits into from
Feb 1, 2022

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Aug 20, 2021 •
edited
Loading

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 call-sites to swift-collections

@tomerd
Copy link
Contributor Author

tomerd commented Aug 20, 2021

pending fixes in swift-collections for arm64 linux and simpler dependences tree. cc @neonichu @lorentey

@tomerd
Copy link
Contributor Author

tomerd commented Aug 30, 2021

hi @lorentey, could you work with @compnerd and @neonichu on getting the CMake build working & tested on arm64 linux so we can move forward with this integration

@tomerd tomerd self-assigned this Aug 30, 2021
@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from 7470dbe to e6aaf46 Compare September 13, 2021 18:26
@tomerd tomerd changed the title Revert "Revert "adopt swift-collections (#3632)"" adopt swift-collections Sep 13, 2021
@tomerd
Copy link
Contributor Author

tomerd commented Sep 13, 2021

@swift-ci please smoke test

@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from e6aaf46 to 80da18d Compare October 5, 2021 19:17
@tomerd
Copy link
Contributor Author

tomerd commented Oct 5, 2021

@swift-ci please smoke test

@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from 80da18d to 1f1b1e6 Compare October 5, 2021 19:22
@tomerd tomerd added ready Author believes the PR is ready to be merged & any feedback has been addressed and removed DO NOT MERGE labels Oct 5, 2021
@tomerd
Copy link
Contributor Author

tomerd commented Oct 5, 2021

@swift-ci please smoke test

@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from 1f1b1e6 to c3fb4e8 Compare October 6, 2021 01:53
@tomerd
Copy link
Contributor Author

tomerd commented Oct 6, 2021

@swift-ci please smoke test

WowbaggersLiquidLunch
WowbaggersLiquidLunch reviewed Oct 6, 2021
View reviewed changes
Package.swift Outdated
Comment on lines 135 to 139
name: "Basics",
dependencies: ["SwiftToolsSupport-auto"]),
dependencies: [
.product(name: "OrderedCollections", package: "swift-collections"),
"SwiftToolsSupport-auto"
]),
Copy link
Contributor

@WowbaggersLiquidLunch WowbaggersLiquidLunch Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for this comment after the PR is already labeled "ready".

I see that each file that uses OrderedCollections imports it directly, and no file in Basics uses OrderedCollections. Is Basics's declared dependency on OrderedCollections here a leftover from one of the previous versions of this PR when Basics re-exported it, or does this dependency now have some other use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for bringing it up @WowbaggersLiquidLunch, I will look into simplifying this

Copy link
Member

Choose a reason for hiding this comment

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

IIUC this means that the dependency declaration would need to be duped onto each individual target that imports OrderedCollections. (Which isn't necessarily a bad thing...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right it would be a bit more painful, while this makes it available everywhere. tradeoffs..

@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from c3fb4e8 to 354bb99 Compare December 2, 2021 04:06
@tomerd tomerd requested a review from elsh as a code owner December 2, 2021 04:06
@tomerd
Copy link
Contributor Author

tomerd commented Dec 2, 2021

this is now rebased on top of #3881 so once that is merged we can take this too

@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from 354bb99 to ae005fb Compare December 15, 2021 04:16
@tomerd
Copy link
Contributor Author

tomerd commented Dec 15, 2021

@swift-ci smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Dec 15, 2021

this is now rebased on top of the swift-system changes and ready to go

@neonichu
Copy link
Contributor

@swift-ci smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Dec 15, 2021

CI failing on what seems to be an unrelated LLVM crash

@tomerd
Copy link
Contributor Author

tomerd commented Dec 15, 2021

@swift-ci smoke test

1 similar comment
@tomerd
Copy link
Contributor Author

tomerd commented Dec 18, 2021

@swift-ci smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Dec 20, 2021

@swift-ci smoke test

@yim-lee yim-lee mentioned this pull request Dec 26, 2021
Merged
@tomerd
Copy link
Contributor Author

tomerd commented Jan 13, 2022

@swift-ci please smoke test

@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from 9f809ad to ec0b977 Compare January 15, 2022 03:35
@tomerd
Copy link
Contributor Author

tomerd commented Jan 15, 2022

@swift-ci please smoke test

@tomerd tomerd mentioned this pull request Jan 18, 2022
Closed
tomerd added 2 commits January 18, 2022 15:51
@tomerd
integrate swift-collections
3dc4f27 
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 call-sites to swift-collections
@tomerd
rebase
50bfcbb
@tomerd tomerd force-pushed the revert-3677-revert-collections-again branch from ec0b977 to 50bfcbb Compare January 19, 2022 00:04
@tomerd
Copy link
Contributor Author

tomerd commented Jan 19, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Jan 19, 2022

test toolchain build: swiftlang/swift#40896

@tomerd
Copy link
Contributor Author

tomerd commented Jan 25, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Jan 31, 2022

@swift-ci please smoke test

@tomerd
Copy link
Contributor Author

tomerd commented Feb 1, 2022

@compnerd FYI we are planning to merge this, which may effect the Windows build

@compnerd
Copy link
Member

compnerd commented Feb 1, 2022

I think that this is covered with the current state of the build:
https://github.com/apple/swift-installer-scripts/blob/main/platforms/Windows/devtools.wxs#L144 already packages OrderedCollections.dll and https://github.com/apple/swift/blob/main/utils/build-windows-toolchain.bat#L556 already passes along the proper flags to the build of SPM.

@tomerd tomerd merged commit 92f9d94 into main Feb 1, 2022
@compnerd compnerd deleted the revert-3677-revert-collections-again branch March 26, 2023 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorentey lorentey lorentey left review comments

@WowbaggersLiquidLunch WowbaggersLiquidLunch WowbaggersLiquidLunch left review comments

@neonichu neonichu neonichu approved these changes

@abertelrud abertelrud Awaiting requested review from abertelrud

@friedbunny friedbunny Awaiting requested review from friedbunny

@elsh elsh Awaiting requested review from elsh

Assignees

@tomerd tomerd

Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomerd @neonichu @compnerd @lorentey @WowbaggersLiquidLunch