Skip to content
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

Adopt Swift Collections' OrderedSet and OrderedDictionary #222

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Adopt Swift Collections' OrderedSet and OrderedDictionary #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2de87c
adopt Swift Collection's `OrderedSet` and `OrderedDictionary`
WowbaggersLiquidLunch Feb 1, 2022
d5359c1
add swift-collections to `TSCBasic` and `TSCUtiility`’s `CMakeLists`
WowbaggersLiquidLunch Feb 1, 2022
7afde18
cmake
WowbaggersLiquidLunch Feb 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,21 @@
# macOS
.DS_Store

# SwiftPM
.swiftpm
/.build
/Packages
Package.resolved

# CMake
CMakeCache.txt
CMakeFiles

# Xcode
/*.xcodeproj
xcuserdata/
.swiftpm
build

# VScode
.vscode
Package.resolved

/Packages
build
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ find_package(Foundation QUIET)
find_package(Threads QUIET)
find_package(SQLite3 REQUIRED)
find_package(SwiftSystem CONFIG REQUIRED)
find_package(SwiftCollections REQUIRED)

add_subdirectory(Sources)
add_subdirectory(cmake/modules)
35 changes: 21 additions & 14 deletions Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ let package = Package(
name: "TSCTestSupport",
targets: ["TSCTestSupport"]),
],
dependencies: [],
targets: [

// MARK: Tools support core targets
Expand All @@ -63,17 +62,23 @@ let package = Package(
/** TSCBasic support library */
name: "TSCBasic",
dependencies: [
"TSCLibc",
"TSCclibc",
.product(name: "SystemPackage", package: "swift-system"),
"TSCLibc",
"TSCclibc",
.product(name: "OrderedCollections", package: "swift-collections"),
Copy link
Contributor Author

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")'

Copy link
Contributor

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")

Copy link
Contributor

@tomerd tomerd Jun 7, 2021 •
edited
Loading

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

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @neonichu @abertelrud

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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!

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

.product(name: "SystemPackage", package: "swift-system"),
],
exclude: CMakeFiles + ["README.md"]),
.target(
/** Abstractions for common operations, should migrate to TSCBasic */
name: "TSCUtility",
dependencies: ["TSCBasic", "TSCclibc"],
dependencies: [
"TSCBasic",
"TSCclibc",
.product(name: "OrderedCollections", package: "swift-collections"),
],
exclude: CMakeFiles),


// MARK: Additional Test Dependencies

.target(
Expand Down Expand Up @@ -102,17 +107,19 @@ let package = Package(
)

/// When not using local dependencies, the branch to use for llbuild and TSC repositories.
let relatedDependenciesBranch = "main"
let relatedDependenciesBranch = "main"

if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil {
package.dependencies += [
.package(url: "https://github.com/apple/swift-system.git", .upToNextMinor(from: "1.1.1")),
]
} else {
if ProcessInfo.processInfo.environment["SWIFTCI_USE_LOCAL_DEPS"] == nil {
package.dependencies += [
.package(path: "../swift-system"),
]
}
.package(url: "https://github.com/apple/swift-collections.git", .branch("main")),
.package(url: "https://github.com/apple/swift-system.git", .upToNextMinor(from: "1.1.1")),
]
} else {
package.dependencies += [
.package(path: "../swift-collections"),
.package(path: "../swift-system"),
]
}

// FIXME: conditionalise these flags since SwiftPM 5.3 and earlier will crash
// for platforms they don't know about.
Expand Down
3 changes: 1 addition & 2 deletions Sources/TSCBasic/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ add_library(TSCBasic
Lock.swift
OSLog.swift
ObjectIdentifierProtocol.swift
OrderedDictionary.swift
OrderedSet.swift
WritableByteStream.swift
Path.swift
PathShims.swift
Expand All @@ -58,6 +56,7 @@ target_compile_options(TSCBasic PUBLIC
"$<$<PLATFORM_ID:Windows>:SHELL:-Xcc -D_CRT_SECURE_NO_WARNINGS>")
target_link_libraries(TSCBasic PUBLIC
SwiftSystem::SystemPackage
SwiftCollections::OrderedCollections
TSCLibc)
target_link_libraries(TSCBasic PRIVATE
TSCclibc)
Expand Down
8 changes: 5 additions & 3 deletions Sources/TSCBasic/GraphAlgorithms.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
/*
This source file is part of the Swift.org open source project

Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See http://swift.org/LICENSE.txt for license information
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import OrderedCollections

public enum GraphError: Error {
/// A cycle was detected in the input.
case unexpectedCycle
Expand Down Expand Up @@ -69,7 +71,7 @@ public func topologicalSort<T: Hashable>(

// Otherwise, visit each adjacent node.
for succ in try successors(node) {
guard stack.append(succ) else {
guard stack.append(succ).inserted else {
// If the successor is already in this current stack, we have found a cycle.
//
// FIXME: We could easily include information on the cycle we found here.
Expand Down Expand Up @@ -120,7 +122,7 @@ public func findCycle<T: Hashable>(
// FIXME: Convert to stack.
func visit(_ node: T, _ successors: (T) throws -> [T]) rethrows -> (path: [T], cycle: [T])? {
// If this node is already in the current path then we have found a cycle.
if !path.append(node) {
if !path.append(node).inserted {
let index = path.firstIndex(of: node)!
return (Array(path[path.startIndex..<index]), Array(path[index..<path.endIndex]))
}
Expand Down
125 changes: 0 additions & 125 deletions Sources/TSCBasic/OrderedDictionary.swift
View file
Open in desktop

This file was deleted.

Loading