Skip to content

Conversation

@pcbeard
Copy link
Contributor

@pcbeard pcbeard commented Sep 17, 2021

If neither Darwin, nor Glibc are available, use Foundation to import
math functions.

Use dynamic library target on Windows since ar tool doesn't seem to
exist.

Description

This change uses Foundation to import math functions on platforms where neither Darwin, nor Glibc exist (e.g. Windows).

Detailed Design

Defines a new compiler flag, FRB_MATH_FOUNDATION which is set in Package.swift if neither Darwin nor Glibc can be imported. It is possible that using Foundation may work mostly on all platforms, but I haven't investigated this yet.

#if FRB_MATH_FOUNDATION
import Foundation
#endif

Testing

Ran unit tests on Linux and Windows. All tests still pass on Linux. Only one fails on Windows:

Tests\FirebladeMathTests\FunctionTests.swift:31: error: FunctionTests.test_asinh : XCTAssertEqual failed: ("5.505348060078275") is not equal to ("5.505348060078276")

Looks like a difference in just one digit, probably due to a system library difference.

Performance

Should have no impact on performance of existing targets.

Source Impact

Should have no impact on existing users.

Checklist

  • I've read the Contribution Guidelines
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (to the extent possible).
  • I've added benchmarks covering new functionality (if appropriate).
  • I've verified that my change does not break any existing tests or introduce unexpected benchmark regressions.
  • I've updated the documentation (if appropriate).

If neither Darwin, nor Glibc are available, use Foundation to import
math functions.

Use dynamic library target on Windows since ar tool doesn't seem to
exist.
@pcbeard pcbeard requested a review from ctreffs as a code owner September 17, 2021 20:10
@ctreffs
Copy link
Member

ctreffs commented Sep 20, 2021

Thank you very much for your addition! 👍
I would really appreciate if we made a windows-ci for this as well - should be easy to set up.

# Conflicts:
#	Sources/FirebladeMath/Functions/sqrt.swift
#	Sources/FirebladeMath/Functions/tan.swift
#	Sources/FirebladeMath/Functions/tanh.swift
@ctreffs
Copy link
Member

ctreffs commented Sep 21, 2021

Thank you very much for your addition! 👍
I would really appreciate if we made a windows-ci for this as well - should be easy to set up.

I've updated the CI and fixed some import bugs. I would like you to make the Windows-CI work, then I we can merge this PR.
Thanks for your help :-)

@ctreffs
Copy link
Member

ctreffs commented Sep 21, 2021

With regards to the precission issues - XCTAssertEqual has functions where one can specify a certain accuracy. See https://developer.apple.com/documentation/xctest/equality_and_inequality_assertions#topics

@pcbeard
Copy link
Contributor Author

pcbeard commented Sep 22, 2021

I noticed there are ci scripts in .github/workflows and a lot of stuff in ci-windows.yml. I'm not really in any position to test this, other than to run a few of the commands. On my Windows system, with the Swift toolchain installed, I am able to build this library just fine with swift build and swift test. The rest of the setup of the workflow looks more like what you'd need if you were building the swift compiler itself. For example this section looks unnecessary:

    - name: Install Supporting Files
      run: |
          Copy-Item "$env:SDKROOT\usr\share\ucrt.modulemap" -destination "$env:UniversalCRTSdkDir\Include\$env:UCRTVersion\ucrt\module.modulemap"
          Copy-Item "$env:SDKROOT\usr\share\visualc.modulemap" -destination "$env:VCToolsInstallDir\include\module.modulemap"
          Copy-Item "$env:SDKROOT\usr\share\visualc.apinotes" -destination "$env:VCToolsInstallDir\include\visualc.apinotes"
          Copy-Item "$env:SDKROOT\usr\share\winsdk.modulemap" -destination "$env:UniversalCRTSdkDir\Include\$env:UCRTVersion\um\

I'm currently using:

swift-5.4.3-RELEASE-windows10.exe

@ctreffs
Copy link
Member

ctreffs commented Sep 22, 2021

Test Case 'Quat4f_EulerTests.testQuatFromEulerAngles_0_90_0' started at 2021-09-22 05:14:35.797

D:\a\math\math\Tests\FirebladeMathTests\Quat4f+EulerTests.swift:74: error: Quat4f_EulerTests.testQuatFromEulerAngles_0_90_0 : XCTAssertEqual failed: ("3.1415927") is not equal to ("3.1415925") - 

D:\a\math\math\Tests\FirebladeMathTests\Quat4f+EulerTests.swift:76: error: Quat4f_EulerTests.testQuatFromEulerAngles_0_90_0 : XCTAssertEqual failed: ("3.1415927") is not equal to ("3.1415925") - 
 
... 

Test Case 'Quat4f_EulerTests.testQuatFromEulerAngles_0_n90_0' started at 2021-09-22 05:14:35.819

D:\a\math\math\Tests\FirebladeMathTests\Quat4f+EulerTests.swift:91: error: Quat4f_EulerTests.testQuatFromEulerAngles_0_n90_0 : XCTAssertEqual failed: ("3.1415927") is not equal to ("3.1415925") - 

D:\a\math\math\Tests\FirebladeMathTests\Quat4f+EulerTests.swift:93: error: Quat4f_EulerTests.testQuatFromEulerAngles_0_n90_0 : XCTAssertEqual failed: ("3.1415927") is not equal to ("3.1415925") - 

There seem to be only two failing tests left of Windows. So this is gonna be working soon :-)

@ctreffs
Copy link
Member

ctreffs commented Sep 22, 2021

I noticed there are ci scripts in .github/workflows and a lot of stuff in ci-windows.yml. I'm not really in any position to test this, other than to run a few of the commands. On my Windows system, with the Swift toolchain installed, I am able to build this library just fine with swift build and swift test. The rest of the setup of the workflow looks more like what you'd need if you were building the swift compiler itself. For example this section looks unnecessary:

    - name: Install Supporting Files
      run: |
          Copy-Item "$env:SDKROOT\usr\share\ucrt.modulemap" -destination "$env:UniversalCRTSdkDir\Include\$env:UCRTVersion\ucrt\module.modulemap"
          Copy-Item "$env:SDKROOT\usr\share\visualc.modulemap" -destination "$env:VCToolsInstallDir\include\module.modulemap"
          Copy-Item "$env:SDKROOT\usr\share\visualc.apinotes" -destination "$env:VCToolsInstallDir\include\visualc.apinotes"
          Copy-Item "$env:SDKROOT\usr\share\winsdk.modulemap" -destination "$env:UniversalCRTSdkDir\Include\$env:UCRTVersion\um\

I'm currently using:

swift-5.4.3-RELEASE-windows10.exe

See: https://github.com/compnerd/swift-build/blob/master/.github/workflows/swift-format.yml as a current Swift windows-ci example with the latest version as comparison.
Will adjust the CI script again to reduce clutter.

@ctreffs ctreffs enabled auto-merge (squash) September 22, 2021 07:24
@ctreffs ctreffs merged commit 65aa6b5 into fireblade-engine:master Sep 22, 2021
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.

2 participants