-
Notifications
You must be signed in to change notification settings - Fork 29
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
macOS support for swiftly #121
Conversation
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.
In general looks good. I assume adding $HOME/.local/bin
to the PATH is done in the install script. I can't remember offhand. @patrickfreed maybe you can answer that.
We need to get the tests working though.
@@ -566,19 +585,27 @@ if [[ -f "$HOME_DIR/config.json" ]]; then | |||
detected_existing_installation="true" | |||
if [[ "$overwrite_existing_intallation" == "true" ]]; then | |||
echo "Overwriting existing swiftly installation at $(replace_home_path $HOME_DIR)" |
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.
$HOME_DIR still seems to be pointing to $HOME/.local/share/swiftly and not $HOME/Library/Application Support/swiftly. See line 533
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.
Thanks, I'll update those locations too. This is all abstracted nicely in the Swift code through the Platform protocol, so hopefully we can start using that soon instead of this duplication.
I don't want to push people into a position where they need Xcode installed to run with Swift (in the future I do hope we can install the swift toolchain without requiring Xcode). But I'm wondering, if Xcode is installed or at least |
README.md
Outdated
@@ -41,7 +41,7 @@ Target: x86_64-unknown-linux-gnu | |||
- 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. | |||
|
|||
Right now, swiftly is in the very early stages of development and is only supported on Linux, but the long term plan is to also support macOS. For more detailed information about swiftly's intended features and implementation, check out the [design document](DESIGN.md). | |||
Right now, swiftly is in early stages of development and is only supported on Linux and macOS. For more detailed information about swiftly's intended features and implementation, check out the [design document](DESIGN.md). |
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.
Right now, swiftly is in early stages of development and is only supported on Linux and macOS. For more detailed information about swiftly's intended features and implementation, check out the [design document](DESIGN.md). | |
Right now, swiftly is in early stages of development and is supported on Linux and macOS. For more detailed information about swiftly's intended features and implementation, check out the [design document](DESIGN.md). |
I think we can drop the "only" now. We're missing Windows, but certainly cover a lot more of the userbase now.
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.
Thanks, I've fixed the wording here.
Sources/SwiftlyCore/Platform.swift
Outdated
@@ -88,7 +88,12 @@ extension Platform { | |||
|
|||
/// The "toolchains" subdirectory of swiftly's home directory. Contains the Swift toolchains managed by swiftly. | |||
public var swiftlyToolchainsDir: URL { | |||
#if !os(macOS) |
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.
Can we invert this?
#if os(macOS)
#elseif os(Linux)
#else
#error("Unsupported platform")
#endif
I find that generally cognitively easier to parse, but also assume we'll be adding Windows at some point.
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.
This should be implemented at the individual Platform
level actually, so we shouldn't need any #if
directives at all. Let's just move this default implementation to Linux
and define the other one in MacOS
.
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.
This has been replaced with a Platform abstraction for both the toolchain and bin directories.
Yeah, by default the install script will add a line to source This is the contents of my export SWIFTLY_HOME_DIR="$HOME/.local/share/swiftly"
export SWIFTLY_BIN_DIR="$HOME/.local/bin"
if [[ ":$PATH:" != *":$SWIFTLY_BIN_DIR:"* ]]; then
export PATH="$SWIFTLY_BIN_DIR:$PATH"
fi |
DESIGN.md
Outdated
| | ||
-- config.json | ||
| | ||
– env | ||
``` | ||
|
||
Instead of downloading tarballs containing the toolchains and storing them directly in `~/.swiftly/toolchains`, we instead install Swift toolchains to `~/Library/Developer/Toolchains` via the `.pkg` files provided for download at swift.org. (Side note: we’ll need to request that other versions than the latest be made available). To select a toolchain for use, we update the symlink at `~/.swiftly/active-toolchain` to point to the desired toolchain in `~/Library/Developer/Toolchains`. In the env file, we’ll contain a line that looks like `export PATH=”$HOME/.swiftly/active-toolchain/usr/bin:$PATH`, so the version of swift being used will automatically always be from the active toolchain. `config.json` will contain version information about the selected toolchain as well as its actual location on disk. | ||
Instead of downloading tarballs containing the toolchains and storing them directly in `~/.local/share/swiftly/toolchains`, we instead install Swift toolchains to `~/Library/Developer/Toolchains` via the `.pkg` files provided for download at swift.org. (Side note: we’ll need to request that other versions than the latest be made available). To select a toolchain for use, we update the symlinks at `~/.local/bin` to point to the desired toolchain in `~/Library/Developer/Toolchains`. In the env file, we’ll contain a line that looks like `export PATH=”$HOME/.local/bin:$PATH`, so the version of swift being used will automatically always be from the active toolchain. `config.json` will contain version information about the selected toolchain as well as its actual location on disk. |
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.
This side note is interesting. Are we working on making other toolchains available already? I would opt for removing this side note because if we make them available this side note is stale
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 think this side is stale. I just tried a few older releases and the downloads are available, e.g.
https://download.swift.org/swift-5.9.2-release/xcode/swift-5.9.2-RELEASE/swift-5.9.2-RELEASE-osx.pkg
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've installed some very old releases of the toolchain, such as around 3.x, in my testing with macOS. I'll remove some of these notes as the picture is different now.
Sources/SwiftlyCore/Platform.swift
Outdated
@@ -88,7 +88,12 @@ extension Platform { | |||
|
|||
/// The "toolchains" subdirectory of swiftly's home directory. Contains the Swift toolchains managed by swiftly. | |||
public var swiftlyToolchainsDir: URL { | |||
#if !os(macOS) |
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.
This should be implemented at the individual Platform
level actually, so we shouldn't need any #if
directives at all. Let's just move this default implementation to Linux
and define the other one in MacOS
.
Sources/Swiftly/Config.swift
Outdated
#if !os(macOS) | ||
throw Error(message: msg) | ||
#else | ||
let pd = PlatformDefinition.init(name: "xcode", nameFull: "osx", namePretty: "macOS", architecture: nil) | ||
return Config.init(inUse: nil, installedToolchains: [], platform: pd) | ||
#endif |
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'd rather us handle this elsewhere or do it uniformly for Linux and macOS. What's the motivation for having this here?
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 motivation was to have Swiftly be capable of being run without the installer script in dev, or even through other types of installers (e.g. homebrew). The test strategy for macOS is manual at the moment, and there's no release for it yet for the install script to download. There needs to be a way to get a swiftly binary onto a Mac and run through tests.
MacOS doesn't require all of the auto-detection of platform like Linux. This all becomes more uniform in the future with Swiftly self-install through the init subcommand.
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 existing way to do this is to set the following environment variables:
- SWIFTLY_PLATFORM_NAME
- SWIFTLY_PLATFORM_NAME_FULL
- SWIFTLY_PLATFORM_NAME_PRETTY
You can see example values for these files in the various dockerfiles that were previously used for CI:
swiftly/docker/docker-compose.2204.yaml
Lines 14 to 16 in ab38db0
- SWIFTLY_PLATFORM_NAME=ubuntu2204 | |
- SWIFTLY_PLATFORM_NAME_FULL=ubuntu22.04 | |
- SWIFTLY_PLATFORM_NAME_PRETTY="Ubuntu 22.04" |
I think it'd be best if we continued to rely on these environment variables for this, since changing this code might affect non-test usage of swiftly.
@@ -1,10 +1,12 @@ | |||
// swift-tools-version:5.7 | |||
// swift-tools-version:5.10 |
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.
If we're bumping to 5.10 we should probably switch on strict concurrency checking
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.
Thanks, I'm definitely interested in enabling better checks. How does one turn on the strict concurrency checking?
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.
In your target definition in Package.swift add
swiftSettings: [.enableExperimentalFeature("StrictConcurrency=complete")]
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.
Thanks, I tried enabling this and there were a non-trivial number of warnings (errors in Swift 6). I've raised an issue #124 to address the concurrency problems and enable the check.
} | ||
|
||
// Ensure swiftly doesn't overwrite any existing executables without getting confirmation first. | ||
let swiftlyBinDirContents = try FileManager.default.contentsOfDirectory(atPath: self.swiftlyBinDir.path) |
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 wonder if it's worth us migration to NIOFileSystem, since we already have NIO as a dependency. Aside from the nicer API, it would be one less thing to use when depending on new Foundation which should make for a smaller binary
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 can raise this as a separate issue. I believe that there are Foundation dependencies all over the code. Pulling all of that out would be a significant sweep.
I'm not familiar with NIOFileSystem. Something that I really like about FileManager API is that it appears to be very mockable so that we could refactor the tests to operate on a full mock of the filesystem. Is that capability available with NIOFS?
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.
To be clear I'm not suggesting we pull out all of Foundation, but minimising it to just FoundationEssentials
etc could help reduce the size (especially if we don't need ICU).
Which specific parts would you use for mocking (not getting into a discussion about that word 🤣 )? NIOFileSystem has a similar API in that you have FileSystem.shared
you perform the operations on, but the API is much more modern and built with Concurrency in mind (may also help solve some of the issues in #124 )
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.
In general, I agree that NIOFileSystem is more appropriate here since it uses async code to do the work. However, I would defer that from this PR since it is something we can do separately. WDYT @0xTim ?
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.
Yes let's defer this to another PR. I've added an issue #125
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.
Yeah absolutely shouldn't be a blocker for this PR
If users are using Xcode then Swiftly can help them to get toolchains installed and Xcode can pick them up from there, along with the xcode-select mechanisms. Otherwise, Swiftly will install the pkg files available from swift.org and provide a toolchain selection mechanism via use, the config.json, and either the symlinks/proxies (in the future). There's no need for Xcode if the user doesn't want to use it. |
dfb441d
to
c612354
Compare
Thanks everyone. @adam-fowler @patrickfreed is there anything left that's blocking this from being merged? |
Do you think you could get the tests running? Although that might be quite a large undertaking. @shahmishal do you think we could get macOS CI up and running? |
The plan for macOS right now is just ad-hoc testing, since much of the core swiftly logic remains the same here and is covered by the install and xctest cases. I believe that those are all still functional with this patch, but it will be good to get the CI hooked back up so that you all don't have to take my word for it. I would like to revisit testing in the near future to see if we can make the xctests work in full mocked isolation, rework the installation tests to use something Swift-ish instead of directly depending on docker/docker-compose. |
@swift-server-bot test this please |
@shahmishal I don't seem to be able to trigger CI |
@cmcgee1024 so I tried to use this and kept getting a |
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 only remaining asks are reducing the frequency of the PATH hash warning message and removing the macOS specific configuration default when the file is missing.
FYI you can bypass the API rate limit error by passing a permissionless GitHub token to your swiftly command invocation. |
Is this happening when you try to run the tests from macOS, or Linux? For macOS, I don't think that they're going to work in their current form. In either case, I'm not sure how these changes in this PR cause NIOTooManyBytes, so I'd be curious if you get the same thing with the latest in main. The rate limits can be overcome using a GitHub personal token from your account and no permissions at all since the releases API of a public project is public. You can give it to swiftly using an environment variable like this:
I hope that we can mock these much better in the future so that they don't hit rate limits, susceptible for external conditions, filesystem state. There'll be a few integration and end-to-end tests to check for smoke, but I expect these to much fewer in number, so easier to retry when they fail. |
The type of testing that I'm doing on macOS at the moment is ad-hoc. It involves copying over the binary onto a fresh system, and running smoke tests. This auto-configuration is really helpful for this until we have the init subcommand in an upcoming patch. Can we keep this in place for the sake of testing so that I don't have to create a custom version of the installer shell script? |
I've reproduced the NIOTooManyBytesError on Ubuntu 22.04 Linux with the swift docker image, and it's also happening on main. I've opened a new issue #128 to track this down. |
Do you mean, you believe the XCTests are working for macOS? Because currently most of them are failing on macOS. |
@adam-fowler I've just confirmed that the xctests are working in Linux on this branch as well as main as long as the GitHub token is provided. It appears that without a token the GH API's sometimes give inaccurate content lengths under load. I expect that the tests do not work on macOS, and so the plan is ad-hoc testing on there. |
@swift-server-bot test this please |
@swift-ci test macOS |
c612354
to
0f3f894
Compare
@swift-server-bot test this please |
1 similar comment
@swift-server-bot test this please |
@swift-ci test macOS |
1 similar comment
@swift-ci test macOS |
@swift-server-bot test this please |
Hi @cmcgee1024 |
@adam-fowler it appears that the signature file was empty or corrupted on three out of the four linux validation checks. They're all running the same tests. I wonder if this is some kind of transient failure at the time the tests were run. |
@swift-server-bot test this please |
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.
We'll also want to make sure the swiftly installation tests work on macOS before merging.
@@ -461,4 +535,67 @@ public struct MockToolchainDownloader: HTTPRequestExecutor { | |||
|
|||
return try Data(contentsOf: archive) | |||
} | |||
|
|||
#elseif os(macOS) |
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 first two halves of this branch are identical, can we combine them and only have the OS-specific parts inside the branches?
override class func setUp() { | ||
if Self.requestExecutor == nil { | ||
Self.requestExecutor = ProxyHTTPRequestExecutorImpl() | ||
Install.httpClient = SwiftlyHTTPClient(executor: Self.requestExecutor) | ||
SelfUpdate.httpClient = SwiftlyHTTPClient(executor: Self.requestExecutor) | ||
} | ||
} |
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 think we'll need to unconditionally reset all the httpClients to use the proxy executor, since many tests will replace them with mocked http clients. To make this easier, it would probably be preferable to have one single global singleton HTTP client defined in SwiftlyCore or something.
@swift-server-bot add to allowlist |
@swift-server-bot test this please |
Provide more details about gpg verification failues
@swift-ci test macOS |
@swift-ci test macOS |
@patrickfreed I've refactored the way that the http client is shared and mocked and all of the tests are passing on both Linux and macOS. I hope that this is ready to squash and merge at this point. Otherwise, is there anything that still needs to be addressed? |
I'm merging this PR since the tests are now passing and the discussions points seem to have converged. Please raise issues for anything that remains. Thanks everyone! |
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.
Sorry for the delay, the only outstanding comment I have is this about deduplicating some code, but otherwise looks good to me. Feel free to defer that to a follow on if you'd like. Excited to see swiftly running on macOS!
Oh I also think it would be good to verify the installation tests still work as well |
@swift-server-bot test install please |
It works much like it does already for Linux with some notable differences:
Create a MacOS struct that implements the existing Platform protocol. Make a platform-specific target for this module. Bump the required swift toolchain version to resolve compiler errors and set the minimum macOS version to 13.
Update the README.md with some macOS details and fix some of the details that were outdated for Linux.
Add some helpful notes regarding the need to rehash the zsh on macOS since even when the swiftly bin directory has higher precedence in the PATH it sometimes gets snagged on the /usr/bin/swift, which doesn't detect the user installed toolchains and sometimes tries to get the user to install Xcode.
Make the shell script swiftly installer capable of operating in a standard macOS environment. First, detect that the environment is macOS, and then adjust the getopts for macOS's more limited implementation with the short opts. Also, remove any of the Linux specific steps to detect the distribution, check for gpg, and attempt to install Linux system packages.