[NFC] Remove `SupportedPlatforms`, add `PlatformVersionProvider` #7161

MaxDesiatov · 2023-12-03T00:05:55Z

Unblocks PackageGraph value types refactoring in #7160 and macros cross-compilation fix in #7118.

Motivation:

The PackageModel module has no business of defining supported platforms computation for resolved targets, products, and packages. This belongs to PackageGraph, but was previously leaking out into PackageModel by passing the derivedXCTestPlatformProvider closure around. That prevented us from marking SupportedPlatforms as Hashable and marking any of Resolved* types as value types also Hashable. In the future, when we need to make them Sendable, passing mutable state captured in a closure could become problematic.

Modifications:

Add new PlatformVersionProvider type, which is a value type that can be easily made Hashable and Sendable. Instead of passing closures around, platform version computation code is now gathered in a single file (new PlatformVersionProvider.swift), with all possible context captured in its inner enum Implementation.

This new type is adopted by Resolved* types in the PackageGraph module, which actively uses minimum platform version computations.

Renamed func getDerived to func getSupportedPlatform, now that this function delegating to PlatformVersionProvider is declared on Resolved* types.

Result:

SupportedPlatforms type, which is nothing more than [SupportedPlatform] array with an additional closure, can be removed. Types that previously had properties of SupportedPlatforms can be easily converted to value types and made Hashable in a way that accounts for their content instead of class instance identity.

Multiple places in our codebase pass around and store `[any PackageConditionProtocol]` arrays of existentials with `FIXME` notes that those should be sets and not arrays. Adding `Hashable` requirement to `PackageConditionProtocol` doesn't fix the issue, since existentials of this protocol still wouldn't conform to `Hashable`. A simple alternative is to create `enum PackageCondition` with cases for each condition type. We only have two of those types and they're always declared in the same package. There's no use case for externally defined types for this protocol. That allows us to convert uses of `[any PackageConditionProtocol]` to `Set<PackageCondition>`. Existing protocol kept around until clients of SwiftPM can migrate to the new `PackageCondition` enum.

Depends on #7117, unblocks #7160. The `PackageModel` module has no business of defining supported platforms computation for resolved targets, products, and packages. This belongs to `PackageGraph`, but was previously leaking out into `PackageModel` by passing the `derivedXCTestPlatformProvider` closure around. That prevented us from converting marking `SupportedPlatforms` as `Hashable` and marking any of `Resolved*` types as value types and also `Hashable`. In the future, when we'll also need to make them `Sendable`, this could become problematic, passing so much state captured in a closure, mostly by accident.

tomerd

code changes seem fine, @neonichu is more familiar with the logic so would be good for him to review

Sources/Build/BuildDescription/ProductBuildDescription.swift

MaxDesiatov · 2023-12-05T17:30:48Z

@swift-ci test

MaxDesiatov · 2023-12-05T17:30:52Z

@swift-ci test windows

MaxDesiatov · 2023-12-05T19:16:43Z

@swift-ci test

MaxDesiatov · 2023-12-07T20:09:15Z

@swift-ci test

MaxDesiatov · 2023-12-07T20:25:26Z

@swift-ci test windows

tomerd · 2023-12-08T19:25:33Z

@swift-ci test windows

neonichu · 2023-12-12T21:41:37Z

Sources/PackageGraph/Resolution/PlatformVersionProvider.swift

+            }
+            return platforms.first!.version
+
+        case .customXCTestMinimumDeploymentTargets(let customXCTestMinimumDeploymentTargets):


Since we are creating the implementation enum here, I wonder if we should make custom and default two separate cases?

Good catch, I also found that Implementation.empty case is unused. Ready for review 🙂

MaxDesiatov · 2023-12-13T12:59:28Z

@swift-ci test

MaxDesiatov · 2023-12-13T12:59:42Z

@swift-ci test windows

MaxDesiatov · 2023-12-13T16:26:43Z

@swift-ci test

MaxDesiatov · 2023-12-13T16:27:08Z

@swift-ci test windows

MaxDesiatov · 2023-12-13T17:31:37Z

@swift-ci test windows

