Add new Linux platforms: Ubuntu 24.04, Debian 12, Fedora 39 #173
Conversation
Add support for these new platforms to swiftly, the autodetection, the tests, and docker support. Remove the swiftly installer portion of the code.
|
@swift-ci test macOS |
|
@swift-ci test macOS |
| case "5": | ||
| return PlatformDefinition.amazonlinux2 | ||
| default: | ||
| guard let choiceNum = Int(choice) else { |
There was a problem hiding this comment.
I think right now if you were to run this and enter 0 for cancel, it would explode, it would go on to run
return self.linuxPlatforms[choiceNum - 1] and you'd get an index exception
There was a problem hiding this comment.
I think that this guard will guard against non-integer input, such as "foo", or "bar". The guard below should check for 0, or something larger than the linuxPlatforms count and cancel the installation. The final array subscript at the return statement should be safely in range.
There was a problem hiding this comment.
oh I see, the guard clause tripped me up, Makes sense
| } | ||
| return self.manualSelectPlatform(platformPretty) | ||
| } | ||
| let releaseInfo = try String(contentsOfFile: releaseFile, encoding: .utf8) |
There was a problem hiding this comment.
should we still wrap this in an equivalent try/catch which calls to self.manualSelectPlatform if the info can't be read correctly?
There was a problem hiding this comment.
I think that we want to heavily emphasize the platform auto-detection because it will make the user's life a ton easier. If there's some kind of problem reading the /etc/os-release or /usr/lib/os-release to the point where we can't even read the bytes into a String with the expected UTF-8 encoding the user should probably be made aware of this corruption.
From the linux manual it says that "[a]ll strings should be in UTF-8 encoding, and non-printable characters should not be used."
| for info in releaseInfo.split(separator: "\n").map(String.init) { | ||
| if info.hasPrefix("ID=") { | ||
| id = String(info.dropFirst("ID=".count)).replacingOccurrences(of: "\"", with: "") | ||
| } else if info.hasPrefix("ID_LIKE=") { | ||
| idlike = String(info.dropFirst("ID_LIKE=".count)).replacingOccurrences(of: "\"", with: "") | ||
| } else if info.hasPrefix("VERSION_ID=") { | ||
| versionID = String(info.dropFirst("VERSION_ID=".count)).replacingOccurrences(of: "\"", with: "") | ||
| } else if info.hasPrefix("UBUNTU_CODENAME=") { | ||
| ubuntuCodeName = String(info.dropFirst("UBUNTU_CODENAME=".count)).replacingOccurrences(of: "\"", with: "") |
There was a problem hiding this comment.
Do we still need to account for stings which start with "UBUNTU_CODENAME" here?
There was a problem hiding this comment.
We're reading this file line-by-line and we don't have any need for the ubuntu codename anymore. I think it's safe to just skip ubuntu codename lines.
Sources/LinuxPlatform/Linux.swift
Outdated
| } | ||
|
|
||
| return PlatformDefinition(name: "ubi9", nameFull: "ubi9", namePretty: "RHEL 9") | ||
| return PlatformDefinition.rhel9 | ||
| } else if let pd = [PlatformDefinition.ubuntu1804, .ubuntu2004, .ubuntu2204, .ubuntu2310, .ubuntu2404, .debian12, .fedora39].first(where: { $0.name == id + versionID }) { |
There was a problem hiding this comment.
If we're gonna maintain a static data structure here, might as well make it a dictionary so we get better performance on look ups? Any thoughts? It's not really a big deal but since we're going to maintain the list of linux versions here might be worth it
There was a problem hiding this comment.
In terms of performance I'm not too worried about it since this is likely run very infrequently, just at install time, and only once. This is a one-off list of the linux platforms where we can match directly with the name and version id.
In the future, I hope that we add an API to swift.org so that this can become entirely dumb, making it much easier to just add new linux platforms by adding new entries to the swift.org API, and not have to update the swiftly code each time.
| @@ -125,6 +137,26 @@ public struct Linux: Platform { | |||
| "tzdata", | |||
| "zlib1g-dev", | |||
| ] | |||
| case "ubuntu2404": | |||
There was a problem hiding this comment.
I think we're missing 2310 from this list
There was a problem hiding this comment.
I couldn't find a list of packages for ubuntu 23.10 from the Dockerfiles. If someone knows where the docker file is, I can add it here.
This is where I usually look for them: https://github.com/swiftlang/swift-docker/tree/main/6.0/ubuntu
There was a problem hiding this comment.
I manually pieced together the list by looking at the differences between ubuntu 22.04, and 24.04. Tested it out in docker and it looks to be working fine. There's only the swift 5.10.1 release available for that one though, so I wonder how useful and used this version of ubuntu will be.
There was a problem hiding this comment.
23.10 only exists under https://github.com/swiftlang/swift-docker/blob/main/5.10/ubuntu/23.10/Dockerfile; but 23.10 reached end of life on July 2024.
There was a problem hiding this comment.
I have removed 23.10 from the list because we can't bring up infrastructure for a platform that is EOL.
| // THEN: we get at least 1 release | ||
| XCTAssertTrue(1 <= releases.count) | ||
|
|
||
| if newPlatforms.contains(platform) { continue } // Newer distros don't have main snapshots yet |
There was a problem hiding this comment.
How will we eventually know to remove this for proper testing? Seems like the kind of thing I'd forget 😄
There was a problem hiding this comment.
Integration tests that are in the middle of the test pyramid like this are tricky, but necessary.
The trouble with mocking the responses is that we won't know if the API might change in some way that breaks swiftly. I think that we want to catch obvious integration problems between the swift.org API and what we can potentially request from it. If that means that we don't fully cover all possibilities with all supported platforms I think that's ok. This was a good test when I started working on the patch to verify that the platform definition strings were correct from the standpoint of the swift.org API.
| return v | ||
| } | ||
|
|
||
| let name = try? getEnv("SWIFTLY_PLATFORM_NAME") |
There was a problem hiding this comment.
were these variables just unused?
There was a problem hiding this comment.
These variables have been slowly been superseded by platform auto detection, first in the production part of swiftly on macOS, then gradually the tests. This is removing the last remnant of them because I didn't want to miss test coverage of the auto-detection on the different CI platforms since I had modified some of that code here.
@shahmishal can we get the CI to add the new platforms? I think that I added all of the docker files, and docker compose, but the CI didn't pick up Debian 12, Fedora 39, or the two new Ubuntu (24.04, 23.10). I still see only the usual RHEL9, Ubuntu 22.04, etc.
There was a problem hiding this comment.
Switching over to GH actions has yielded these new platforms for testing. See the "checks" section of this PR.
docker/docker-compose.2404.yaml
Outdated
|
|
||
| services: | ||
|
|
||
| test-setup: |
There was a problem hiding this comment.
are there Dockerfiles for this?
There was a problem hiding this comment.
I believe that the docker files for these are covered by the images from docker hub, like swift:5.10-noble, and also the test.dockerfile and install-test.dockerfile which are ubuntu-based with some variables for the ubuntu version. These Dockerfiles should be invoked by the CI, but I don't know the magic hooks to make the CI try these new platforms now that we have the docker and docker compose files. Maybe @shahmishal can hook up CI for them?
There was a problem hiding this comment.
In this latest version of the PR none of these are needed anymore since we switched to GH actions. The docker setup is managed by the official docker containers by the reusable workflow in swiftlang/github-workflow.
| @@ -1,716 +0,0 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
A bit behind I think, why did we remove all of these scripts + their tests?
There was a problem hiding this comment.
This bash script was the original curl | bash installation system for swiftly in the previous version. We've slowly migrated it to the new swiftly init mechanism for a few reasons, such as much better options handling, and a generally safer mechanism.
I'm removing it here because it's vestigial at this point, and confuses new contributors who still see this as the installer. Plus, it would need to have support for these new platforms.
There was a problem hiding this comment.
But it was also responsible for pulling the latest release binary of swiftly right? So is the implication that going forward we should expect users to build from source? Or should we host release binaries on the repo? Just wondering if the readme/release process needs to be changed here
There was a problem hiding this comment.
There's a few possibilities:
- Direct download of the binary package from swift.org (tarball and macOS pkg) from the downloads page (see [install] Add Swiftly to the install page swift-org-website#830 for the current draft)
- Reliable API or download URL where the package can be downloaded from swift.org
- System package managers (apt, yum, brew)
There was a problem hiding this comment.
This should be done in a separate PR to discuss is rather than bundling it in as part of this PR
cmcgee1024
changed the title
|
@swift-ci test macOS |
|
The macOS tests are all passing: |
docker/docker-compose.debian12.yaml
Outdated
|
|
||
| test-setup: | ||
| image: swiftly:debian12-test | ||
| build: |
There was a problem hiding this comment.
should this specify the newly added Dockerfile?
There was a problem hiding this comment.
Since we've moved to the new GitHub Actions infrastructure I've removed the old docker compose files and Dockerfiles from here.
|
@swift-ci test macOS |
1 similar comment
|
@swift-ci test macOS |
cmcgee1024
changed the title
cmcgee1024
changed the title
|
@swift-ci test macOS |
|
The macOS tests are all passing: |
…ot available Add required system packages that are missing on debian 12 and fedora 39
|
@swift-ci test macOS |
|
We've moved to the new GitHub Actions infrastructure, so there's been a few changes here.
|
|
The macOS tests are passing: |
| "revision" : "3b13e439a341bbbfe0f710c7d1be37221745ef1a", | ||
| "version" : "0.6.1" | ||
| "revision" : "5b130e04cc939373c4713b91704b0c47ceb36170", | ||
| "version" : "0.7.1" |
There was a problem hiding this comment.
There was a compile error in the TSC code only when compiling in these new Linux platforms, likely due to a header change in libc. The new version fixes this problem.
| @@ -44,7 +44,6 @@ Target: x86_64-unknown-linux-gnu | |||
| ## Platform support | |||
|
|
|||
| - Linux-based platforms listed on https://swift.org/download | |||
| - CentOS 7 will not be supported due to some dependencies of swiftly not supporting it, however. | |||
There was a problem hiding this comment.
Is it worth noting here that 23.10 isn't supported for 5.10 releases? technically that is listed on swift.org/download? Just thinking out loud, it doesn't matter much
Add support for these new platforms to swiftly, the autodetection, the tests, and docker support.
Remove the swiftly installer portion of the code.