Skip to content

Add support for Musl (#429)#430

Closed
arasan01 wants to merge 1 commit intojpsim:mainfrom
arasan01:main
Closed

Add support for Musl (#429)#430
arasan01 wants to merge 1 commit intojpsim:mainfrom
arasan01:main

Conversation

@arasan01
Copy link
Copy Markdown

@arasan01 arasan01 commented Sep 25, 2024

Summary

Change import to allow a choice between Glibc and Musl.

related Issue: #429
replacing musl pow: https://git.musl-libc.org/cgit/musl/tree/include/math.h

Check

$ xcrun --toolchain org.swift.600202409171a swift --version
Apple Swift version 6.0.2-dev (LLVM 43d73cb0cc589f7, Swift aaa632cea622394)
Target: arm64-apple-macosx14.0

$ xcrun --toolchain org.swift.600202409171a swift build --swift-sdk x86_64-swift-linux-musl -c release
Building for production...
[3/3] Compiling Yams Constructor.swift
Build complete! (7.32s)

$ xcrun --toolchain org.swift.600202409171a swift build -c release
Building for production...
[9/9] Compiling Yams Constructor.swift
Build complete! (16.99s)

Change import to allow a choice between Glibc and Musl
musl pow ref: https://git.musl-libc.org/cgit/musl/tree/include/math.h
#elseif canImport(Musl)
import CoreFoundation
import Musl
private let cpow: (_: Double, _: Double) -> Double = Musl.pow
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weren't native Swift math functions added to the STL in 2019?
https://github.com/swiftlang/swift/pull/23140/files

Do we still need to import any C libraries here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the background to the introduction of this, it now seems like it would be okay to use STL math functions. In that case, we would need to check that there are no problems with all toolchains. If we are only going to support Musl, I think it would have less of an impact if you just explicitly loaded cpow.

introduce commit: 682d498

commit message:

We must disambiguate `Double.pow(_: Double, _: Double) -> Double` and
`__C.pow(_: Double, _: Double) -> Double` as the Swift for TensorFlow
branch adds the former into the standard library.  The explicit module
dispatch ensures that we make the overload resolution unambiguous
allowing building Yams.

Copy link
Copy Markdown
Contributor

@bradleymackey bradleymackey Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this workaround was only to support some of the low level/standard library changes as part of Swift for Tensorflow. As that project is now archived (and has been for several years at this point), I think reverting these Tensorflow-specific workarounds is fine and probably desirable so we don't run into these issues with other platforms/libc. Implementation in #436.

@bradleymackey
Copy link
Copy Markdown
Contributor

bradleymackey commented Nov 28, 2024

Is there any possibility this can be merged? This is blocking SwiftLint from creating a static Linux build: realm/SwiftLint#3974

Prefer #436

@lynchsft
Copy link
Copy Markdown
Collaborator

lynchsft commented Dec 2, 2024

It needs a this CI fix

And I would really love to see the CI results of not messing with the C lib imports and instead relying on the Swift standard library.

@bradleymackey bradleymackey mentioned this pull request Dec 2, 2024
@SimplyDanny SimplyDanny mentioned this pull request Dec 14, 2024
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

Successfully merging this pull request may close these issues.

4 participants