Use swift.org API for getting releases and some snapshot toolchains #153

cmcgee1024 · 2024-08-08T18:15:54Z

See issue #149

Using the new swift.org API is more accurate than the GitHub API, also it isn't subject to the same rate limits. Use the swift.org API instead of the GitHub API for fetching metadata about the released swift toolchains, and the newer swift branches for the snapshots.

However, there is currently no metadata for snapshots on older branches than 6.0 or main. In this case use the GitHub API to get the metadata, but only for swiftly install and not the swiftly list-available commands, since the latter is very expensive when using the GitHub API.

…napshot toolchains

…itectures

…efore 6.0

Bump versions in update tests to versions that amazon linux 2 has tagged releases

cmcgee1024 · 2024-08-09T11:17:49Z

@swift-ci test macOS

cmcgee1024 · 2024-08-09T13:35:50Z

The macOS CI passes the tests, but there's the persistent environmental problem that reports an overall failure.

Test Suite 'All tests' passed at 2024-08-09 11:22:46.405.
	 Executed 67 tests, with 0 failures (0 unexpected) in 72.886 (72.895) seconds

adam-fowler

In general looks good, I've a couple of questions and minor edit requests

adam-fowler · 2024-08-10T16:15:02Z

Sources/Swiftly/ListAvailable.swift


-        Note that listing available snapshots is currently unsupported. It will be introduced in a future release.
+        Note that listing available snapshots before 6.0 is unsupported.


I assumed this is because the data isn't available at the moment. Could this change?

adam-fowler · 2024-08-10T16:17:29Z

Sources/Swiftly/ListAvailable.swift

            print("----------------------------")
