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

Adds support for Musl #151

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Adds support for Musl #151

wants to merge 2 commits into from

Conversation

t089
Copy link

@t089 t089 commented Nov 13, 2024

Resolves #143.

This adds support for building the project with the "Swift Linux SDK".

For this imports of Glibc needs to be conditionalized for cases where musl is available.

Also there are some small API differences between musl and Glibc.

Also the target "OTelTesting" should be a testTarget otherwise it will be built together with the rest when invoking swift build. This fails though because the Swift Linux SDK does not contain any testing framework.

Also removes a seemingly unused import Foundation.

This adds support for building the project with the "Swift Linux SDK".

For this import of Glibc needs to conditionalized for cases where `musl` is available.

Also there are some small API differences between musl and Glibc.

Also the target "OTelTesting" should be a `testTarget` otherwise it is being built together with the rest when invoking `swift build`. This fails though because the Swift Linux SDK does not conatin any testing framework.
@t089
Copy link
Author

t089 commented Nov 18, 2024

Just realized that this is a duplicate of #148. The approach taken here is a bit different though:

  • This PR avoids the import of Foundation for OTelEnvironment
  • Instead of conditionally (not) compiling OTelTesting, this PR converts it into a testTarget so that it is only part of the "testing" build.

@lovetodream what do you think?

@lovetodream
Copy link

  • This PR avoids the import of Foundation for OTelEnvironment

In my PR, I've replaced environ with ProcessInfo.processInfo.environment (due to thread safety and no need for different implementations in glibc and musl), Hummingbird did the same some time ago. In that version the Foundation import would still be needed.

  • Instead of conditionally (not) compiling OTelTesting, this PR converts it into a testTarget so that it is only part of the "testing" build.

I am not sure if this is the right thing to do, as OTelTesting is not really a test target. It is more like a support library for writing tests for OTel (similar to NIOTestUtils in swift-nio), at least that's my understanding of it.

At the end, both approaches achieve the same goal, so either one is fine for me 😄
Let's see what @slashmo thinks about it.

@t089
Copy link
Author

t089 commented Nov 18, 2024

I am not sure if this is the right thing to do, as OTelTesting is not really a test target. It is more like a support library for writing tests for OTel (similar to NIOTestUtils in swift-nio), at least that's my understanding of it.

Yeah, fair. But NIOTestUtils does not depend on any testing framework. If you want to depend on a testing framework it needs to be a testTarget.

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

Successfully merging this pull request may close these issues.

Support Musl
2 participants