Depends on #7161. Unblocks macros cross-compilation fix in #7118. ### Motivation: Value types are easier to reason about as they prevent "spooky action at a distance" bugs, and can be easily made `Equatable`, `Hashable`, and most important of all `Sendable`. ### Modifications: Converted `ResolvedTarget`, `ResolvedProduct`, and `ResolvedPackage` from `final class`es to `struct`s. Renamed `underlyingTarget`, `underlyingProduct`, and `underlyingPackage` to just `underlying` to avoid tautological patterns like `target.underlyingTarget`. ### Result: It's easier to apply changes in `PackageGraph`. Also, in my benchmarks done locally this makes package graph resolution consistently ~5% faster when resolving the package graph of SwiftPM's own `Package.swift`. #### `main` branch: ``` ╒══════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕ │ Metric │ p0 │ p25 │ p50 │ p75 │ p90 │ p99 │ p100 │ Samples │ ╞══════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡ │ Time (system CPU) (ms) │ 7 │ 8 │ 11 │ 14 │ 15 │ 18 │ 18 │ 25 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (total CPU) (ms) │ 12 │ 13 │ 16 │ 19 │ 20 │ 23 │ 23 │ 25 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (user CPU) (μs) │ 4688 │ 4857 │ 4890 │ 4956 │ 5107 │ 5347 │ 5347 │ 25 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (wall clock) (ms) │ 390 │ 397 │ 402 │ 409 │ 414 │ 420 │ 420 │ 25 │ ╘══════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛ ``` #### This PR's branch: ``` ╒══════════════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕ │ Metric │ p0 │ p25 │ p50 │ p75 │ p90 │ p99 │ p100 │ Samples │ ╞══════════════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡ │ Time (system CPU) (ms) │ 6 │ 8 │ 10 │ 13 │ 16 │ 17 │ 17 │ 26 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (total CPU) (ms) │ 11 │ 13 │ 15 │ 18 │ 20 │ 21 │ 21 │ 26 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (user CPU) (μs) │ 4476 │ 4567 │ 4648 │ 4796 │ 4931 │ 4998 │ 4998 │ 26 │ ├──────────────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤ │ Time (wall clock) (ms) │ 375 │ 379 │ 384 │ 391 │ 394 │ 398 │ 398 │ 26 │ ╘══════════════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛ ``` ### Future directions: I'm unable to fully remove `Resolved*Builder` classes yet, as they rely on `Target` being a reference type and module aliasing relying on side effects of modifying instances of `Target`. In turn, `Target` is a class hierarchy, which we should convert to multiple value types that use composition in the future. In the meantime this also prevents us from making it, and all types that contain it, `Sendable`.

MaxDesiatov added 10 commits December 2, 2023 20:38

Add doc comment to the new type

6fd1606

Fix tests regression

456959e

Remove custom ResolvedTarget.Dependency conformance

420f01a

Roll back to arrays and reduce the diff

76121f4

Update comments and reduce the diff

4bd4bb4

Reduce the diff

d6c0c8a

Reduce the diff

f3004a7

Reduce the diff to track the regression

a6760d9

MaxDesiatov added concurrency no functional change No user-visible functional changes included modules graph Modules dependency resolution labels Dec 3, 2023

MaxDesiatov requested review from neonichu and tomerd as code owners December 3, 2023 00:05

This was referenced Dec 3, 2023

Make Resolved* in PackageGraph value types #7160

Merged

Support macros when cross-compiling #7118

Merged

MaxDesiatov self-assigned this Dec 3, 2023

MaxDesiatov removed the concurrency label Dec 3, 2023

Base automatically changed from maxd/package-condition to main December 5, 2023 16:59

tomerd approved these changes Dec 5, 2023

View reviewed changes

Sources/Build/BuildDescription/ProductBuildDescription.swift Outdated Show resolved Hide resolved

MaxDesiatov added 2 commits December 5, 2023 17:15

Merge branch 'main' into maxd/platform-version-provider

9387af3

Rename getDerived to getSupportedPlatform

b777e12

Merge branch 'main' into maxd/platform-version-provider

5b6670e

MaxDesiatov mentioned this pull request Dec 11, 2023

Verify product dependencies w/ target platform, vs. hardcoded macOS #6963

Merged

neonichu reviewed Dec 12, 2023

View reviewed changes

Address PR feedback

20b84c9

MaxDesiatov requested a review from neonichu December 13, 2023 12:59

Fix build error

49e0955

neonichu approved these changes Dec 13, 2023

View reviewed changes

MaxDesiatov enabled auto-merge (squash) December 13, 2023 17:59

MaxDesiatov merged commit 10be62b into main Dec 13, 2023

MaxDesiatov deleted the maxd/platform-version-provider branch December 13, 2023 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC] Remove `SupportedPlatforms`, add `PlatformVersionProvider` #7161

[NFC] Remove `SupportedPlatforms`, add `PlatformVersionProvider` #7161

MaxDesiatov commented Dec 3, 2023 •

edited

Loading

tomerd left a comment •

edited

Loading

MaxDesiatov commented Dec 5, 2023

MaxDesiatov commented Dec 5, 2023

MaxDesiatov commented Dec 5, 2023

MaxDesiatov commented Dec 7, 2023

MaxDesiatov commented Dec 7, 2023

tomerd commented Dec 8, 2023

neonichu Dec 12, 2023

MaxDesiatov Dec 13, 2023 •

edited

Loading

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

[NFC] Remove SupportedPlatforms, add PlatformVersionProvider #7161

[NFC] Remove SupportedPlatforms, add PlatformVersionProvider #7161

Conversation

MaxDesiatov commented Dec 3, 2023 • edited Loading

Motivation:

Modifications:

Result:

tomerd left a comment • edited Loading

Choose a reason for hiding this comment

MaxDesiatov commented Dec 5, 2023

MaxDesiatov commented Dec 5, 2023

MaxDesiatov commented Dec 5, 2023

MaxDesiatov commented Dec 7, 2023

MaxDesiatov commented Dec 7, 2023

tomerd commented Dec 8, 2023

neonichu Dec 12, 2023

Choose a reason for hiding this comment

MaxDesiatov Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

MaxDesiatov commented Dec 13, 2023

[NFC] Remove `SupportedPlatforms`, add `PlatformVersionProvider` #7161

[NFC] Remove `SupportedPlatforms`, add `PlatformVersionProvider` #7161

MaxDesiatov commented Dec 3, 2023 •

edited

Loading

tomerd left a comment •

edited

Loading

MaxDesiatov Dec 13, 2023 •

edited

Loading