-            for toolchain in toolchains where toolchain.isStableRelease() {
+            for toolchain in toolchains {


Good call don't need to separate these two lists

The output of swiftly list separates them still, so I think it would be a good idea to be consistent with that. Regardless of that, they were originally separated in part because snapshots are released on a (nearly?) daily basis, so they would always dominate the output here, and I'd imagine that most users are looking for available releases anyways. So I'd support continuing to list these separately.

This else only gets invoked in the absence of a selector, which defaults to all of the releases, since every snapshot build for every branch would be overwhelming and probably not very useful.

I can retain the "release" toolchains label to make it clear that these are only releases, but the where clause shouldn't be necessary here.

adam-fowler · 2024-08-10T16:19:56Z

Sources/SwiftlyCore/HTTPClient.swift

@@ -22,6 +22,100 @@ private func makeRequest(url: String) -> HTTPClientRequest {
    return request
 }



Is it worthwhile breaking out the swift.org data and parsing into a separate file?

I'm on the fence about this.

It feels like the swift.org metadata is a foundational use of the swiftly http client, so why not just put the code there? On the other hand, because of the GitHub fallback there's some asymmetry now because it's in another source file. But, there's also a feeling that the fallback may go away someday, so that could be why it is separate.

As I see it there is the client and the usage of the client which are two different things

Sources/SwiftlyCore/HTTPClient.swift

adam-fowler · 2024-08-10T16:43:23Z

Sources/SwiftlyCore/HTTPClient.swift

@@ -88,21 +207,91 @@ public struct SwiftlyHTTPClient {
            return release
        }

-        return try await self.mapGitHubTags(limit: limit, filterMap: filterMap) { page in
-            try await self.getReleases(page: page)
+        guard !swiftOrgFiltered.isEmpty else {


Is this really necessary?

Agreed, this is not needed. I'll remove it.

patrickfreed · 2024-08-12T04:59:38Z

Sources/Swiftly/ListAvailable.swift

            print("----------------------------")
-            for toolchain in toolchains where toolchain.isStableRelease() {
+            for toolchain in toolchains {


The output of swiftly list separates them still, so I think it would be a good idea to be consistent with that. Regardless of that, they were originally separated in part because snapshots are released on a (nearly?) daily basis, so they would always dominate the output here, and I'd imagine that most users are looking for available releases anyways. So I'd support continuing to list these separately.

Sources/SwiftlyCore/HTTPClient.swift

patrickfreed · 2024-08-12T05:12:49Z

Sources/SwiftlyCore/HTTPClient.swift

+        return if let limit = limit {
+            Array(finalSnapshotToolchains[0..<limit])
+        } else {
+            finalSnapshotToolchains


We have several variables for the snapshots, but I think we only really need swiftOrgFiltered. Can we drop finalSnapshotToolchains and snapshotToolchains?

I'm looking into making this simpler and easier to follow.

Sources/Swiftly/ListAvailable.swift

cmcgee1024 · 2024-08-20T16:07:10Z

@swift-ci test macOS

cmcgee1024 · 2024-08-20T16:27:38Z

The macOS CI passed the xctests despite reporting an overall failure:

Test Suite 'All tests' passed at 2024-08-20 16:11:50.665.
	 Executed 67 tests, with 0 failures (0 unexpected) in 73.780 (73.789) seconds

cmcgee1024 · 2024-08-22T11:59:26Z

@patrickfreed @adam-fowler how do these latest changes look? Any remaining issues?

patrickfreed

Overall looks good, super excited to see swiftly using official APIs now. I have a bunch of smaller, mostly style suggestions. Feel free to merge after addressing without waiting for another round of feedback from me.

patrickfreed · 2024-08-23T02:45:10Z

Documentation/SwiftlyDocs.docc/swiftly-cli-reference.md

+*A filter to use when listing toolchains.*
+
+
+The toolchain selector determines which toolchains to list. If no selector is provided, all available toolchains will be listed:


should note here that only release toolchains will be listed.

patrickfreed · 2024-08-23T03:50:49Z

Sources/Swiftly/ListAvailable.swift

-            .map(ToolchainVersion.stable)
-            .filter { selector?.matches(toolchain: $0) ?? true }
+            tc = try await SwiftlyCore.httpClient.getSnapshotToolchains(platform: config.platform, branch: branch).map { ToolchainVersion.snapshot($0) }
+        default:


Rather than using default here, can we explicitly check for .release? It reads a bit clearer that way.

Thanks, I've changed this to add explicit stable/latest case, and a default to catch no selector provided.

patrickfreed · 2024-08-23T04:03:16Z

Sources/SwiftlyCore/HTTPClient.swift

+        let arch = if a == nil {
+#if arch(x86_64)
+            "x86_64"
+#elseif arch(arm64)
+            "aarch64"
+#else
+            #error("Unsupported processor architecture")
+#endif


we do this in a few places, can we extract this to a method/computed variable on Platform? I think it would also be good to define an enum for it so we don't have to rely on magic strings here and elsewhere.

This pattern is isolated to two places at the moment, mostly isolated to path/filename calculation and internal swift org metadata. It might be possible to isolate all of it in the HTTPClient, or an HTTPClient+SwiftOrg.

There are some problems with pulling this out into a global enum at the Platform level. In macOS, the arch isn't used at all because of the universal binaries/packages. Also, there is some disagreement between the naming of the 64-bit arm architecture. In some places. it's "arm64" while others is "aarch64." It doesn't feel like an enum could be universally defined across OSes and cases without being a leaky abstraction.

I feel that in Swift it's quite common to put these kinds of macros (os/arch) at the place where they are needed as long as it isn't excessive and it can maintain locality of change when/if the logic changes. For now, it's only these two places: HTTPClient, and Install.

patrickfreed · 2024-08-23T04:03:53Z

Tests/SwiftlyTests/HTTPClientTests.swift

+            PlatformDefinition.macOS,
+            PlatformDefinition.ubuntu2204,
+            PlatformDefinition.ubuntu2004,
+            // PlatformDefinition.ubuntu1804,


why commented out?

Ubuntu 18.04 doesn't have toolchains available for the releases that we are testing here. I'll update the comment to explain.

patrickfreed · 2024-08-23T04:08:32Z

Sources/SwiftlyCore/HTTPClient.swift

+        let branchLabel = switch branch {
+        case .main:
+            "main"
+        case let .release(major, minor):
+            "\(major).\(minor)"
+        }


branch.name gives us this.

patrickfreed · 2024-08-23T04:15:52Z

Sources/SwiftlyCore/HTTPClient.swift

+        let swiftOrgReleases: [SwiftOrgRelease] = try await self.getFromJSON(url: url, type: [SwiftOrgRelease].self)
+
+        var swiftOrgFiltered: [ToolchainVersion.StableRelease] = swiftOrgReleases.compactMap { swiftOrgRelease in
+            if platform.name != "xcode" {


"xcode" -> PlatformDefinition.xcode.name

patrickfreed · 2024-08-23T04:16:38Z

Sources/SwiftlyCore/HTTPClient.swift

+                }
+            }
+
+            guard let version = try? ToolchainVersion(parsing: swiftOrgRelease.stableName),


if parsing fails, I think we'd want to let the user know about it in some way, either just by throwing here (which I think is fine, since we should never fail parsing swift.org results) or logging.

patrickfreed · 2024-08-23T04:17:54Z

Sources/SwiftlyCore/HTTPClient.swift

+            "\(major).\(minor)"
+        }
+
+        let platformName = if platform.name == "xcode" {


"xcode" -> PlatformDefinition.xcode.name, also below.

patrickfreed · 2024-08-23T04:25:48Z

Sources/SwiftlyCore/HTTPClient.swift

+
+    /// platformName is a mapping from the 'name' field of the swift.org platform object
+    /// to swiftly's PlatformDefinition name.
+    var platformName: String {


To better align with this var's usage/documentation, it might be better to just return the matching PlatformDefinition or nil if it isn't supported. That way we can rely on the PlatformDefinition constants as the single source of supported platform names, and we also don't need to define constants for platforms we don't support. If we go that direction, renaming this something like asPlatformDefinition or something might make sense.

If we don't change this in that way, then I think at least renaming it to canonicalPlatformName or swiftlyPlatformName communicates the difference between the other name field a bit better. We'd also want to use the PlatformDefinition name constants here as well.

Thanks, this is a really nice pattern here. We can return the constants defined in PlatformDefinition for platforms that swiftly supports, and manually constructed PlatformDefinitions for the ones that it doesn't, which can provide enough metadata to represent what the swift.org API is producing. For yet unknown (to swiftly) swift.org platforms, then it's just nil until this code adds a representation for it.

patrickfreed · 2024-08-23T04:30:45Z

Sources/SwiftlyCore/HTTPClient.swift

+        }
+
+        let platformName = if platform.name == "xcode" {
+            "macos"


the inconsistency between these swift.org APIs and swift.org's download URLs here is a bit odd. Is there any chance the API would be updated to use "xcode" like the downloads do so we can avoid needing to adjust for it here?

This question also applies for the non-macOS platform names. It'd be nice if e.g. ubuntu1804 were used rather than the pretty name.

@shahmishal here is some feedback regarding the swift.org API. There are some inconsistencies between the new API and the toolchain download URL's / filenames. We're working around these for now in swiftly, but perhaps this can be made a little more ergonomic for other consumers of the API.

Use PlatformDefinition constants for cases of swiftly supported platforms Construct PlatformDefinitions for known swift.org platforms that are not supported by swiftly Use branch.name for snapshot url construction Update command-line reference documentation of list-available for the case of no selector

cmcgee1024 · 2024-08-23T12:51:26Z

@swift-ci test macOS

cmcgee1024 added 7 commits August 8, 2024 14:14

DRAFT: Use swift.org API instead of GitHub for getting releases and s…

a834083

…napshot toolchains

Test the swift.org api against all combinations of platforms and arch…

a4a1cee

…itectures

Use GitHub API as a fallback for downloading release snapshots from b…

e8c0553

…efore 6.0

Restore the cached github api mocked responses for the tests

b6df2dc

Bump versions in update tests to versions that amazon linux 2 has tagged releases

Restore the token option for swiftly install for older snapshots

b3d8012

Adjust the snapshot limitation in list-available subcommand

6b9fad4

Fix soundness

8eb66fd

cmcgee1024 marked this pull request as ready for review August 9, 2024 11:26

cmcgee1024 requested review from adam-fowler and patrickfreed August 9, 2024 11:26

cmcgee1024 changed the title ~~DRAFT: Use swift.org API for getting releases and snapshot toolchains~~ Use swift.org API for getting releases and some snapshot toolchains Aug 9, 2024

adam-fowler requested changes Aug 10, 2024

View reviewed changes

patrickfreed reviewed Aug 12, 2024

View reviewed changes

apply review feedback

c95e0e2

adam-fowler approved these changes Aug 22, 2024

View reviewed changes

patrickfreed approved these changes Aug 23, 2024

View reviewed changes

patrickfreed mentioned this pull request Aug 23, 2024

Drop usage of GitHub API #157

Closed

cmcgee1024 merged commit b0dc1b3 into swiftlang:main Aug 23, 2024
5 of 6 checks passed


		Note that listing available snapshots is currently unsupported. It will be introduced in a future release.
		Note that listing available snapshots before 6.0 is unsupported.

		@@ -22,6 +22,100 @@ private func makeRequest(url: String) -> HTTPClientRequest {
		return request
		}

		A filter to use when listing toolchains.


		The toolchain selector determines which toolchains to list. If no selector is provided, all available toolchains will be listed:

Use swift.org API for getting releases and some snapshot toolchains #153

Use swift.org API for getting releases and some snapshot toolchains #153

Conversation

cmcgee1024 commented Aug 8, 2024 • edited Loading

cmcgee1024 commented Aug 9, 2024

cmcgee1024 commented Aug 9, 2024

adam-fowler left a comment

Choose a reason for hiding this comment

adam-fowler Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmcgee1024 commented Aug 20, 2024

cmcgee1024 commented Aug 20, 2024

cmcgee1024 commented Aug 22, 2024

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmcgee1024 commented Aug 23, 2024

cmcgee1024 commented Aug 8, 2024 •

edited

Loading

adam-fowler Aug 10, 2024 •

edited

Loading