cgrindel / rules_swift_package_manager

Collection of utilities and Bazel rules to aid in the development and maintenance of Swift repositories using Bazel.
Apache License 2.0
78 stars 31 forks source link

feat: Add a mechanism to set additional attributes on generated swift libraries. #1222

Open AttilaTheFun opened 2 months ago

AttilaTheFun commented 2 months ago

In order to merge my example using grpc-swift from RSPM I need to be able to add the alwayslink = True tag to the generated build file for SwiftAtomics. The tests will crash on Ubuntu if this is unset.

If we have a tag class to customize attributes for packages in MODULE.bazel, that would allow me to fix this issue.

From this thread: https://github.com/cgrindel/rules_swift_package_manager/issues/1214

It was specifically the ManagedAtomic type in swift atomics that was failing on ubuntu.

I think the flag was not necessary on apple platforms because their linker has different default behavior.

The flag controls whether the linker should ignore object files that are unreferenced elsewhere in the program.

In Swift, you can extend types to conform to a protocol outside of their defining file / module. I believe in this case an extension was "falling off" and the program was crashing at runtime.

There was some discussion around whether this should just be the default for swift_library.

As for RSPM, I think a tag class that allows this to be configured on a per-dependency basis sounds good. If you add that, I'm happy to rebase the PR and get it working again.

https://stackoverflow.com/questions/48653517/what-does-bazel-alwayslink-true-mean

brentleyjones commented 2 months ago

Ideally we wouldn't need to customize this and we can get all the information about if we need this or not from the information that SPM provides? If SPM is able to compile this with that info, we should be able to as well.

AttilaTheFun commented 2 months ago

@brentleyjones How do you think we should determine when we need to apply the alwayslink = True flag?

These deps work as expected on linux when building with SPM but they fail at runtime with Bazel.

luispadron commented 2 months ago

Does SPM always link as well, e.g. when doing something like swift build?

AttilaTheFun commented 2 months ago

@luispadron I did not see any specific configuration for the module, but it's possible this is a default behavior in swift build

https://github.com/apple/swift-atomics/blob/main/Package.swift

A swift_binary built from this package with Bazel crashed on Ubuntu but the same executable built with SPM worked.

Keith suggested the alwayslink flag and that fixed it, which is why I added that to swift atomics in rules swift.

I traced it back to the ManagedAtomic type and an extension in another file adding a conformance to a protocol.

It's possible that Apple configures the linker differently than bazel when running swift build.

brentleyjones commented 2 months ago

This feels like we need to adjust our default behavior to mimic what SPM does.

AttilaTheFun commented 2 months ago

@brentleyjones @cgrindel this is the minimal example project I created to reproduce the issue with Bazel and show that it works with SPM:

https://github.com/AttilaTheFun/swift_atomics_test

I used a Ubuntu VM that matched the version used by RSPM / rules_swift CI.

These are the instructions I wrote for myself to install the VM and install Bazel and Swift on it too:

Installing a Multipass Ubuntu VM with Bazel and Swift

Install Ubuntu VM with Multipass

Install multipass:

brew install multipass

Check that it was successful:

multipass version

List the available VMs:

multipass find

Install a Ubuntu 20.04 (aka focal) with the default config:

multipass launch jammy

If you want a different config:

multipass launch jammy -n jammy -c 2 -m 4G -d 20G

To ssh into the running machine:

multipass shell jammy

To start the VM:

multipass start jammy

To copy files to / from the VM:

multipass transfer jammy:file.txt .

Install Bazelisk

Change directory into the user binaries directory:

cd /usr/bin

Download the bazelisk binary to this directory:

sudo wget https://github.com/bazelbuild/bazelisk/releases/download/v1.19.0/bazelisk-linux-arm64

Change this URL for a newer bazelisk.

Rename the binary file:

sudo mv bazelisk-linux-arm64 bazelisk

Make it executable:

sudo chmod +x bazelisk

Check that it was installed correctly:

bazelisk version

Install swift

Instructions from: https://www.swift.org/install/linux/

Change into the home directory:

cd ~/

Update the package registry:

sudo apt-get update

Install the package dependencies:

apt-get install \
          binutils \
          git \
          gnupg2 \
          libc6-dev \
          libcurl4-openssl-dev \
          libedit2 \
          libgcc-9-dev \
          libpython3.8 \
          libsqlite3-0 \
          libstdc++-9-dev \
          libxml2-dev \
          libz3-dev \
          pkg-config \
          tzdata \
          unzip \
          zlib1g-dev

Download the tar file:

wget https://download.swift.org/swift-5.9-release/ubuntu2004-aarch64/swift-5.9-RELEASE/swift-5.9-RELEASE-ubuntu20.04-aarch64.tar.gz

Uncompressed the tar file:

tar xzf swift-5.9-RELEASE-ubuntu20.04-aarch64.tar.gz

Delete the tar file:

rm -rf swift-5.9-RELEASE-ubuntu20.04-aarch64.tar.gz

Add the swift binaries to the PATH:

export PATH=/home/ubuntu/swift-5.9-RELEASE-ubuntu20.04-aarch64/usr/bin:"${PATH}"

Verify Swift was installed successfully:

swift --version

Use clang instead of gcc:

export CC=clang
brentleyjones commented 1 month ago

Based on https://github.com/bazelbuild/rules_swift/commit/2cc542ab55335dff09da8af42e4c69d7f005699d, we should probably unconditionally set it?

SwiftPM works by linking all the .o files, so this should end up in a state that seem users familiar with SwiftPM/Xcode will expect. Otherwise an @objc class might not be findable by name only, or a protocol conformance might not get included leading to confusing runtime problems.

AttilaTheFun commented 1 month ago

Yeah @brentleyjones I'd be in favor of matching the SPM behavior in swift_library. Then this wouldn't be necessary.

It could be gated behind a config if this is considered a breaking change.

brentleyjones commented 1 month ago

Since we are pre-1.0, I think we should just make the change.