-
Notifications
You must be signed in to change notification settings - Fork 147
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
Add support for Musl (#429) #430
base: main
Are you sure you want to change the base?
Conversation
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Prefer #436 |
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. |
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