Skip to content
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

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

Open
AttilaTheFun opened this issue Sep 11, 2024 · 10 comments

Comments

@AttilaTheFun
Copy link
Contributor

AttilaTheFun commented Sep 11, 2024

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: #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
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Collaborator

luispadron commented Sep 14, 2024

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

@AttilaTheFun
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@AttilaTheFun
Copy link
Contributor Author

AttilaTheFun commented Sep 17, 2024

@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
Copy link
Collaborator

brentleyjones commented Oct 4, 2024

Based on bazelbuild/rules_swift@2cc542a, 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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@brentleyjones
Copy link
Collaborator

#1385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants