Skip to content

Conversation

danbev
Copy link
Member

@danbev danbev commented May 9, 2025

ruby : omit test_build_options locally

This commit omits the test for test_build_options when run locally as
it currently fails on Linux and MacOS platforms.

The motivation for this change is that currently when running the tests
locally on a non-macOS platform the test fails with the following error:

    .F
    ========================================================================
    Failure: test_build_options(TestPackage):
      <["ACCELERATE_FRAMEWORK",
       "CMAKE_OSX_ARCHITECTURES",
       "CMAKE_OSX_SYSROOT",
       "FOUNDATION_LIBRARY",
       "METALKIT_FRAMEWORK",
       "METAL_FRAMEWORK"]> was expected to be empty.
    /home/danbev/work/ai/whisper.cpp/bindings/ruby/tests/test_package.rb:43:in `test_build_options'
         40:     options = BuildOptions::Options.new
         41:     assert_empty options.missing_options
         42:     unless ENV["CI"]
      => 43:       assert_empty options.extra_options
         44:     end
         45:   end
         46: end
    ========================================================================

@danbev danbev requested a review from KitaitiMakoto May 9, 2025 13:50
Copy link
Collaborator

@KitaitiMakoto KitaitiMakoto left a comment

Choose a reason for hiding this comment

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

Thank you for maintaining even Ruby bindings.

Test for extra options is intended to detect removed options such as WHISPER_CCACHE, but not intended to maintain platform differences. When I run tests and this test failures, I check pull requests and determine we should remove the options or just ignore the failure.

Now options for Ruby bindings is union of options for all platforms because I thought it's too hard for me to check and maintain options for every platform, and extra options hurt nothing.

If someone or automation matrix could check and maintain platform differences, it's good to do so. Is that possible? If not, how about to add comment to describe above and keep Options class simple?

@danbev
Copy link
Member Author

danbev commented May 9, 2025

Now options for Ruby bindings is union of options for all platforms because I thought it's too hard for me to check and maintain options for every platform, and extra options hurt nothing.

Could we simply remove the asserts from test_build_options instead perhaps, as like you said if it does not hurt anything we could just log a message and manually inspect if required?

@KitaitiMakoto
Copy link
Collaborator

KitaitiMakoto commented May 9, 2025

Okay, just remove it now. Later I will add a switch for my purpose later.

@danbev
Copy link
Member Author

danbev commented May 9, 2025

Okay, just remove it now. Later I will add a switch for my purpose later.

Great, thanks!

This commit omits the test for `test_build_options` when run locally as
it currently fails on Linux and MacOS platforms.
`
The motivation for this change is that currently when running the tests
locally on a non-macOS platform the test fails with the following error:
```console
.F
========================================================================
Failure: test_build_options(TestPackage):
  <["ACCELERATE_FRAMEWORK",
   "CMAKE_OSX_ARCHITECTURES",
   "CMAKE_OSX_SYSROOT",
   "FOUNDATION_LIBRARY",
   "METALKIT_FRAMEWORK",
   "METAL_FRAMEWORK"]> was expected to be empty.
/home/danbev/work/ai/whisper.cpp/bindings/ruby/tests/test_package.rb:43:in `test_build_options'
     40:     options = BuildOptions::Options.new
     41:     assert_empty options.missing_options
     42:     unless ENV["CI"]
  => 43:       assert_empty options.extra_options
     44:     end
     45:   end
     46: end
========================================================================
```
@danbev danbev force-pushed the ruby-local-test branch from b70d2e7 to 116c5a9 Compare May 10, 2025 05:08
@danbev danbev changed the title ruby : add check for macOS in options.rb ruby : omit test_build_options locally May 10, 2025
@danbev danbev requested a review from KitaitiMakoto May 10, 2025 05:13
@danbev danbev merged commit 2e310b8 into ggml-org:master May 10, 2025
52 checks passed
@danbev danbev deleted the ruby-local-test branch May 22, 2025 04:22
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