rules_swift_package_manager icon indicating copy to clipboard operation
rules_swift_package_manager copied to clipboard

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

Open AttilaTheFun opened this issue 1 year ago • 10 comments

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

AttilaTheFun avatar Sep 11 '24 16:09 AttilaTheFun

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.

brentleyjones avatar Sep 13 '24 18:09 brentleyjones

@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.

AttilaTheFun avatar Sep 13 '24 20:09 AttilaTheFun

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

luispadron avatar Sep 14 '24 23:09 luispadron

@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.

AttilaTheFun avatar Sep 15 '24 00:09 AttilaTheFun

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

brentleyjones avatar Sep 17 '24 12:09 brentleyjones

@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

AttilaTheFun avatar Sep 17 '24 15:09 AttilaTheFun

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.

brentleyjones avatar Oct 04 '24 18:10 brentleyjones

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.

AttilaTheFun avatar Oct 04 '24 21:10 AttilaTheFun

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

brentleyjones avatar Oct 04 '24 21:10 brentleyjones

https://github.com/cgrindel/rules_swift_package_manager/pull/1385

brentleyjones avatar Dec 06 '24 15:12 brentleyjones