-
Notifications
You must be signed in to change notification settings - Fork 124
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' OrderedSet
and OrderedDictionary
#222
base: main
Are you sure you want to change the base?
Adopt Swift Collections' OrderedSet
and OrderedDictionary
#222
Conversation
this is great, thanks for the PR @shahmishal can you help get swiftlang/swift#37431 merged? it is blocking this |
Converted to draft because I need to update |
dependencies: [ | ||
"TSCLibc", | ||
"TSCclibc", | ||
.product(name: "OrderedCollections", package: "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.
Just want to understand this and make sure I didn't do it wrong: Why can't I just use "OrderedCollections"
, instead of .product
?
SwiftPM gives this error when I try to use "OrderedCollections"
directly:
dependency 'OrderedCollections' in target 'TSCBasic' requires explicit declaration; reference the package in the target dependency with '.product(name: "OrderedCollections", package: "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.
generally, these dependencies need to be explicit (as you have). if product and package name are identical (e.g. .product(name: "Foo", package: "Foo")
, its enough to just use the string as short hand (e.g. "Foo"
)
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.
the reason why it needs to be explicit is that it is designed to drive target base dependencies - and avoid needing to clone the package if its not required. one could argue the cloning is not too bad - that what is more important is the the dependency dont get linked - but that argument can go both ways. personally, I think simplifying the manifest so that explicitly setting the package name is not required is worth the clone time, especially with the repository cache in place
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.
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 believe you're right about the reason this was added, but I don't think it is just for performance — I believe that one of the motivations for the target-based dependencies is also so that it's possible to still use a package if one of its dependencies is unavailable but is not needed by products being used (typically to avoid incompatibilities that are introduced by test support libraries and other things that aren't linked into the client). But I'm not 100% sure.
I agree, this is rather tedious, and I wonder if the situation with incompatible libraries arises often enough that it warrants the complexity. Although I suppose that if it does happen then there is no good way to resolve it absent the target-based dependency resolution.
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.
So I don't have actionable suggestion for this PR — those are just thoughts based on the cc: regarding this topic.
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 believe you're right about the reason this was added, but I don't think it is just for performance — I believe that one of the motivations for the target-based dependencies is also so that it's possible to still use a package if one of its dependencies is unavailable but is not needed by products being used (typically to avoid incompatibilities that are introduced by test support libraries and other things that aren't linked into the client). But I'm not 100% sure.
what I was imagining here (and may cause other issues, so needs to be discuss in more details, probably in an amendment pitch/proposal) is that SwiftPM can clone the all the dependencies and use that to build a map of product -> package before determining what packages it can then drop since they are not used. this will allow users only need to specify the product name and SwiftPM can deduce the package name on it own (unless two packages vend the same product name) - much like it is already doing to generate the error message!
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 just read SE-226 to learn about target-based dependency resolution. However, I'm still confused, because in SwiftPM's Package.swift
, I can use "OrderedCollections"
directly instead of .product
. What's different between TSCBasic and SwiftPM's Build
that the former needs explicit declaration for dependency while the latter doesn't?
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.
much like it is already doing to generate the error message
This is also part of my original confusion. If SwiftPM can figure it out, then why do I need to specify which package a target is from.
My understanding of SE-226 is that it helps reduce the number of dependencies' dependencies to be cloned, but a target's immediate dependency packages still need to be cloned regardless (because otherwise you can't link/build the dependencies). Not specifying which package the dependency is in doesn't increase the amount of cloning, but uses more compute power to find the dependency. And it seems to me it might be a good trade-off to use a bit more compute power to reduce user friction.
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.
My understanding of SE-226 is that it helps reduce the number of dependencies' dependencies to be cloned, but a target's immediate dependency packages still need to be cloned regardless (because otherwise you can't link/build the dependencies). Not specifying which package the dependency is in doesn't increase the amount of cloning, but uses more compute power to find the dependency. And it seems to me it might be a good trade-off to use a bit more compute power to reduce user friction.
personally I feel this is worth pitching as an amendment and PRing
92b5aab
to
c2dc3e9
Compare
Can someone help me with the CMake build? I added I know I should add |
@compnerd Is this something you could advise on? My CMake skills are quite rudimentary, otherwise I'd have a go at a suggestion. |
OrderedSet
and OrderedDictionary
OrderedSet
and OrderedDictionary
Id be happy to help, but I'm pretty busy with the concurrency work. The change required for the Windows CI should be relatively simple - I've already added CMake support for SwiftCollections. Adding a checkout and build into the rules at https://github.com/compnerd/swift-build should do the trick. The integration for the CMake build should be relatively simple as well - just build and pass along the paths to the config files that are generated and that should just do the necessary work. |
@swift-ci test |
@WowbaggersLiquidLunch given how CI is set up, the right order here would be to merge swiftlang/swift-package-manager#3595 first, then create a second PR for SwiftPM where we adjust bootstrap to make PackageCollections available to TSC, do a cross PR tests for SwiftPM and TSC and merge both |
I think you mean OrderedCollections instead of PackageCollections? I don't know how the CI is set up. I didn't even know that OrderedCollections needs to be made available to TSC through SwiftPM's bootstrap as well. But what you say sounds good to me. |
They're better optimised and tested.
3070675
to
ff10cd5
Compare
ff10cd5
to
7afde18
Compare
rebased on main and resolved merge conflicts @swift-ci please smoke test |
This comment has been minimized.
This comment has been minimized.
@tomerd I un-drafted this PR because its counterpart in SwiftPM is merged. Could you help take a look at the CMakeLists? I had to change them when resolving merge conflicts, and I'm not sure if I did everything correctly there. |
@swift-ci test |
thanks @WowbaggersLiquidLunch this will take a bit more work as there are dependencies on this this (like the driver) which will need to be adjusted as well |
They're better optimised and tested. This also cuts some weight from TSC.
This PR needs to be tested together with swiftlang/swift-package-manager#3533 and swiftlang/swift#37431.