-
Notifications
You must be signed in to change notification settings - Fork 2
Add Linux support to the Swift package #42
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
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.
This is really great work, thanks for taking care of it!! Nice to have Linux support this early, because we'll never have to do a big project to catch it up
Approving pending a few small items
.buildkite/pipeline.yml
Outdated
@@ -47,6 +47,26 @@ steps: | |||
plugins: *common_plugins | |||
agents: | |||
queue: mac | |||
- label: ":swift: :linux: Build and Test" | |||
command: | | |||
echo "--- :linux: Install dependencies" |
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.
Could we extract all of this into a script and make it so that make test-swift-linux
runs it in the context of a Docker container automatically?
Then this pipeline item can just be make test-swift-linux
and not need the Docker plugin, etc?
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.
👍 Done in 21fa174.
.buildkite/pipeline.yml
Outdated
echo "--- :rust: Installing Rust" | ||
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -v -y | ||
. "$$HOME/.cargo/env" | ||
rustup toolchain install nightly |
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 don't think we need nightly
for Linux compatibility – it should just be needed for third-tier platforms like tvOS, watchOS, etc?
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.
No, we don't need it. Removed in 21fa174.
⬆️ this commit hasn't been built because there is an open incident in buildkite. |
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 looks like it works in CI, but I ran it locally and got:
test-swift-linux
[...]
swift test -Xlinker -Ltarget/swift-bindings/libwordpressFFI-linux -Xlinker -lwp_api
error: invalid tool type in 'tools' map
error: unable to load build file
Is that happening for you as well?
@jkmassel Yes, it did. You can workaround it by running It's caused by swiftlang/swift-package-manager#6755, because the docker image uses Swift 5.9 (5.10 is not available on AWS Public ECR yet) and you are probably using 5.10 on your mac. We could exclude |
Difference between import pre-built Apple and Linux library to SPM
Currently, we use
.binaryTarget
to import the libwp_api into the Swift package. However, the.binaryTarget
is only supported on Apple platforms at the moment. On Linux, we'll need to import libwp_api using.systemLibrary
target. See the "Requiring a System Library Without pkg-config" section in SPM documentation for details. I'll explain some essential steps in detail.Pre-built libwp_api
When import a pre-built library into Swift package, SPM needs to know where the library API (for compilation) and binary (for linking) are. Apple's xcframework, which is supplied using
.binaryTarget
, contains both information. On Linux,.systemLibrary
can provide those information using pkg-config. We don't have pkg-config support in libwp_api, so we'll need to provide the Linux library in other ways.The
make swift-linux-library
command generates a directory like this:This library is imported as SPM target on Linux:
.systemLibrary(name: "libwordpressFFI", path: "target/swift-bindings/libwordpressFFI-linux/")
.However, we still need to tell SPM where the library binary is, because the modulemap file only contains library API. That's where the additional linker options (see the
make test-swift-linx
command) come in:-Xlinker -Ltarget/swift-bindings/libwordpressFFI-linux -Xlinker -lwp_api
.More on the additional linker options
There are two things to note here:
First, the SPM documentation says:
So, hopefully we don't need to pass these additional cli options soon. SE-0305 introduces an "artifactbundle" format, which I guess could be a format that
.binaryTarget
will support. We'll wait and see.Second, we could add a
link "wp_api"
line to the modulemap file and omit the-Xlinker -lwp_api
. I can see the value in that if it saves us from passing any cli options. But considering we still to pass another linker option, it feels less valuable to me.static or dynamic libwp_api
You may have noticed that I used
libwp_api.a
the static library, instead oflibwp_api.so
the dynamic library. That's because of an error when running the binary that's linked againts the dynamic library:I couldn't figure out why the library can't be found. Also, I don't see a major difference in linking static library vs dyanmic library here. We may have a holistic solution when we look into distributing this library to Linux if that happens later.
BTW, you can reproduce this error by changing
libwp_api.a
tolibwp_api.so
in theswift-linux-library
make command and runningmake test-swift
.Cross-compliation
Another thing to note is I didn't do cross complation on Linux yet, which should be pretty straightforward. I'd like to get these changes previewed first.
Test Instructions
You can run
make test-swift
command in a docker container. See the new pipeline step for how to set up a docker container for testing.BTW, I'm open to put those env setup code into a dockerfile so that we can run
make test-swift-linux
directly on macOS.